Closed
Bug 1378754
Opened 7 years ago
Closed 7 years ago
Autofill doesn't work when using drag to new window
Categories
(Toolkit :: Form Manager, defect)
Tracking
()
VERIFIED
FIXED
mozilla56
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)
[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.
Comment 1•7 years ago
|
||
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.
Comment 2•7 years ago
|
||
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]
Comment 3•7 years ago
|
||
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.
Comment 4•7 years ago
|
||
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.
Comment 5•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
(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)
Assignee | ||
Comment 8•7 years ago
|
||
This doesn't meet the definition of critical IMO: "Crashes, loss of data, severe memory leak"
Severity: critical → major
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: selee → MattN+bmo
Assignee | ||
Comment 11•7 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/#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•7 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•7 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.
Assignee | ||
Updated•7 years ago
|
Attachment #8888204 -
Flags: review- → review?(bugs)
Comment 14•7 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.
tracking-firefox56:
--- → +
Assignee | ||
Comment 16•7 years ago
|
||
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)
Assignee | ||
Comment 17•7 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/#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?
Comment 18•7 years ago
|
||
Ah, we shouldn't have removed the mutation observer when !aRemoveMarkings
Comment 19•7 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)
Assignee | ||
Updated•7 years ago
|
Attachment #8888559 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 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•7 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•7 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•7 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•7 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•7 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•7 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
Assignee | ||
Updated•7 years ago
|
Attachment #8888203 -
Flags: review?(dolske)
Comment 31•7 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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Reporter | ||
Comment 32•7 years ago
|
||
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)
Comment 33•7 years ago
|
||
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
Updated•7 years ago
|
Flags: needinfo?(MattN+bmo)
You need to log in
before you can comment on or make changes to this bug.
Description
•