Page MenuHomePhabricator

Bug 1554453 - Reset nsAutocompleteController internal state when the login manager marks an focus field. r=sfoster,tgiles
ClosedPublic

Authored by dimi on Nov 16 2021, 2:05 PM.
Referenced Files
Unknown Object (File)
Fri, Mar 20, 3:41 AM
Unknown Object (File)
Thu, Mar 19, 10:24 AM
Unknown Object (File)
Mon, Mar 16, 7:10 AM
Unknown Object (File)
Sun, Mar 15, 3:37 PM
Unknown Object (File)
Sun, Mar 15, 3:36 PM
Unknown Object (File)
Sat, Mar 14, 8:43 PM
Unknown Object (File)
Sun, Mar 8, 5:00 AM
Unknown Object (File)
Sat, Mar 7, 7:43 PM
Subscribers

Details

Summary

In nsAutocompleteController::HandleKeyNavigation, we might trigger a search operation (or open the popup)
However, when we find there is already a search result received previously, and the search string is the same
as the current one, we won't trigger a new search[1] because the result should be the same.
This might be true for most of the cases, but not true when clients that perform the search is differnt.
As far as I know, there are at least 3 different autocomplete clients: form history, CC/Address autocomplete, and login manager.
For example, the same search string might return a different result when it is searched by the login manager
and by the form history component.

Here is how the intermittent occurs:

  1. The page is loaded
  2. Password field is detected so the login manager fetches the login data from the parent asynchronously
  3. The testcase continues to run. The username field is focused
  4. Autocomplete search for the focus field is triggered. Since the login manager has not yet told FormFillController this is a field we care, we search the form history and get an empty result.
  5. Login data is sent from parent to child, _fillForm is called. The LoginManagerChild calls MarkAsLoginManagerField
  6. Key event occurs on the field. Autocomplete search is triggered again. However, the search is not executed because [1]
  7. Popup is not shown. Testcase timeout.

When #5 occurs before #3, the autocomplete search in #4 will be processed by the login manager, and we will get the result
and open the popup in this case

In the current architecture, I don't see an easy way that we can distinguish whether the autocomplete client is different
in nsAutocompleteController. So instead of doing it in nsAutocompleteController, this patch fixes this issue in FormFillController.

In FormFillController, when we call MarkAsLoginManagerField on a focus field, we call ResetInternalState() to
tell nsAutocompleteController to clear previous search result.

[1] https://searchfox.org/mozilla-central/rev/fb8d77331582639ea6848a61dd8ee812fac31b77/toolkit/components/autocomplete/nsAutoCompleteController.cpp#516-521

Event Timeline

dimi planned changes to this revision.Nov 16 2021, 2:05 PM
dimi created this revision.
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".Nov 16 2021, 2:05 PM
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
dimi requested review of this revision.Nov 16 2021, 4:29 PM
dimi updated this revision to Diff 508490.
dimi retitled this revision from WIP: Bug 1554453 - Reset nsAutocompleteController internal state when the login manager marks an focus field. to Bug 1554453 - Reset nsAutocompleteController internal state when the login manager marks an focus field. r=sfoster,tgiles.
dimi edited the summary of this revision. (Show Details)
dimi added reviewers: sfoster, tgiles.
toolkit/components/satchel/nsFormFillController.cpp
326–332

This change is to sync with how we do this in MarkAsLoginManagerField, which I think it is correct because we don't have to call MaybeStartControllingInput again when it is focused (we always control the focus input in FormFillController)

Thank you for figuring this out and the very detailed commit description! This seems like the best solution right now for resolving the intermittent issue

This revision is now accepted and ready to land.Nov 17 2021, 4:42 PM

No sure if you are blocking on me, but I'm happy to defer to tgiles on this one.

dimi added a commit: Restricted Diffusion Commit.Feb 18 2022, 7:09 PM