New issue
Advanced search Search tips

Issue 859156 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 20
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

FIRST_USE vote has wrong properties mask when updating via the bubble

Project Member Reported by cfroussios@chromium.org, Jun 29 2018

Issue description

1. 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  
 
After further investigation, I can reproduce with:
1. Visit facebook.com
2. Enter new credentials and save them via the key icon
3. Refresh

Expected:
No FIRST_USE vote uploaded

Observed:
FIRST_USE vote uploaded
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?
Cc: vabr@chromium.org dvadym@chromium.org
actually cc'ing.
See comment above
Looking at PasswordManager::OnPasswordFormsRendered, change-password forms get an exception for re-appearance, if they re-appear empty. Is this what you observe?
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.
Labels: -Pri-3 Pri-1
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)
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)?
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Cc: -vabr@chromium.org

Sign in to add a comment