4.84 MB, application/x-sqlite3
245 bytes, text/html
Bug 1503342 - Rename richlistbox.children to richlistbox.itemChildren so .children will refer to the normal DOM API;r=paolo
47 bytes, text/x-phabricator-request
|Details | Review|
36.98 KB, patch
|Details | Diff | Splinter Review|
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)
Component: Untriaged → Autocomplete
Product: Firefox → Toolkit
[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?
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true
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.
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: nobody → bgrinstead
Status: NEW → ASSIGNED
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
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.
Thanks for filing, this patch fixes the issue for me locally.
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
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?
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?
(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.
Pushed by email@example.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
Please request beta uplift approval when you get a chance.
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 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
I suspect this'll need a rebase for beta
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 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+
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.
Removing qe+ flag and closing issue as verified - fixed based on Comment 24.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.