I had a closer look at how PasswordFormManager (PFM) is used in the UI code. It basically just provides data about the saved form to allow it being stored, updated, or the associated statistics changed. It is not necessary to pass the whole PFM to the UI code, instead PFM should be able to return a copy of this data. This will prevent the refcounting, it will also prevent the need to move PFM out of PasswordManager, therefore covering the case when would have passed PFM out of PasswordManager and later needed to re-create it again.
I briefly checked this high-level plan with vasilii@, who agrees that sending a PFM to the UI code is not ideal. Because kolos@ is OOO today, and I'm concerned that people might start to build on top of r475889 in the meantime, making change of the design painful, I am going to revert r475889 now. As the next step, I'll start more detailed design for the above plan, and will work with kolos@ on finalising and implementing that.
We should also make sure to test our fallback against citicards.com. With the current chrome://flags/#enable-password-force-saving, I am still unable to save on citicards.com.
I did some performance testing of PFM::Clone (to be added in https://chromium-review.googlesource.com/c/559543). I ran the attached unittest in release build on my high-performance machine (HP620) and it repeatedly executed 100'000 iterations of Clone within 500 ms, leaving one Clone call under 5 µs. Given that the frequency of executing Clone is expected at the order of tens of seconds (likely much less frequent), the performance effect seems negligible.
TEST_F(PasswordFormManagerTest, Clone_Performance) {
CloningFormFetcher fetcher;
auto form_manager = base::MakeUnique<PasswordFormManager>(
password_manager(), client(), client()->driver(), *observed_form(),
base::MakeUnique<MockFormSaver>(), &fetcher);
fetcher.SetNonFederated({saved_match()}, 0u);
PasswordForm saved_login = *observed_form();
saved_login.username_value = ASCIIToUTF16("newuser");
saved_login.password_value = ASCIIToUTF16("newpass");
form_manager->ProvisionallySave(
saved_login, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
constexpr size_t kIterations = 100000;
std::unique_ptr<PasswordFormManager> clones[kIterations];
base::ElapsedTimer timer;
for (size_t i = 0; i < kIterations; ++i) {
clones[i] = form_manager->Clone();
}
base::TimeDelta elapsed = timer.Elapsed();
LOG(INFO) << kIterations << " of PFM::Clone made in "
<< elapsed.InMilliseconds() << " ms";
}
Comment 1 by bugdroid1@chromium.org
, May 31 2017