Closed Bug 1356763 Opened 7 years ago Closed 7 years ago

4.8ms uninterruptible reflow at ensureElementIsVisible@chrome://global/content/bindings/richlistbox.xml:208:30

Categories

(Firefox :: Address Bar, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 55
Iteration:
55.5 - May 15
Performance Impact high
Tracking Status
firefox55 --- fixed

People

(Reporter: rjward0, Assigned: tnguyen)

References

Details

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

Attachments

(1 file, 2 obsolete files)

Here's the stack:

ensureElementIsVisible@chrome://global/content/bindings/richlistbox.xml:208:30
set_selectedIndex@chrome://global/content/bindings/autocomplete.xml:1098:13
onResultsAdded@chrome://browser/content/urlbarBindings.xml:1817:15
_appendCurrentResult@chrome://global/content/bindings/autocomplete.xml:1406:13
_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
_addMatch@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:1733:5
_onResultRow@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:1636:5
handleResult@resource://gre/modules/Sqlite.jsm:774:13
This looks a lot like a duplicate of bug 1353708. Were you using the latest nightly when you saw this? Was there a scrollbar in the location bar panel?
I can repro with latest nightly, but not consistently. I got these stacks using 20170415, the one I submitted previously was with the 20170414 nightly. 

I'm not exactly sure what specific behavior triggers it yet, but it does seem to be related to a scrollbar in the location bar 

I'll try to find more exact STR if I can.  


ensureElementIsVisible@chrome://global/content/bindings/richlistbox.xml:208:30
set_selectedIndex@chrome://global/content/bindings/autocomplete.xml:1098:13
notifyResults@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:2111:5
_addMatch@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:1733:5
_addSearchEngineMatch@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:1499:5
_matchCurrentSearchEngine@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:1464: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@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:2234:5
onInput@chrome://browser/content/urlbarBindings.xml:1116:17
onxblinput@chrome://global/content/bindings/autocomplete.xml:649:9


	

ensureElementIsVisible@chrome://global/content/bindings/richlistbox.xml:208:30
set_selectedIndex@chrome://global/content/bindings/autocomplete.xml:1098:13
notifyResults@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:2111:5
_addMatch@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:1733:5
_addSearchEngineMatch@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:1499:5
_matchCurrentSearchEngine@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:1464: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
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:455:5
startSearch@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:2234:5
onInput@chrome://browser/content/urlbarBindings.xml:1116:17
onxblinput@chrome://global/content/bindings/autocomplete.xml:649:9
(In reply to Robert Ward from comment #2)

> I'm not exactly sure what specific behavior triggers it yet, but it does
> seem to be related to a scrollbar in the location bar 

That would make sense, in bug 1353708 we fixed the sync reflow specifically for the common case where there's no scrollbar.
Component: Untriaged → Location Bar
See Also: → 1353708
Two more traces related to this, the second one is the same one I got with yesterday's nightly. 


ensureElementIsVisible@chrome://global/content/bindings/richlistbox.xml:208:30
set_selectedIndex@chrome://global/content/bindings/autocomplete.xml:1098:13
_onSearchBegin@chrome://browser/content/urlbarBindings.xml:1856:11
onSearchBegin@chrome://global/content/bindings/autocomplete.xml:1111:13
onSearchBegin@chrome://global/content/bindings/autocomplete.xml:244:13
onInput@chrome://browser/content/urlbarBindings.xml:1116:17
onxblinput@chrome://global/content/bindings/autocomplete.xml:649:9

ensureElementIsVisible@chrome://global/content/bindings/richlistbox.xml:208:30
set_selectedIndex@chrome://global/content/bindings/autocomplete.xml:1098:13
onResultsAdded@chrome://browser/content/urlbarBindings.xml:1817:15
_appendCurrentResult@chrome://global/content/bindings/autocomplete.xml:1406:13
_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
_addMatch@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:1733:5
_onResultRow@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:1636:5
_matchKnownUrl/<@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/UnifiedComplete.js:1336:7
handleResult@resource://gre/modules/Sqlite.jsm:774:13


There's a reflow in the native code (I tried attaching a debugger but I'm not sure how to proceed, it hit the break point as expected but opened a disassembly I don't know how to read) that seems possibly related? See here: http://i.imgur.com/2yUrGDi.gif 

Looks to be associated with the scroll bar, which it places inside the box and expands as it populates but removes at the end (is that behavior intended?)
(In reply to Robert Ward from comment #4)

> See here: http://i.imgur.com/2yUrGDi.gif 
> 
> Looks to be associated with the scroll bar, which it places inside the box
> and expands as it populates but removes at the end (is that behavior
> intended?)

Marco, do you happen to know what causes this flickering scrollbar on the right side of the awesomebar panel (I can reproduce on Mac too), and if we have a bug on file to get rid of it?
Flags: needinfo?(mak77)
Flags: qe-verify?
Priority: -- → P2
(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> Marco, do you happen to know what causes this flickering scrollbar on the
> right side of the awesomebar panel (I can reproduce on Mac too), and if we
> have a bug on file to get rid of it?

I know that the scrollbar may appear briefly and then disappear when the panel grows, it's something we don't visually want. It may be due to the delayed AdjustHeight calls, that exists to avoid flicker, looks like it may be imperfect. Just to visually remove the scrollbar it may be enough to set overflow:hidden in our case (maxRows == maxResults)...

Btw, my change in bug 1353708 should avoid this ensureElementIsVisible call, unless the maxRichResults pref has been modified.
maxRows should only depend on the maxrows attribute, while maxResults should only depend on the pref, so I don't know how we'd bypass that if condition.
Someone should check what's up there and why we pass the condition, unless Robert has modified the maxRichResults pref.
Flags: needinfo?(mak77)
PS: The condition I put there is supposed to only consider the final case, when all the results are fetched, the fact in the middle we could show a scrollbar or not shouldn't matter, since maxRows and maxResults are just fixed values (that for the awesomebar should both be set to 10).
To avoid the scrollbar flicker, I think we may want urlbarbindings to forcibly set maxRows = maxResults AND set overflow: hidden.
But first we need to know why we hit the ensureElementIsVisible path.
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]
Another stack trace:

ensureElementIsVisible@chrome://global/content/bindings/richlistbox.xml:213:30
set_selectedIndex@chrome://global/content/bindings/autocomplete.xml:1098:13
handleEnter@chrome://browser/content/urlbarBindings.xml:1155:22
handleKeyPress@chrome://global/content/bindings/autocomplete.xml:538:24
onKeyPress@chrome://browser/content/urlbarBindings.xml:220:18
onxblkeypress@chrome://global/content/bindings/autocomplete.xml:652:8
MozReview-Commit-ID: 8SelC4HZl0m
Attachment #8866622 - Flags: review?(mak77)
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Priority: P2 → P1
Attachment #8866622 - Flags: review?(mak77)
Attachment #8866622 - Attachment is obsolete: true
MozReview-Commit-ID: 8SelC4HZl0m
Comment on attachment 8866638 [details] [diff] [review]
Avoid ensureElementIsVisible in no input case and correct reset maxRows

Hi Marco, could you please take a look?
Attachment #8866638 - Flags: review?(mak77)
Comment on attachment 8866638 [details] [diff] [review]
Avoid ensureElementIsVisible in no input case and correct reset maxRows

Review of attachment 8866638 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing for now, waiting for answers.

::: toolkit/content/widgets/autocomplete.xml
@@ +1042,5 @@
>          }, 0);
>  
> +        // Reset the maxRows property to the cached "normal" value (if there's
> +        // any), and reset normalMaxRows so that we can detect whether it was set
> +        // by the inputinput when the popupshowing handler runs.

typo: "inputinput"

@@ +1091,5 @@
>            // invoke it only if there may be a scrollbar, so if we could fetch
>            // more results than we can show at once.
>            // maxResults is the maximum number of fetched results, maxRows is the
>            // maximum number of rows we show at once, without a scrollbar.
> +          if (this.mInput && this.mInput.maxRows &&

why is this necessary? the maxRow getter already checks mInput, and yes, it could return defaultMaxRows, but that's in rare cases andnot for the location bar.
Attachment #8866638 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #14)
> Comment on attachment 8866638 [details] [diff] [review]
> Avoid ensureElementIsVisible in no input case and correct reset maxRows
> 

> @@ +1091,5 @@
> >            // invoke it only if there may be a scrollbar, so if we could fetch
> >            // more results than we can show at once.
> >            // maxResults is the maximum number of fetched results, maxRows is the
> >            // maximum number of rows we show at once, without a scrollbar.
> > +          if (this.mInput && this.mInput.maxRows &&
> 
> why is this necessary? the maxRow getter already checks mInput, and yes, it
> could return defaultMaxRows, but that's in rare cases andnot for the
> location bar.

Hmm, you are right, I just thought that we should care about the case null mInput (in that case maxResults > maxRows (== 6))

I just took a round into the code and I see mInput is only available in _openAutocompletePopup
https://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/browser/base/content/urlbarBindings.xml#1560

That comes from native
https://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1689

But we may do some others may trigger set_selectedIndex, for example 
https://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1680
( in stack in comment 4).
Perhaps we could override the default max row for the case location bar, or only set selected index after _openAutocompletePopup. I prefer the second way
I think we have two cases could go, hope that I don't miss anything:
- set selected index too early (_onSearchBegin, startSearch)
- wrong reset _normalMaxRows
Attachment #8866638 - Attachment is obsolete: true
Attachment #8867065 - Flags: review?(mak77)
Comment on attachment 8867065 [details] [diff] [review]
Avoid ensureElementIsVisible if there's no popup open  and correct reset maxRows

Review of attachment 8867065 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thank you for the investigation.
Attachment #8867065 - Flags: review?(mak77) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95f2e76cdc8b
Avoid ensureElementIsVisible if there's no popup open and correct reset maxRows. r=mak
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/95f2e76cdc8b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.5 - May 15
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
I think it's been long enough since the fix landed that QA isn't useful on this specific bug anymore; we would have had bugs filed from nightly users if we had regressed something significant here. Also, bug 1264988 is related, so maybe we can do QA in that bug instead once it's fixed.
Flags: qe-verify+ → qe-verify-
QA Contact: adrian.florinescu
Performance Impact: --- → P1
Whiteboard: [ohnoreflow][qf:p1][photon-performance] → [ohnoreflow][photon-performance]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: