Closed Bug 1378754 Opened 3 years ago Closed 3 years ago

Autofill doesn't work when using drag to new window

Categories

(Toolkit :: Form Manager, defect)

56 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox54 --- disabled
firefox55 --- disabled
firefox56 + verified

People

(Reporter: Ovidiu, Assigned: MattN)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M4])

Attachments

(6 files, 1 obsolete file)

Attached file Tel @autocomplete.htm
[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]
Attached video Password Manager Popup
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)
See Also: → 936026
(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 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-
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 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 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 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 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 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 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+
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.
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.
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
Attachment #8888203 - Flags: review?(dolske)
Depends on: 1384779
Attached 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)
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
Blocks: 936026
You need to log in before you can comment on or make changes to this bug.