Closed Bug 1358792 Opened 7 years ago Closed 7 years ago

24.99ms uninterruptible reflow at adjustSiteIconStart@chrome://global/content/bindings/autocomplete.xml:2379:13

Categories

(Toolkit :: Autocomplete, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Iteration:
57.2 - Aug 29
Performance Impact medium
Tracking Status
firefox57 --- fixed

People

(Reporter: rowbot, Assigned: Paolo)

References

Details

(Whiteboard: [ohnoreflow][reserve-photon-performance][fxsearch])

Attachments

(1 file)

Here's the stack:

adjustSiteIconStart@chrome://global/content/bindings/autocomplete.xml:2379:13
_onChanged@chrome://global/content/bindings/autocomplete.xml:2069:31
_appendCurrentResult/<@chrome://global/content/bindings/autocomplete.xml:1398:17
setTimeout handler*_appendCurrentResult@chrome://global/content/bindings/autocomplete.xml:1397:15
_appendCurrentResult/this._appendResultTimeout<@chrome://global/content/bindings/autocomplete.xml:1411:58
setTimeout handler*_appendCurrentResult@chrome://global/content/bindings/autocomplete.xml:1411:41
_appendCurrentResult/this._appendResultTimeout<@chrome://global/content/bindings/autocomplete.xml:1411:58
setTimeout handler*_appendCurrentResult@chrome://global/content/bindings/autocomplete.xml:1411:41
_invalidate@chrome://global/content/bindings/autocomplete.xml:1193:11
invalidate@chrome://global/content/bindings/autocomplete.xml:1162:11
notifyResults@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:2111:5
finishSearch@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:2284:5
startSearch/<@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:2241:33
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
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
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
TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
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
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

This occurred while I was typing an address in the address bar.
The code is:
            // Allow margin-inline-start not to be specified in CSS initially.
            let style = window.getComputedStyle(this._typeIcon);
http://searchfox.org/mozilla-central/rev/6e1c138a06a80f2bb3167d9dac050cc0fd4fb39e/toolkit/content/widgets/autocomplete.xml#2376

Marco, do you know in which case the margin-inline-start value isn't in the CSS?
Flags: needinfo?(mak77)
Component: Untriaged → Autocomplete
Product: Firefox → Toolkit
Flags: qe-verify?
Priority: -- → P2
Few lines above we remove when a non numeric value is passed in, that happens for example when there are more than 2 buttons in the toolbar before the urlbar.
http://searchfox.org/mozilla-central/rev/f225dbcb15ca2e38f7d434a9278a41d2340e7cf3/browser/base/content/urlbarBindings.xml#1613

Also, looks like we never set it initially, the last time I was confused by margin-inline-start, that is set on every platform. We seem to always calculate this dynamically the first time the awesomebar is opened. We could maybe set an initial value based on the default, and override it if the toolbar is customized?

Drew wrote this code and may have better ideas, plus I'm on PTO for a couple days.
Flags: needinfo?(mak77) → needinfo?(adw)
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf][photon-performance][fxsearch]
(In reply to Marco Bonardo [::mak] from comment #2)
> Also, looks like we never set it initially [...] We seem to always
> calculate this dynamically the first time the awesomebar is opened.

That's right, the initial value is calculated dynamically so it's guaranteed to be right.  If there's a way to set it statically in CSS, that should be OK too as long as it's right of course.
Flags: needinfo?(adw)
Whiteboard: [ohnoreflow][qf][photon-performance][fxsearch] → [ohnoreflow][qf:p1][photon-performance][fxsearch]
Flags: qe-verify? → qe-verify-
Priority: P2 → P3
Whiteboard: [ohnoreflow][qf:p1][photon-performance][fxsearch] → [ohnoreflow][qf:p1][reserve-photon-performance][fxsearch]
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance][fxsearch] → [ohnoreflow][qf:p2][reserve-photon-performance][fxsearch]
Priority: P3 → P2
Whiteboard: [ohnoreflow][qf:p2][reserve-photon-performance][fxsearch] → [ohnoreflow][qf:p2][photon-performance][fxsearch]
Priority: P2 → P3
Whiteboard: [ohnoreflow][qf:p2][photon-performance][fxsearch] → [ohnoreflow][qf:p2][reserve-photon-performance][fxsearch]
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: P3 → P1
I tested this with and without the site icons alignment. Tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3133408469a347f6465b4cd3b1aadfa5164a4275
I tested this on RTL and found out that the icon would change position slightly if we didn't apply rounding.
Comment on attachment 8899432 [details]
Bug 1358792 - Fix uninterruptible reflow at adjustSiteIconStart.

https://reviewboard.mozilla.org/r/170714/#review176262

Good idea and nice seeing the test agree with it!
Attachment #8899432 - Flags: review?(mak77) → review+
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8633b8546a94
Fix uninterruptible reflow at adjustSiteIconStart. r=mak
https://hg.mozilla.org/mozilla-central/rev/8633b8546a94
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Iteration: --- → 57.2 - Aug 29
Performance Impact: --- → P2
Whiteboard: [ohnoreflow][qf:p2][reserve-photon-performance][fxsearch] → [ohnoreflow][reserve-photon-performance][fxsearch]
You need to log in before you can comment on or make changes to this bug.