FIRST_USE vote has wrong properties mask when updating via the bubble |
|||||
Issue description1. Visit facebook.com 2. Enter and save new credentials 3. (log out and) return to facebook.com 4. Observe that credentials are filled 5. Modify the password 6. Click the key icon and accept the update of the credentials Expected: Chrome will upload a crowdsourcing vote where the properties mask is: username: not focused, autofilled, not typed, known password: focused, not autofilled, typed, not known Observed: username: not focused, autofilled, not typed, not known password: not focused, autofilled, not typed, not known
,
Jul 2
I've narrowed it down further. We are seeing a successful submission every single time we visit facebook.com and we have saved credentials to fill. Chrome version 69.0.3480.0 The relevant piece of that stack #0 0x7fcd447e70fc base::debug::StackTrace::StackTrace() #1 0x558793f20994 password_manager::VotesUploader::SetKnownValueFlag() #2 0x558793f1f5a5 password_manager::VotesUploader::UploadFirstLoginVotes() #3 0x558793ef6b5a password_manager::PasswordFormManager::ProcessUpdate() #4 0x558793ef6978 password_manager::PasswordFormManager::Save() #5 0x558793ef1d4e password_manager::PasswordManager::OnLoginSuccessful() #6 0x558793ef301a password_manager::PasswordManager::OnPasswordFormsRendered() +dvadym, vabr This seems to go against our condition for successful submission: not seeing the forms again. Has any of this changed intentionally?
,
Jul 2
actually cc'ing. See comment above
,
Jul 3
Looking at PasswordManager::OnPasswordFormsRendered, change-password forms get an exception for re-appearance, if they re-appear empty. Is this what you observe?
,
Jul 3
There is also PasswordManager::OnPasswordFormSubmittedNoChecks which is used in cases when Chrome gives up on checking the submission success (such as during submission through in-page navigation). But looking at your stack trace, this is not what you are observing.
,
Jul 16
Sorry the delay, forwarding from my chromium account was disabled :/ I see neither of these cases. The are actually no |visible_forms| on the call which results in successful login detection. PasswordManager::OnPasswordFormsRendered() gets called multiple times per page load and only one results in successful submission (and it happens before the call which actually announces the visible forms). Are the multiple calls expected? And if so, which is the condition which is supposed to filter them out? CanProvisionalManagerSave()? (also increasing priority, since this affects issue 838979)
,
Jul 16
The following metric is affected by this bug and can be monitored to verify upcoming fixes. No-sumbission should approach 0% . https://uma.googleplex.com/p/chrome/histograms/?endDate=20180714&dayCount=1&histograms=PasswordManager.AcceptedSaveUpdateSubmissionIndicatorEvent%2CPasswordManager.SuccessfulSubmissionIndicatorEvent&fixupData=true&showMax=true&filters=isofficial%2Ceq%2CTrue&implicitFilters=isofficial
,
Jul 17
I think PasswordManager::OnPasswordFormsRendered should only happen once per frame. Do you see it happen multiple times for the same frame (equivalently, for the same PasswordManagerDriver)?
,
Jul 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eb26bb6df519a6420489ac2af91c7d562d4c3379 commit eb26bb6df519a6420489ac2af91c7d562d4c3379 Author: Christos Froussios <cfroussios@chromium.org> Date: Wed Jul 18 14:18:21 2018 [Password Manager] Manual fallbacks don't set the |provisional_save_manager| When showing the manual fallbacks, we create a PasswordFormManager and pass it to the UI. Previously, this was done by creating a provisional save manager and moving it, but if we failed to move it, it would remain and signal that a submission in the previous navigation. I've refactored the showing of the manual fallbacks to have no side effects on |provisional_save_manager|. Bug: 859156 Change-Id: Iaf41b201bc6c812f483fdb355f8414a77b0a034f Reviewed-on: https://chromium-review.googlesource.com/1138322 Commit-Queue: Christos Froussios <cfroussios@chromium.org> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Cr-Commit-Position: refs/heads/master@{#576036} [modify] https://crrev.com/eb26bb6df519a6420489ac2af91c7d562d4c3379/components/password_manager/core/browser/password_manager.cc [modify] https://crrev.com/eb26bb6df519a6420489ac2af91c7d562d4c3379/components/password_manager/core/browser/password_manager.h [modify] https://crrev.com/eb26bb6df519a6420489ac2af91c7d562d4c3379/components/password_manager/core/browser/password_manager_unittest.cc
,
Sep 20
Closing this. The issue is not observed anymore. The metric https://uma.googleplex.com/p/chrome/histograms/?endDate=20180714&dayCount=1&histograms=PasswordManager.AcceptedSaveUpdateSubmissionIndicatorEvent%2CPasswordManager.SuccessfulSubmissionIndicatorEvent&fixupData=true&showMax=true&filters=isofficial%2Ceq%2CTrue&implicitFilters=isofficial has improved noticeably, but it's not fully fixed. There are probably more causes.
,
Nov 29
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by cfroussios@chromium.org
, Jul 2