Closed Bug 1738980 Opened 1 month ago Closed 1 month ago

Race condition in autocomplete and from autofill

Categories

(Toolkit :: Form Autofill, defect, P2)

defect

Tracking

()

RESOLVED FIXED
96 Branch
Tracking Status
firefox96 --- fixed

People

(Reporter: owlish, Assigned: dimi)

References

Details

Attachments

(1 file)

Was discovered while tracing an intermittent mSearchesOngoing > 0 && mSearches.Contains(aSearch) assertion failure in nsAutoCompleteController.

Failure was not reproducible locally, and only happened in Web Render builds in automation. It reproduced always in the same test, in the same spot, here: https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/AutocompleteTest.kt#1032-1033 . There we first set value and then focus on the field. If those two statements are switched, the bug will be reproduced. My current understanding of it is this:

  • Focus triggers the autocomplete machinery recognize the fields.
  • Calling setValue on the input caused “change” DOM event fire for the input field.
  • On that event, FormAutofillContent actor calls nsFormFillController.showPopup(), if the popup is pending.
  • Since the input is non-zero length, that causes this series of calls: nsFormFillController.showPopup() => nsAutoCompleteController::HandleText => nsAutoCompleteController::StartSearches(). This last one mutates mSearches - it adds the input to that list. It also sets the timer for the search.
  • ( The timer is cleared when closing popup - when selecting something from the popup or when the popup is invalidated by user deleting what she typed or typing something new; and also when nsAutoCompleteController::StopSearch() is called.)
  • When the timer expires, the timer framework calls nsAutoCompleteController::BeforeSearches(), which sets mSearchesOngoing to the length of mSearches - 1 at this point.
  • Meanwhile, the test focuses on the next input element, and the focus event triggers nsAutoCompleteController::ResetInternalState calling StopSearch() and thus clearing mSearches and the timer. If we are lucky, this event happens before the timer is up and before nsAutoCompleteController::onSearchResult is called, if we are not - nsAutoCompleteController::onSearchResult happens with the illegal state.

Adding if (!this.forceStop) here https://searchfox.org/mozilla-central/rev/80fddabd6773cd028ec69dd4f5a2a34fcd6b4387/toolkit/components/formautofill/FormAutofillContent.jsm#277 fixes the intermittent. (See https://treeherder.mozilla.org/jobs?repo=try&revision=e57bb3b6ba36f8b2dc70ada59a869c6ddfebdcb1 )

See Also: → 1733737
Assignee: nobody → dlee
Status: NEW → ASSIGNED

Before this patch, when the search operation is canceled, we return null
in _getRecords instead of returning AutoCompleteResult, See
https://searchfox.org/mozilla-central/rev/80fddabd6773cd028ec69dd4f5a2a34fcd6b4387/toolkit/components/formautofill/FormAutofillContent.jsm#255-271

However, a null result does stop us from calling onSearchResult callback, which triggers
an assertion in nsAutoCompleteController.
https://searchfox.org/mozilla-central/rev/80fddabd6773cd028ec69dd4f5a2a34fcd6b4387/toolkit/components/formautofill/FormAutofillContent.jsm#276-291
https://searchfox.org/mozilla-central/rev/1e7f7235cf822e79cd79ba9e200329ede3d37925/toolkit/components/autocomplete/nsAutoCompleteController.cpp#813

This patches check whether the search operation is canceled before calling onSearchResult.

Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b9985a5aacee
Do not notify autocomplete search result when the search  operation is canceled. r=sfoster,tgiles
Severity: -- → S3
Priority: -- → P2
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a3510e5b766
Do not notify autocomplete search result when the search  operation is canceled. r=sfoster,tgiles
Flags: needinfo?(dlee)
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch
You need to log in before you can comment on or make changes to this bug.