Closed Bug 1350243 Opened 3 years ago Closed 3 years ago

Dynamic <input list="…"> filtering leads to incorrect autocomplete suggestions in non-remote browsers

Categories

(Toolkit :: Form Manager, defect, P1)

52 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 + wontfix
firefox54 + fixed
firefox55 + fixed

People

(Reporter: MattN, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Keywords: regression, site-compat)

Attachments

(2 files)

Attached file Test case
[Tracking Requested - why for this release]: web-facing regression for dynamic <datalist> with in non-e10s tabs

Last good revision: 7c8216f48c38a8498f251fe044509b930af44de6
First bad revision: 56b3f2c6f53e72698fea6c25130efceef2a26548
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7c8216f48c38a8498f251fe044509b930af44de6&tochange=56b3f2c6f53e72698fea6c25130efceef2a26548

This is most likely from bug 1296638.

While :manotejmeka was implementing bug 1349747 he noticed that handling of dynamic <datalist> wasn't working properly for about:preferences in that incorrect results would sometimes show up if non-trivial JS was running to populate the <datalist>.  The problem didn't occur in an HTML testcase with e10s on but does with e10s off. about:preferences never runs in the content process so shows the issue regardless of the global e10s preference.

As a quick hack I noticed that if I force the e10s code path at [1] by replacing `XRE_IsContentProcess()` with `true` then the problem goes away. I haven't dug back into the commits in bug 1296638 to investigate this yet but I believe there was some unification of e10s vs. non-e10s code paths so the same may be needed for this block.

mconley, any chance you can help Manotej out with this?

[1] https://hg.mozilla.org/mozilla-central/annotate/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/toolkit/components/satchel/nsFormFillController.cpp#l861
Flags: needinfo?(mconley)
Flags: in-testsuite?
Keywords: site-compat
[Tracking Requested - why for this release]: See comment 0.
Tracking 53/54/55+ for the reasons in Comment 0.
The problem here is that the UpdateSearchResultRunnable seems to run with higher priority than messages queued in the message manager.

So we reach zero elements in the <datalist>, and a message is sent to close the popup. The _popupOpen state in the browser-content.js script becomes false.

Then items start being added to the <datalist>, and the UpdateSearchResultRunnable runnables get queued by nsFormFillController::RevalidateDataList.

Then one of those UpdateSearchResultRunnable runs, and tells the AutoCompletePopup.jsm to open the popup. Once the popup is open, AutoCompletePopup.jsm tells the content process that the popup is open, and that it should pay attention to invalidations.

BUT, the UpdateSearchResultRunnable's in the queue will be running in the meantime, and these will think that the popup isn't yet open yet, and so we fail to invalidate.

What I suggest is that we just combine the e10s and non-e10s codepaths here.
Flags: needinfo?(mconley)
Comment on attachment 8855493 [details]
Bug 1350243 - Combine e10s and non-e10s code paths when reacting to datalist updates.

https://reviewboard.mozilla.org/r/127348/#review130430

Thanks Mike!
Attachment #8855493 - Flags: review?(MattN+bmo) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/107ccefe10d2
Combine e10s and non-e10s code paths when reacting to datalist updates. r=MattN
https://hg.mozilla.org/mozilla-central/rev/107ccefe10d2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Hi :MattN,
Since this is a regression, do you think this is worth uplifting to Aurora54?
Flags: needinfo?(MattN+bmo)
Sure, though there isn't a new test for it since I mostly cared about about:preferences and I figured non-e10s was going away…
Assignee: nobody → mconley
Flags: needinfo?(MattN+bmo)
Comment on attachment 8855493 [details]
Bug 1350243 - Combine e10s and non-e10s code paths when reacting to datalist updates.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1296638
[User impact if declined]: Users will see incorrect autocomplete suggestions on sites using <input list="…"> with e10s disabled
[Is this code covered by automated tests?]: datalist/@list has some tests but not for this race condition.
[Has the fix been verified in Nightly?]: It at least works for bug 1349747.
[Needs manual test from QE? If yes, steps to reproduce]: Probably not since we're just removing the non-e10s code path and always using the e10s code path but e10s vs. non-e10s may have other differences worth checking for.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: We're just deleting the non-working non-e10s code path and always taking the same (previously e10s) code path.
[String changes made/needed]: None
Attachment #8855493 - Flags: approval-mozilla-aurora?
Comment on attachment 8855493 [details]
Bug 1350243 - Combine e10s and non-e10s code paths when reacting to datalist updates.

Fix a regression. Aurora54+.
Attachment #8855493 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.