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

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P1)

Description

[Matthew N. [:MattN]]

Attachment: 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

Comment 1

[Kohei Yoshino [:kohei]]

[Tracking Requested - why for this release]: See comment 0.

Comment 2

[Marcia Knous [:marcia]]

Tracking 53/54/55+ for the reasons in Comment 0.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1350243 - Combine e10s and non-e10s code paths when reacting to datalist updates.

Review commit: https://reviewboard.mozilla.org/r/127348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127348/

Comment 5

[Matthew N. [:MattN]]

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!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/107ccefe10d2
Combine e10s and non-e10s code paths when reacting to datalist updates. r=MattN

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/107ccefe10d2

Comment 8

[Gerry Chang [:gchang]]

Hi :MattN,
Since this is a regression, do you think this is worth uplifting to Aurora54?

Comment 9

[Matthew N. [:MattN]]

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…

Comment 10

[Matthew N. [:MattN]]

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

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8855493 [details]
Bug 1350243 - Combine e10s and non-e10s code paths when reacting to datalist updates.

Fix a regression. Aurora54+.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/83332293da1e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Per IRC.