New issue
Advanced search Search tips

Issue 760579 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 774955
issue 808410



Sign in to add a comment

Password doesn't save when logging in

Project Member Reported by pinkerton@chromium.org, Aug 30 2017

Issue description

Chrome Version: M62 dev
OS: iOS10.3.x iPadPro
MacOS 10.12, MacBookPro

- Have a previously created password for www.thinkpacifica.com, but not saved in Chrome
- Go to https://www.thinkpacifica.com/app#/app/login
- Sign in

expected:

- should offer to save password to chrome

actual:

- nothing happens

Note that this doesn't work on macOS either. It might be all platforms, but I haven't tried them all. 
 

Comment 1 by sczs@chromium.org, Aug 30 2017

Owner: vasi...@chromium.org
Status: Assigned (was: Untriaged)
vasilii@ could you PTAL 
Owner: ----
Status: Available (was: Assigned)
The bug is in the bucket for the team.
Labels: OS-Android OS-Chrome OS-Linux OS-Windows
Password Manager doesn't see any events that could look like password form submission. It's not clear how can we fix it.

From M-62 it's possible to use manual fallbacks for saving credentials: there is a key icon on the right of address bar after typing in password field, clicking on it allow to save credentials.
Blocking: 774955
So when is Chrome supposed ask the user if they'd like to save the passwords? Only in password creation forms?
Chrome doesn't ask to save password on this site at all even on Desktop.

A user can click on a key icon and save password on Desktop (that's what fallbacks for).
From a product perspective: Chrome is supposed to ask the user to save passwords on all forms (sign-up, sign-in, change password) where the action is successful (user created an account, signed in, changed password).

From an eng perspective: We can only ask the user if we see that the user submitted a form. We should have a fallback for the user for sites where we don't see form submissions. Having a fallback does not mean that we should not try to fix the issue that we fail to see form submissions.
Thanks. That answers my question in #5.

Comment 9 by vabr@chromium.org, Jan 9 2018

Cc: -vabr@chromium.org
Here is what happens:

The FormTracker uses a WebFormElementsObserverImpl to be informed when the login form is deleted: https://cs.chromium.org/chromium/src/components/autofill/content/renderer/form_tracker.cc?l=236&rcl=20d3e4e6f5e91361f76ba2fc67c956d729cb0bae

The WebFormElementsObserverImpl only registers to be notified if the form is removed from the parent element:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/exported/WebFormElementObserverImpl.cpp?l=53-57&rcl=20d3e4e6f5e91361f76ba2fc67c956d729cb0bae

The structure of the DOM on the page is more or less as follows:
<div class="content">
  <div class="form-wrapper">
    <form>...</form>
  </div>
</div>
Instead of removing the <form> from the DOM, the <div class="content"> is removed.
Labels: -Pri-3 Pri-1
Owner: battre@chromium.org
Status: Assigned (was: Available)
Checked with Jochen and the current behavior is not intended. This should be a pretty straight forward fix (or a performance nightmare). As this potentially affects a bigger number of sites, I am increasing the priority.
Status: Started (was: Assigned)
CL is here https://chromium-review.googlesource.com/c/chromium/src/+/892882

Fixing this on iOS will be a totally different thing, though.
Thanks. 
Should there be a different bug to track the iOS fix or we keep this one?
I'd say let's make a different bug for iOS so Test verification isn't a nightmare as the timing of the two fixes will be different. 
Project Member

Comment 15 by bugdroid1@chromium.org, Feb 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fadf05485d1a5703562fc7639f5b099875f7fe18

commit fadf05485d1a5703562fc7639f5b099875f7fe18
Author: Dominic Battre <battre@chromium.org>
Date: Fri Feb 02 13:15:47 2018

More robust capturing <form>s that are taken out of the DOM.

Chrome's password manager tries to capture events where <form> elements
are taken out of the DOM or made invisible. This is used to detect
form submissions. This trigger failed in the past if a parent element
for the form was removed from the DOM or made invisible (as opposed
to the form it self). This CL makes the trigger logic more robust.

Bug:  760579 ,  710431 
Change-Id: Idbc72d7ab57d9e6c61b0907ac5c9b3f96346d4a5
Reviewed-on: https://chromium-review.googlesource.com/892882
Commit-Queue: Dominic Battré <battre@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534037}
[modify] https://crrev.com/fadf05485d1a5703562fc7639f5b099875f7fe18/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
[modify] https://crrev.com/fadf05485d1a5703562fc7639f5b099875f7fe18/third_party/WebKit/Source/core/exported/WebFormElementObserverImpl.cpp

Blocking: 808410
Labels: -OS-iOS
Status: Fixed (was: Started)
I have filed issue 808410 for iOS.
Cc: nyerramilli@chromium.org ligim...@chromium.org ajha@chromium.org
 Issue 593288  has been merged into this issue.
Project Member

Comment 19 by bugdroid1@chromium.org, Feb 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0fd617ef5b871cbaaa3c2197e43617c594aa273f

commit 0fd617ef5b871cbaaa3c2197e43617c594aa273f
Author: Adam Klein <adamk@chromium.org>
Date: Wed Feb 07 23:47:39 2018

Revert "More robust capturing <form>s that are taken out of the DOM."

This reverts commit fadf05485d1a5703562fc7639f5b099875f7fe18.

Reason for revert: Speculative revert due to Dev-blocking crash bug

See https://crbug.com/808911#c25 for more details.

Original change's description:
> More robust capturing <form>s that are taken out of the DOM.
> 
> Chrome's password manager tries to capture events where <form> elements
> are taken out of the DOM or made invisible. This is used to detect
> form submissions. This trigger failed in the past if a parent element
> for the form was removed from the DOM or made invisible (as opposed
> to the form it self). This CL makes the trigger logic more robust.
> 
> Bug:  760579 ,  710431 
> Change-Id: Idbc72d7ab57d9e6c61b0907ac5c9b3f96346d4a5
> Reviewed-on: https://chromium-review.googlesource.com/892882
> Commit-Queue: Dominic Battré <battre@chromium.org>
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#534037}

TBR=battre@chromium.org,ojan@chromium.org,jochen@chromium.org
Bug:  760579 ,  710431 , 808911

Change-Id: I22af4860f9944b2a80bc5aae19048109a66b84c3
Reviewed-on: https://chromium-review.googlesource.com/906845
Reviewed-by: Adam Klein <adamk@chromium.org>
Commit-Queue: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535192}
[modify] https://crrev.com/0fd617ef5b871cbaaa3c2197e43617c594aa273f/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
[modify] https://crrev.com/0fd617ef5b871cbaaa3c2197e43617c594aa273f/third_party/WebKit/Source/core/exported/WebFormElementObserverImpl.cpp

Project Member

Comment 20 by bugdroid1@chromium.org, Feb 8 2018

Labels: merge-merged-3342
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/72dfba7281ad7706c8a1d72a61e376fa02579e66

commit 72dfba7281ad7706c8a1d72a61e376fa02579e66
Author: Adam Klein <adamk@chromium.org>
Date: Thu Feb 08 01:23:06 2018

Revert "More robust capturing <form>s that are taken out of the DOM."

This reverts commit fadf05485d1a5703562fc7639f5b099875f7fe18.

Reason for revert: Speculative revert due to Dev-blocking crash bug

See https://crbug.com/808911#c25 for more details.

Original change's description:
> More robust capturing <form>s that are taken out of the DOM.
> 
> Chrome's password manager tries to capture events where <form> elements
> are taken out of the DOM or made invisible. This is used to detect
> form submissions. This trigger failed in the past if a parent element
> for the form was removed from the DOM or made invisible (as opposed
> to the form it self). This CL makes the trigger logic more robust.
> 
> Bug:  760579 ,  710431 
> Change-Id: Idbc72d7ab57d9e6c61b0907ac5c9b3f96346d4a5
> Reviewed-on: https://chromium-review.googlesource.com/892882
> Commit-Queue: Dominic Battré <battre@chromium.org>
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#534037}

(cherry picked from commit 0fd617ef5b871cbaaa3c2197e43617c594aa273f)

Change-Id: Ie779c0bb36df4319a6220b6b10c02434ec26984c
Tbr: battre@chromium.org,ojan@chromium.org,jochen@chromium.org
Bug:  760579 ,  710431 , 808911
Reviewed-on: https://chromium-review.googlesource.com/907598
Reviewed-by: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/branch-heads/3342@{#5}
Cr-Branched-From: 7a08be580eb458170d11b34a8de66fe08db41bc5-refs/heads/master@{#534887}
[modify] https://crrev.com/72dfba7281ad7706c8a1d72a61e376fa02579e66/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
[modify] https://crrev.com/72dfba7281ad7706c8a1d72a61e376fa02579e66/third_party/WebKit/Source/core/exported/WebFormElementObserverImpl.cpp

Project Member

Comment 21 by bugdroid1@chromium.org, Feb 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fd8c26e0c11b2db8df69b7b5a3937c3e0eb63f67

commit fd8c26e0c11b2db8df69b7b5a3937c3e0eb63f67
Author: Dominic Battre <battre@chromium.org>
Date: Fri Feb 09 11:00:25 2018

More robust capturing <form>s that are taken out of the DOM.

Chrome's password manager tries to capture events where <form> elements
are taken out of the DOM or made invisible. This is used to detect
form submissions. This trigger failed in the past if a parent element
for the form was removed from the DOM or made invisible (as opposed
to the form it self). This CL makes the trigger logic more robust.

Bug:  760579 ,  710431 ,  593288 , 808911
Change-Id: I3dbe4618a8811da3299a1884bc37db3d46a79adb
Reviewed-on: https://chromium-review.googlesource.com/910968
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Dominic Battré <battre@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535682}
[modify] https://crrev.com/fd8c26e0c11b2db8df69b7b5a3937c3e0eb63f67/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
[modify] https://crrev.com/fd8c26e0c11b2db8df69b7b5a3937c3e0eb63f67/third_party/WebKit/Source/core/exported/WebFormElementObserverImpl.cpp

Sign in to add a comment