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

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Location Bar
P1
normal
RESOLVED FIXED
4 months ago
a month ago

People

(Reporter: Robert Ward, Assigned: tnguyen)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 months ago
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?
(Reporter)

Comment 2

4 months ago
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: → bug 1353708
(Reporter)

Comment 4

4 months ago
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)

Updated

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

Updated

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

Updated

3 months ago
Blocks: 1363757
No longer blocks: 1348289
(Assignee)

Comment 10

3 months ago
Created attachment 8866622 [details] [diff] [review]
Only reset maxRows if there's cached normal value

MozReview-Commit-ID: 8SelC4HZl0m
(Assignee)

Updated

3 months ago
Attachment #8866622 - Flags: review?(mak77)
Comment hidden (obsolete)

Updated

3 months ago
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Priority: P2 → P1
(Assignee)

Updated

3 months ago
Attachment #8866622 - Flags: review?(mak77)
(Assignee)

Updated

3 months ago
Attachment #8866622 - Attachment is obsolete: true
(Assignee)

Comment 12

3 months ago
Created attachment 8866638 [details] [diff] [review]
Avoid ensureElementIsVisible in no input case and correct reset maxRows

MozReview-Commit-ID: 8SelC4HZl0m
(Assignee)

Comment 13

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

Comment 15

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

Comment 16

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

Updated

3 months ago
Attachment #8866638 - Attachment is obsolete: true
(Assignee)

Comment 17

3 months ago
Created attachment 8867065 [details] [diff] [review]
Avoid ensureElementIsVisible if there's no popup open  and correct reset maxRows

MozReview-Commit-ID: 8SelC4HZl0m
(Assignee)

Updated

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

Comment 19

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&tochange=c868e0a0fd54dce64df0df06bf714479ac7bdd72
Keywords: checkin-needed

Comment 20

3 months ago
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
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

3 months ago
Iteration: --- → 55.5 - May 15

Updated

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

Updated

a month ago
QA Contact: adrian.florinescu
You need to log in before you can comment on or make changes to this bug.