1358443 - 9.59ms uninterruptible reflow at adjustHeight@chrome://global/content/bindings/autocomplete.xml:1239:32

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, enhancement, P1)

Description

[Brian Carpenter [:geeknik]]

Here's the stack:

adjustHeight@chrome://global/content/bindings/autocomplete.xml:1239:32
_invalidate/this._adjustHeightTimeout<@chrome://global/content/bindings/autocomplete.xml:1184:58
setTimeout handler*_invalidate@chrome://global/content/bindings/autocomplete.xml:1184:41
invalidate@chrome://global/content/bindings/autocomplete.xml:1162:11
showPopupWithResults@resource://gre/modules/AutoCompletePopup.jsm:179:7
receiveMessage@resource://gre/modules/AutoCompletePopup.jsm:245:9

Comment 1

[Florian Quèze [:florian]]

This must be due to one of the 4 getBoundingClientRect calls in this method: http://searchfox.org/mozilla-central/rev/7aa21f3b531ddee90a353215bd86e97d6974e25b/toolkit/content/widgets/autocomplete.xml#1226

Comment 2

[Brian Carpenter [:geeknik]]

Another stack: 

adjustHeight@chrome://global/content/bindings/autocomplete.xml:1239:32
_invalidate/this._adjustHeightTimeout<@chrome://global/content/bindings/autocomplete.xml:1184:58
setTimeout handler*_invalidate@chrome://global/content/bindings/autocomplete.xml:1184:41
invalidate@chrome://global/content/bindings/autocomplete.xml:1162:11
notifyResults@resource://gre/components/UnifiedComplete.js:2114:5
_addMatch@resource://gre/components/UnifiedComplete.js:1736:5
_addSearchEngineMatch@resource://gre/components/UnifiedComplete.js:1502:5
_matchCurrentSearchEngine@resource://gre/components/UnifiedComplete.js:1467:5
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
TaskImpl_run@resource://gre/modules/Task.jsm:324:15
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
Task_spawn@resource://gre/modules/Task.jsm:166:12
TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:389:16
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:455:5
startSearch@resource://gre/components/UnifiedComplete.js:2237:5
onInput@chrome://browser/content/urlbarBindings.xml:1125:17
onxblinput@chrome://global/content/bindings/autocomplete.xml:649:9

Comment 3

[Mike Conley (:mconley) (:⚙️) (PTO: May 24 - June 2nd)]

Now that bug 1434376 is fixed, I think someone can probably take another crack at this. I'm queueing for the Firefox Performance team.

Comment 4

[Mike Conley (:mconley) (:⚙️) (PTO: May 24 - June 2nd)]

Hey Gijs, do you have a sense of whether or not it's worth spot-fixing this while the AwesomeBar dropdown refactor is still ~6 months out from MVP?

Comment 5

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from comment #4)
> Hey Gijs, do you have a sense of whether or not it's worth spot-fixing this
> while the AwesomeBar dropdown refactor is still ~6 months out from MVP?

I don't remember off-hand. I'll look at this again tomorrow or Friday. IIRC the (horizontal row) overflow reflows are more common/frequent than this one so I dunno how significant the impact would be, but I guess it won't hurt to take another look to be sure.

Comment 6

[Marco Bonardo [:mak]]

yes, the overflow flushes are more common, but also harder to get rid of.

Comment 7

[:Gijs (he/him)]

After some discussion with Marco, I think I have something that works, which I'll attach, though I'll be curious if other people notice anything that behaves worse post-patch than pre-patch in terms of flickering. I don't see much difference either way, which I somewhat expected because I basically expect the cost of the reflow to now shift to the calls to onOverflow().

Comment 8

[:Gijs (he/him)]

Attachment: Bug 1358443 - remove reflows from adjustHeight itself, r?mak,florian

This goes back to relying on rows being the same height.
Places where we replace the popup will likely not use the richlistbox,
and we no longer have code that changes the height or exceeds the maximum
number of visible children with a scrollbar, so we should be OK.

To determine the padding on the richlistbox and the height of the initial
row, I've used a promiseDocumentFlushed callback. It's possible this causes
flicker the first time the popup opens. I can't see any, but it's quite
possible I'm missing something.

Comment 9

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2796f6f012d64ee3655f6cb3bfee93db84e5580

Comment 10

[:Gijs (he/him)]

This seems to fail a number of autocomplete (non-browser) chrome mochitests, because window.promiseDocumentFlushed() is not available. I'll look at this tomorrow - there are other test-failures, too, but off-hand they seem like they should be easier to address.

Comment 11

[:Gijs (he/him)]

Per discussion with mconley and mak on IRC, I'm adjusting only the URL bar here. I'll file a follow-up bug to consider if we can make improvements to the general in-content autocomplete even though formautofill is "special", and though window.documentPromiseFlushed() is not available on non-chrome documents (so the autocomplete mochitest-chrome tests don't like it...).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62ba1b74793bd00cd046385fcd7c02a157a69bfc

Comment 12

[:Gijs (he/him)]

Removed animate support from toolkit per mak's comment. New trypush:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9997fbfdde9732345dcb950aeb3fc29bcc90e18

Comment 13

[Phabricator Automation]

Comment on attachment 8993403 [details]
Bug 1358443 - remove reflows from adjustHeight itself, r?mak,florian

Marco Bonardo [::mak] has approved the revision.

https://phabricator.services.mozilla.com/D2242

Comment 14

[Phabricator Automation]

Comment on attachment 8993403 [details]
Bug 1358443 - remove reflows from adjustHeight itself, r?mak,florian

Florian Quèze [:florian] has approved the revision.

https://phabricator.services.mozilla.com/D2242

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ce819c0a081
remove reflows from adjustHeight itself, r=mak,florian

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c0ccf6a9bfc
Backed out changeset 0ce819c0a081 for browser chromes failures on browser_urlbar_search_no_speculative_connect_with_client_cert.

Comment 17

[Cosmin Sabou [:CosminS]]

Backed out for browser chromes failures on browser_urlbar_search_no_speculative_connect_with_client_cert

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=0ce819c0a08153a533805a0c347f9cd298bf5b9a&tochange=6c0ccf6a9bfc3749de76ee24e3bcf4f4efa2f54f&filter-searchStr=browser%20chrome&selectedJob=190070311

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=190070311&repo=autoland&lineNumber=3062

Backout link: https://hg.mozilla.org/integration/autoland/rev/6c0ccf6a9bfc3749de76ee24e3bcf4f4efa2f54f

09:36:44     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js | The node is there. - true == true - 
09:36:44     INFO - Buffered messages finished
09:36:44     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js | The second item is selected - Got 0, expected 1
09:36:44     INFO - Stack trace:
09:36:44     INFO - chrome://mochikit/content/browser-test.js:test_is:1305
09:36:44     INFO - chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js:popup_mousedown_no_client_cert_dialog_until_navigate_test:165
09:36:44     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1103
09:36:44     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1094
09:36:44     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:996
09:36:44     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
09:36:44     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js | A valid string reason is expected - 
09:36:44     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js | Reason cannot be empty - 
09:36:44     INFO - Console message: [JavaScript Error: "getScreenshot(https://localhost:50879/) failed: TypeError: NetworkError when attempting to fetch resource." {file: "resource://activity-stream/lib/Screenshots.jsm" line: 47}]
09:36:44     INFO - getScreenshotForURL@resource://activity-stream/lib/Screenshots.jsm:47:7
09:36:44     INFO - async*maybeCacheScreenshot@resource://activity-stream/lib/Screenshots.jsm:101:32
09:36:44     INFO - async*_fetchScreenshot@resource://activity-stream/lib/TopSitesFeed.jsm:231:11
09:36:44     INFO - async*_fetchIcon@resource://activity-stream/lib/TopSitesFeed.jsm:219:11
09:36:44     INFO - async*getLinksWithDefaults@resource://activity-stream/lib/TopSitesFeed.jsm:158:11
09:36:44     INFO - async*refresh@resource://activity-stream/lib/TopSitesFeed.jsm:180:25
09:36:44     INFO - async*onAction@resource://activity-stream/lib/TopSitesFeed.jsm:411:9
09:36:44     INFO - _middleware/</<@resource://activity-stream/lib/Store.jsm:51:11
09:36:44     INFO - Store/this[method]@resource://activity-stream/lib/Store.jsm:29:55
09:36:44     INFO - customDispatch/this.placesChangedTimer<@resource://activity-stream/lib/PlacesFeed.jsm:203:11
09:36:44     INFO - 
09:36:44     INFO - Console message: [JavaScript Error: "NetworkError when attempting to fetch resource."]
09:36:44     INFO - get@resource://services-settings/remote-settings.js:309:9
09:36:44     INFO - async*getSite@resource://activity-stream/lib/FaviconFeed.jsm:158:25
09:36:44     INFO - async*fetchIcon@resource://activity-stream/lib/FaviconFeed.jsm:132:24
09:36:44     INFO - async*onAction@resource://activity-stream/lib/FaviconFeed.jsm:182:9
09:36:44     INFO - _middleware/</<@resource://activity-stream/lib/Store.jsm:51:11
09:36:44     INFO - Store/this[method]@resource://activity-stream/lib/Store.jsm:29:55
09:36:44     INFO - _requestRichIcon@resource://activity-stream/lib/TopSitesFeed.jsm:252:5
09:36:44     INFO - _fetchIcon@resource://activity-stream/lib/TopSitesFeed.jsm:216:5
09:36:44     INFO - async*getLinksWithDefaults@resource://activity-stream/lib/TopSitesFeed.jsm:158:11
09:36:44     INFO - async*refresh@resource://activity-stream/lib/TopSitesFeed.jsm:180:25
09:36:44     INFO - async*onAction@resource://activity-stream/lib/TopSitesFeed.jsm:411:9
09:36:44     INFO - _middleware/</<@resource://activity-stream/lib/Store.jsm:51:11
09:36:44     INFO - Store/this[method]@resource://activity-stream/lib/Store.jsm:29:55
09:36:44     INFO - customDispatch/this.placesChangedTimer<@resource://activity-stream/lib/PlacesFeed.jsm:203:11
09:36:44     INFO - 
09:37:28     INFO - Not taking screenshot here: see the one that was previously logged

Comment 18

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbb6e8d694871c1d219dd1612ee706093bcbe0f1

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c334bde84123
remove reflows from adjustHeight itself, r=mak,florian

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c334bde84123