1166995 - Run login capture code when a form or a formless password field is removed from the document tree

Status: VERIFIED FIXED

(Toolkit :: Password Manager, enhancement, P3)

Description

[Matthew N. [:MattN]]

Pages can use JS to handle logins instead of using form submission events and we should try to capture the saved values in those cases. One heuristic is if a password field is removed from a document.

We can likely use a custom DOM event (like bug 1132211) or Mutation Observers/Events for this but whatever we use will likely need to have access to the removed element and the associated username field before it gets deleted.

Some edge cases to consider:
* Do we want to capture if the user closes a tab? What about the page doing window.close (e.g. Persona)? These may also trigger the input to be removed when the document gets destroyed.
* We don't want to capture when the document gets deleted from the bfcache.
* We probably don't want to cause a second capture doorhanger to appear if the values were already captured via other methods (e.g. onbeforesubmit or other heuristics).
* We probably don't want to display multiple capture doorhangers (replacing the previous one) when there were multiple password fields in the form (e.g. a password change or registration form with double-entry) as the user may have already dismissed the first one and reopening would interrupt the user.

Comment 1

[Dimi Lee [:dimi]]

Attachment: Bug 1166995 - P1. Notify DOMDocFetchSuccess when a fetch or XHR request completes successfully

The goal of this series of patches is to address the cases where sites
don't use standard form submission event.

The basic idea is inferring a form is submitted when the form is removed
from the DOM tree, but with one premise:
There must be a successful ajax request sent in the document before the
form is removed.

This is because websites usually send the credentials with an ajax
request (fetch or xhr) and then removes the form.

In summary, this patch does the following:

1. Add NotifyAjaxSuccess API in Document. The API sends a
   "DOMDocAjaxSuccess" event to who registers the event listener.

2. When a fetch request or a XMLHttpReuqest completes and succeeds
   (status code between 200~299), call NotifyAjaxSuccess().

3. Add SetAjaxSuccessEventListener API in Document.webidl.
   LoginMangerChild calls the API when there is user interaction on a
   form (or a form-less password field) to listen to AjaxSuccess event.

4. When LoginMangerChild receives an AjaxSuccess event, it resets the event
   listener to null (because the DOM mutation observer will be set up,
   we don't have to listen to the event anymore)

Comment 2

[Dimi Lee [:dimi]]

Attachment: Bug 1166995 - P2. Call onFormSubmit when a form or a password field is removed from the DOM tree

This patch uses MutationObserver to check whether a form or a form-less
password is removed from the DOM tree. If any node on the parent chain
of the form (or the form-less password filed) is removed, we consider
the node is removed.

This patch also adds a WeakFieldSet formlessModifiedPasswordFields to
record all the form-less password fields that users have interacted
with. This is because before this patch, we only record the "root
element" of a form or a password field, but in order to walk through
the parent chain of a form-less password field to observe DOM tree
changes, we need to have a data structure knowing the password field.

Depends on D106024

Comment 3

[Dimi Lee [:dimi]]

Attachment: Bug 1166995 - P3. Do not check isConnected when onFormSubmit is triggered by form removal inference

In Bug 1386283, we add isConnected to make sure we don't prompt saved
login doorhanger when a form is removed, and then a page navigation
happens afterward. However, the isConnected prevents the form removal
heuristic from working because the username/password fields are not connected
when a form is "removed".

This patch adds an additional parameter to ignore the isConnceted check
when _onFormSubmit is triggered by the form removal inference.

Depends on D106025

Comment 4

[Dimi Lee [:dimi]]

Attachment: Bug 1166995 - P4. Add testcases

Depends on D106026

Comment 5

[Sam Foster [:sfoster] (he/him)]

Attachment: removeFormLoginCapture.html

I think this should be a valid test case?

• If you click either button without interacting with the form it shouldn't put up the capture prompt
• If you put in a password value and click "Just remove form" it shouldn't put up the capture prompt
• If you put in a password value and click "Fetch and remove form" it should prompt to save the password.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: removeFormLoginCapture.html

Adding a valid url to ensure the fetch is successful.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Attachment: removeFormLoginCapture.html test case

Comment 8

[Sam Foster [:sfoster] (he/him)]

:dimi, can you verify attachment 9208834 [details] is a valid test case? That's not working for me... I did confirm the patch works as expected on bigcommerce.com, but I'd like to have a reduced case for this bug.

Comment 9

[Dimi Lee [:dimi]]

Attachment: Bug 1166995 - P5. Add a preference to control whether to capture login when a form is removed

Add preference signon.formRemovalCapture.enabled, default on

Depends on D106027

Comment 10

[Dimi Lee [:dimi]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #8)

:dimi, can you verify attachment 9208834 [details] is a valid test case? That's not working for me... I did confirm the patch works as expected on bigcommerce.com, but I'd like to have a reduced case for this bug.

Hi Sam,
Yes I have verified the patch worked by using the attachment with all the testing scenarios listed in Comment 5.
Not sure why it doesn't work in your environment. Did you include patch P3 while testing?
Anyway, I rebased and updated all the patches, could you help use the latest one and check again? thank you!

Comment 11

[Sam Foster [:sfoster] (he/him)]

(In reply to Dimi Lee [:dimi][:dlee] from comment #10)

Yes I have verified the patch worked by using the attachment with all the testing scenarios listed in Comment 5.
Not sure why it doesn't work in your environment. Did you include patch P3 while testing?
Anyway, I rebased and updated all the patches, could you help use the latest one and check again? thank you!

Ok, I pulled everything down and built again (I had clobbered earlier in the day). This time attachment 9208834 [details] seemed to work ok.
I also checked the login page on bigcommerce.com - I found that my breakpoint in LoginManagerChild.jsm#_onFormSubmit showed that was a regular form submission now? Maybe they redesigned the page..

I've added my r+ to the patches. If you have r+ from :smaug for the parts he had been looking at, I think we are ready to land this.

Comment 12

[Dimi Lee [:dimi]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #11)

Ok, I pulled everything down and built again (I had clobbered earlier in the day). This time attachment 9208834 [details] seemed to work ok.
I also checked the login page on bigcommerce.com - I found that my breakpoint in LoginManagerChild.jsm#_onFormSubmit showed that was a regular form submission now? Maybe they redesigned the page..

bigcommerce uses regular form submission in their login page, but not in registration page. So to test this bug, we'll have to test the registration page.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c02d83b9352
P1. Notify DOMDocFetchSuccess when a fetch or XHR request completes successfully r=smaug,sfoster
https://hg.mozilla.org/integration/autoland/rev/42db612fa2c8
P2. Call onFormSubmit when a form or a password field is removed from the DOM tree r=sfoster,tgiles,smaug
https://hg.mozilla.org/integration/autoland/rev/883288ee1291
P3. Do not check isConnected when onFormSubmit is triggered by form removal inference r=sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/29072892d937
P4. Add testcases r=sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/b932f18c2551
P5. Add a preference to control whether to capture login when a form is removed r=sfoster,tgiles

Comment 14

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/5c02d83b9352
https://hg.mozilla.org/mozilla-central/rev/42db612fa2c8
https://hg.mozilla.org/mozilla-central/rev/883288ee1291
https://hg.mozilla.org/mozilla-central/rev/29072892d937
https://hg.mozilla.org/mozilla-central/rev/b932f18c2551

Comment 15

[Timea Cernea [:tbabos][inactive]]

NI? myself to not forget QA verification.

Comment 16

[Timea Cernea [:tbabos][inactive]]

Marking this as verified-fixed based on the verifications done on the latest Nightly 89 and Beta 88.0b2 in the blocking bugs.