Firefox 63 hangs using a huge form history

VERIFIED FIXED in Firefox 64

Status

()

defect
VERIFIED FIXED
6 months ago
5 months ago

People

(Reporter: jan.fader, Assigned: bgrins)

Tracking

(4 keywords)

63 Branch
mozilla65
Unspecified
All
Points:
---

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63+ wontfix, firefox64+ verified, firefox65+ verified)

Details

Attachments

(4 attachments)

(Reporter)

Description

6 months ago
Posted file 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)
(Reporter)

Comment 1

6 months ago

Updated

6 months ago
Component: Untriaged → Inspector
Product: Firefox → DevTools

Updated

6 months ago
Component: Inspector → Untriaged
Product: DevTools → Firefox

Updated

6 months ago
Component: Untriaged → Autocomplete
Product: Firefox → Toolkit

Updated

6 months ago
See Also: → 1504249

Comment 2

6 months ago
[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?
Blocks: 1479125
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true
Flags: needinfo?(bgrinstead)

Updated

6 months ago
OS: Unspecified → All
(Assignee)

Comment 3

6 months ago
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.

Updated

6 months ago
Keywords: perf
(Assignee)

Comment 4

6 months ago
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.
(Assignee)

Updated

6 months ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 5

6 months ago
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
(Assignee)

Comment 6

6 months ago
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.
(Assignee)

Comment 7

6 months ago
Thanks for filing, this patch fixes the issue for me locally.
(Assignee)

Comment 9

6 months ago
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

6 months ago
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?
(Assignee)

Comment 11

6 months ago
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.
For the sake of regression triage, can we get a prioritization on this, please?
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 13

6 months ago
(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.
Flags: needinfo?(bgrinstead)

Comment 14

6 months ago
Pushed by bgrinstead@mozilla.com:
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

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71321c967598
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: qe-verify+
Please request beta uplift approval when you get a chance.

Comment 17

5 months ago
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.
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 18

5 months ago
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
Flags: needinfo?(bgrinstead)
Attachment #9022255 - Flags: approval-mozilla-beta?
(Assignee)

Comment 19

5 months ago
I suspect this'll need a rebase for beta
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 20

5 months ago
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
Flags: needinfo?(bgrinstead)
Comment on attachment 9025475 [details] [diff] [review]
richlistitemchildren-for-beta-uplift.patch

perf fix for autocomplete, verified in nightly, approved for 64.0b11
Attachment #9025475 - Flags: approval-mozilla-beta+
Attachment #9022255 - Flags: approval-mozilla-beta?

Comment 23

5 months ago
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.
Wontfix for 63 as we are unlikely to have another dot release before 64 ships.

Comment 25

5 months ago
Removing qe+ flag and closing issue as verified - fixed based on Comment 24.
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

5 months ago
Duplicate of this bug: 1506070
You need to log in before you can comment on or make changes to this bug.