Autofill doesn't work when using drag to new window

VERIFIED FIXED in Firefox 56

Status

()

defect
--
major
VERIFIED FIXED
2 years ago
3 months ago

People

(Reporter: Ovidiu, Assigned: MattN)

Tracking

(Depends on 1 bug, Blocks 1 bug)

56 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 disabled, firefox55 disabled, firefox56+ verified)

Details

(Whiteboard: [form autofill:M4])

Attachments

(6 attachments, 1 obsolete attachment)

Reporter

Description

2 years ago
[Environment:]

Windows, Mac, Linux

Nightly 56.0a1 20170705170357 

[Steps:]

Preconditions
Go to Preferences/ Privacy and Security / Form Autofill / Enable Profile autofill. (default in Nightly)
Make sure you have at least two saved profile.


1.Open Firefox.
2.Open the attached Html in FF window. 
3.Double click in the Tel textbox.
4.Drag the tab out from the FF window.
5.3.Double click in the Tel textbox from the new window that was created. 


[Actual Result:]
A new FF window is created but the autofill doesn't work.  


[Expected Result:]

Autofill should work in the new window.
Note: The above scenario is a reduced one: any autofill form or any autofill field used will fail to trigger autocomplete in the "Move to a new window" or "drag to a new window" scenario.
When a tab is dragged to a new window, the fields in the tab should be marked again.
`identifyAutofillFields`[1] returns early for the performance concern, and that's the cause of this issue.

I am going to prepare a patch to fix this issue and not to affect the performance.

Here is an one line patch to fix this issue:
diff --git a/browser/extensions/formautofill/FormAutofillContent.jsm b/browser/extensions/formautofill/FormAutofillContent.jsm
index 4535c7e8571c..1667e2a7a028 100644
--- a/browser/extensions/formautofill/FormAutofillContent.jsm
+++ b/browser/extensions/formautofill/FormAutofillContent.jsm
@@ -468,7 +468,6 @@ var FormAutofillContent = {
       formHandler = new FormAutofillHandler(formLike);
     } else if (!formHandler.isFormChangedSinceLastCollection) {
       this.log.debug("No control is removed or inserted since last collection.");
-      return;
     }

     formHandler.collectFormFields();


[1] http://searchfox.org/mozilla-central/rev/cef8389c687203085dc6b52de2fbd0260d7495bf/browser/extensions/formautofill/FormAutofillContent.jsm#469-471
Assignee: nobody → selee
Status: NEW → ASSIGNED
Whiteboard: [form autofill:M4]
Password Manager Popup(in short, PassMgrPopup) for ID and Password work before dragging the tab to another window.

00:10 Drag the tab to another window.
00:16 Show the correct PassMgrPopup for Password.
00:20 Show the form history popup rather than PassMgrPopup for ID.

After reloading the tab at 00:28, PassMgrPopup for ID and Password work correctly again.
Password Manager Popup has the incorrect behavior (see comment 3), but the incorrect behavior is not exact the same with FormAutofill.

The investigation in comment 2 indicates that the memory for marking fields seems not existing in FormController anymore in the new window in my rough surmise.
If we want to have a short-term fix for FormAutofill issue, a solution is to invoke `formHandler.collectFormFields();` after dragging a tab to another window. However, I need to find a way to know if the tab is dragged to another window.

If we want to dive deeper, we could find the root cause of the memory issue and fix both Password Manager and FormAutofill probably. As you may expect, this costs more time.
Some related notes here:
* Both e10s and non-e10s mode have the same issue.
* In e10s mode, the original tab has the same PID with the new window one.
Hi Matt, is this something that can be triaged and fixed soon? SoftVision has identified this bug as a gating issue for 56 pre-beta merge. Thanks!
Flags: needinfo?(MattN+bmo)
(In reply to Ritu Kothari (:ritu) from comment #6)
> Hi Matt, is this something that can be triaged and fixed soon? SoftVision
> has identified this bug as a gating issue for 56 pre-beta merge. Thanks!

I already triaged this last week with the team and in my opinion it's not a merge blocker as the same issue has existed for password manager for many years, the STR don't seem super common, and the failure case is simply that the user gets form history instead of autofill.

I believe Vance has discussed with the QA team and this isn't a merge pre-merge issue anymore.

I have been investigating today and will attach a potential fix now but I'm not sure if it will be acceptable.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/SwapDocShells
Flags: needinfo?(MattN+bmo)
This doesn't meet the definition of critical IMO: "Crashes, loss of data, severe memory leak"
Severity: critical → major
While moving to a new window (using SwapDocShells), a pagehide event[1] is dispatched causing nsFormFillController to clean up mPwmgrInputs and mAutofillInputs for the document. This commit changes the pagehide handler to not clear the hash tables with persisted=true (which would also fix the same bug in password manager autocomplete).

This approach comes at the cost of increased memory (hash table entries for fields in session history) but would reduce CPU usage compared to the alternative of re-marking password manager and autofill fields upon every pageshow event. This approach also solves the issue of autofill and password manager autocomplete not working after session history navigation.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/SwapDocShells

Review commit: https://reviewboard.mozilla.org/r/159150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159150/
Assignee: selee → MattN+bmo
Comment on attachment 8888204 [details]
Bug 1378754 - Don't clear marked fields in nsFormFillController upon a persisted pagehide.

https://reviewboard.mozilla.org/r/159150/#review164530

::: toolkit/components/satchel/nsFormFillController.cpp:985
(Diff revision 1)
> -      RemoveForDocument(doc);
> +      // Only remove the marked autofill and password manager fields if the page
> +      // isn't going to be persisted (i.e. it's being unloaded) so that
> +      // appropriate autocomplete handling works with bfcache.
> +      bool persisted = aEvent->InternalDOMEvent()->AsPageTransitionEvent()->Persisted();

Reminder to myself to write a test

Comment 12

2 years ago
mozreview-review
Comment on attachment 8888204 [details]
Bug 1378754 - Don't clear marked fields in nsFormFillController upon a persisted pagehide.

https://reviewboard.mozilla.org/r/159150/#review164698

Maybe I'm missing something, but what happens to the data if page is never getting back to life from bfcache. I mean, what if bfcache just get evicted?
If we clear the data in some other way when bfcache is evicted, explain and ask review again.
Attachment #8888204 - Flags: review?(bugs) → review-
Assignee

Comment 13

2 years ago
mozreview-review-reply
Comment on attachment 8888204 [details]
Bug 1378754 - Don't clear marked fields in nsFormFillController upon a persisted pagehide.

https://reviewboard.mozilla.org/r/159150/#review164698

The NodeWillBeDestroyed mutation observer should handle this since MaybeRemoveMutationObserver doesn't remove the observer from marked fields.
Attachment #8888204 - Flags: review- → review?(bugs)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8888204 [details]
Bug 1378754 - Don't clear marked fields in nsFormFillController upon a persisted pagehide.

https://reviewboard.mozilla.org/r/159150/#review164718

ok, we do explicitly remove nodes from hashtables when they are about to be deleted.
Attachment #8888204 - Flags: review?(bugs) → review+
I think dragging to a new window is a fairly common scenario and if form autofill is meant to improve usability, we need to make sure it works in most commonly used scenarios. Happy to see a patch already r+'d.
The autofill and password manager markings will be removed by NodeWillBeDestroyed or RemoveWindowListeners instead.

Review commit: https://reviewboard.mozilla.org/r/159538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159538/
Attachment #8888559 - Flags: review?(bugs)
Comment on attachment 8888559 [details]
Bug 1378754 - Don't clear marked fields in nsFormFillController upon pagehide.

https://reviewboard.mozilla.org/r/159538/#review164900

::: toolkit/components/satchel/nsFormFillController.cpp
(Diff revision 1)
> -
> -      RemoveForDocument(doc);

The last approach had some crashes for key->OwnerDoc within RemoveForDocument https://treeherder.mozilla.org/#/jobs?repo=try&revision=111f29ac2345 which I don't fully understand yet but I realized after our discussion on IRC this morning that perhaps a better solution is to simply not call RemoveForDocument upon pagehide because the hash table entries will be removed by NodeWillBeDestroyed anyways. What do you think?
Ah, we shouldn't have removed the mutation observer when !aRemoveMarkings

Comment 19

2 years ago
mozreview-review
Comment on attachment 8888559 [details]
Bug 1378754 - Don't clear marked fields in nsFormFillController upon pagehide.

https://reviewboard.mozilla.org/r/159538/#review165274

I think I prefer the previous approach.
Attachment #8888559 - Flags: review?(bugs)
Attachment #8888559 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 23

2 years ago
mozreview-review-reply
Comment on attachment 8888559 [details]
Bug 1378754 - Don't clear marked fields in nsFormFillController upon pagehide.

https://reviewboard.mozilla.org/r/159538/#review165274

After discussing on IRC we agreed that the middle approach of only calling the existing RemoveForDocuemnt if `!persisted` was best.
Comment hidden (mozreview-request)

Comment 25

2 years ago
mozreview-review
Comment on attachment 8888203 [details]
Bug 1378754 - Add MOZ_LOG logging to nsFormFillController.cpp.

https://reviewboard.mozilla.org/r/159148/#review166076

::: toolkit/components/satchel/nsFormFillController.cpp:241
(Diff revision 2)
>  }
>  
>  void
>  nsFormFillController::NodeWillBeDestroyed(const nsINode* aNode)
>  {
> +  MOZ_LOG(sLogger, LogLevel::Verbose, ("NodeWillBeDestroyed: %p", aNode));

Just a trivial question: I see some logs using verbose and some using debug, may I know why we applied 2 different type of log here?
Attachment #8888203 - Flags: review?(schung) → review+

Comment 26

2 years ago
mozreview-review-reply
Comment on attachment 8888203 [details]
Bug 1378754 - Add MOZ_LOG logging to nsFormFillController.cpp.

https://reviewboard.mozilla.org/r/159148/#review166076

BTW most of logs here seems not related to the solution itself because most of functions won't be called since doc is cached without refresh IIUC. Is there any difference can be found in the log after problem addressed(except the log for RemoveForDocument)?

Comment 27

2 years ago
mozreview-review
Comment on attachment 8888955 [details]
Bug 1378754 - Test that autofill and login fields remain marked after a persisted pagehide.

https://reviewboard.mozilla.org/r/159970/#review166100

::: browser/extensions/formautofill/test/browser/browser_autocomplete_marked_back_forward.js:54
(Diff revision 2)
> +    browser.goForward();
> +    await stoppedPromise;
> +    await openPopupOn(browser, "#street-address");
> +    checkPopup(autoCompletePopup);
> +
> +    // Ensure the popup is closed before entering the next test.

nit: Maybe we can move this part to head. Here we only need to call `await closePopup(browser);` and replace the element.blur() with document.focus() <= not 100% sure if it works, but IIRC element blur is equal to document focus technically.
Attachment #8888955 - Flags: review?(schung) → review+
Assignee

Comment 28

2 years ago
mozreview-review-reply
Comment on attachment 8888203 [details]
Bug 1378754 - Add MOZ_LOG logging to nsFormFillController.cpp.

https://reviewboard.mozilla.org/r/159148/#review166076

The log lines were useful for debugging the problem and would have been useful for debugging some of the other changes to this file in that last year. I think the lack of a RemoveForDocument line would during a persisted pagehide will be the only difference.

> Just a trivial question: I see some logs using verbose and some using debug, may I know why we applied 2 different type of log here?

The logging that is more verbose is at the verbose level, and the less verbose logging for debugging is at the debug level. So it was based on a subjective opionion of how verbose a log line will be.
Assignee

Comment 29

2 years ago
mozreview-review-reply
Comment on attachment 8888955 [details]
Bug 1378754 - Test that autofill and login fields remain marked after a persisted pagehide.

https://reviewboard.mozilla.org/r/159970/#review166100

> nit: Maybe we can move this part to head. Here we only need to call `await closePopup(browser);` and replace the element.blur() with document.focus() <= not 100% sure if it works, but IIRC element blur is equal to document focus technically.

I tried different options of .focus and .blur without giving an specific element ID and it didn't seem to work. We really should fix the user-facing bug so our tests wouldn't even need to manually close the popup. I think this is bug 1344390 but I thought there may be another.

Comment 30

2 years ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/af4e5e48ca54
Add MOZ_LOG logging to nsFormFillController.cpp. r=steveck
https://hg.mozilla.org/integration/autoland/rev/b5a0702e2572
Don't clear marked fields in nsFormFillController upon a persisted pagehide. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d9fa2317c55a
Test that autofill and login fields remain marked after a persisted pagehide. r=steveck

Comment 31

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/af4e5e48ca54
https://hg.mozilla.org/mozilla-central/rev/b5a0702e2572
https://hg.mozilla.org/mozilla-central/rev/d9fa2317c55a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1384779
Reporter

Comment 32

2 years ago
Posted video Recording #12.mp4
Hi Matt,

I wanted to verify this issue and I found another one, but you need to do 1 more step: please reattached the tab that was detached and double click on any field, you will see that drop-down doesn't work. 

Please tell if I need to log a new bug for this, or it will be handled in this one? 

There is also an attached video with the issue.
Flags: needinfo?(MattN+bmo)
Depends on: 1385901
Marking this issue as verified on: 56.0a1 20170728100358 - Ubuntu 16.04, Windows 10, Mac OSX 10.12.

For comment32 scenario, added bug 1385901.
Status: RESOLVED → VERIFIED
Flags: needinfo?(MattN+bmo)
See Also: → 1421634
You need to log in before you can comment on or make changes to this bug.