1503342 - Firefox 63 hangs using a huge form history

Status: VERIFIED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Jan Fader]

Attachment: formhistory.sqlite

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.77 Safari/537.36

Steps to reproduce:

Copy the attached formhistory.sqlite into your profile.
Start Firefox and open the testAutocomplete.html
Insert a in the search box and scrolldown a little bit.
Input additional characters or try to delete something in the searchbox.


Actual results:

The input hangs sometimes and sometimes the browser freezes completely.


Expected results:

The inputbox should behave like in Firefox 62.0.3 (no hanging of the inputbox or the browser)

Comment 1

[Jan Fader]

Attachment: testAutocomplete.html

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]: Browser Freeze / Hang with normal use


I can reproduce the short freeze and hang with normal use.
And I found a STR of 100% probability.

Steps To Reproduce:
1. Create New Profile
2. Copy the attached formhistory.sqlite into the profile

3. Start Firefox w/ the profile and open the testAutocomplete.html
4. Type "a" into input field so that suggestion list will pop up
5. Scroll down (by mouse) to the very bottom (until you see "arr..." in the list)
6. Press [backspace] key 
7. Type "a" again
8. Repeat Step 6 and 7


Actual Results:
Browser becomes unresponsive at step 6 or step 7
And finally, the following "Warning: Unresponsive script" dialog pops up (on Nightly65.0a1).

A script on this page may be busy, or it may have stopped responding. You can stop the script now, or you can continue to see if the script will complete.
Script: chrome://global/content/bindings/richlistbox.xml:586

Expected Results:
Not hang


Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=249570f5e1ba0dcddb6b0b562a28a03a38a715ef&tochange=c266a7f4237e42503d320f81e82fb822dadd1002

Regressed by: Bug 1479125

@:bgrins
Your bunch of patch seems to cause the regression, Could you please look into this?

Comment 3

[Brian Grinstead [:bgrins]]

Profile: https://perfht.ml/2zmYkrX. It does appear that "children" getter on richlistbox is extremely slow. The weird thing is the change in https://hg.mozilla.org/integration/autoland/rev/c266a7f4237e#l38.13 just added an additional check (that I would have expected to be faster than the status quo, if nothing else). I'll take a look.

Comment 4

[Brian Grinstead [:bgrins]]

I see what happened here. All of the references to richlistbox.children that used to be richlistbox.childNodes are now running through the custom "children" getter on the XBL binding (https://hg.mozilla.org/integration/autoland/rev/c266a7f4237e#l32.51). We should probably rename that "children" to be something more explicit instead of reusing the DOM method name, and then find callers that actually want that filtered children view to use the new method.

Comment 5

[Brian Grinstead [:bgrins]]

Things using .children before the patch will want to continue using the new "children" method:

https://hg.mozilla.org/integration/autoland/file/0b8b186eb712/toolkit/content/widgets/richlistbox.xml

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: Bug 1503342 - Rename richlistbox.children to richlistbox.itemChildren so .children will refer to the normal DOM API;r=paolo

In Bug 1479125 we put calls to .children that were intended to access child elements into the custom
method, which is a slower path. We may eventually want to remove itemChildren altogether and just assume
that all children are items, but that's out of scope for a perf fix like this.

Comment 7

[Brian Grinstead [:bgrins]]

Thanks for filing, this patch fixes the issue for me locally.

Comment 8

[Brian Grinstead [:bgrins]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9faf2b70e2932e910bc97e2a4b562aac0601623

Comment 9

[Brian Grinstead [:bgrins]]

I'm thinking of also making a more targeted patch for uplift that just switches the few consumers of .children that are causing this perf issue back to .childNodes

Comment 10

[:Paolo Amadini]

I'm not sure it's worth the effort, since this wouldn't solve the fact that the "children" property has changed its meaning, right?

Comment 11

[Brian Grinstead [:bgrins]]

The problem is that some consumers switched from `childNodes` to `children` in Bug 1479125 (which put them on the slower path). We actually do want those to use the "real" `children` API instead of childNodes, but in the meantime we could have a patch that flips those few consumers back to childNodes (only for uplift) and then let the children->itemChildren rename ride the train so it has more time to catch any potential fallout from the change.

Alternatively, we could decide either that we don't need to uplift, or that we think this is low-risk enough to uplift the whole patch.

Comment 12

[David Durst [:ddurst]]

For the sake of regression triage, can we get a prioritization on this, please?

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to David Durst [:ddurst] (Regression Engineering Owner for 63) from comment #12)
> For the sake of regression triage, can we get a prioritization on this,
> please?

Assuming this criteria (https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage) I'd say P1 since we want to fix it in this release. If you are asking specifically about 63, I'm not sure if it's worth an uplift to release since it requires a pretty huge form history to trigger this behavior. We should uplift the fix to beta, though.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71321c967598
Rename richlistbox.children to richlistbox.itemChildren so .children will refer to the normal DOM API;r=paolo

Comment 15

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/71321c967598

Comment 16

[Julien Cristau [:jcristau]]

Please request beta uplift approval when you get a chance.

Comment 17

[Timea Cernea [:tbabos][inactive]]

Reproduced on affected Firefox Release 63.0.1 (64-bit) on Windows 7/10, Mac OS 10.13 and Ubuntu 16.04.

On the latest Nightly 65.0a1 (2018-11-15) (64-bit) the issue can no longer be reproduced by following the steps provided in Comment 2 on all the above mentioned OS. Thus, I will mark this issue as verified on Fx 65.

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 9022255 [details]
Bug 1503342 - Rename richlistbox.children to richlistbox.itemChildren so .children will refer to the normal DOM API;r=paolo

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1479125

User impact if declined: Very slow performance with form autocompletion when working with extremely large form history databases

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: See Comment 0

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): It's restoring behavior for specific callers to `richlistbox.children` to pre-Bug-1479125 state - most of the consumers were in test files, and those that were in chrome code are relatively well covered by mochitests. I did some try pushes (with hg updated before Bug 1479125) to confirm that we weren't missing any callers at https://phabricator.services.mozilla.com/D10751

In the general case: switching from `richlistbox.children` -> `richlistbox.itemChildren` should return the same thing (although it comes through as an Array instead of an HTMLCollection).

String changes made/needed: None

Comment 19

[Brian Grinstead [:bgrins]]

I suspect this'll need a rebase for beta

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: richlistitemchildren-for-beta-uplift.patch

Same as https://hg.mozilla.org/mozilla-central/rev/71321c967598 but without any changes to two places that had landed in 65 via Bug 1501897 / https://hg.mozilla.org/mozilla-central/rev/654dc5ab446c:

- browser/components/preferences/in-content/tests/browser_permissions_dialog.js
- browser/components/preferences/sitePermissions.js

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 9025475 [details] [diff] [review]
richlistitemchildren-for-beta-uplift.patch

perf fix for autocomplete, verified in nightly, approved for 64.0b11

Comment 22

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a867939dde09

Comment 23

[Timea Cernea [:tbabos][inactive]]

Verified - Fixed on Firefox 64.0b11 (Build ID: 20181117155033) on Windows 7/10 x64, Ubuntu 16.04. and Mac OS 10.13.
Marking it as verified for firefox 64.

Comment 24

[Pascal Chevrel:pascalc (PTO until April 26)]

Wontfix for 63 as we are unlikely to have another dot release before 64 ships.

Comment 25

[Timea Cernea [:tbabos][inactive]]

Removing qe+ flag and closing issue as verified - fixed based on Comment 24.