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

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Hazel Ward]

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

Comment 1

[Florian Quèze [:florian]]

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?

Comment 2

[Hazel Ward]

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

Comment 3

[Florian Quèze [:florian]]

(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.

Comment 4

[Hazel Ward]

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?)

Comment 5

[Florian Quèze [:florian]]

(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?

Comment 6

[Marco Bonardo [:mak]]

(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.

Comment 7

[Marco Bonardo [:mak]]

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).

Comment 8

[Marco Bonardo [:mak]]

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.

Comment 9

[Marco Castelluccio [:marco]]

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

Comment 10

[Thomas Nguyen (:tnguyen)]

Attachment: Only reset maxRows if there's cached normal value

MozReview-Commit-ID: 8SelC4HZl0m

Comment 11

[Thomas Nguyen (:tnguyen)]

(In reply to Marco Bonardo [::mak] from comment #8)
> 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.

Just checked and I would say why we hit ensureElementIsVisible is maxRows would be wrong reseted (unless the maxRichResults pref has been modified). I did not go deeper to se why mInput would be null but I think add a condition check if we cached _normalMaxRows is safe.

Comment 12

[Thomas Nguyen (:tnguyen)]

Attachment: Avoid ensureElementIsVisible in no input case and correct reset maxRows

MozReview-Commit-ID: 8SelC4HZl0m

Comment 13

[Thomas Nguyen (:tnguyen)]

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?

Comment 14

[Marco Bonardo [:mak]]

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.

Comment 15

[Thomas Nguyen (:tnguyen)]

(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

Comment 16

[Thomas Nguyen (:tnguyen)]

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

Comment 17

[Thomas Nguyen (:tnguyen)]

Attachment: Avoid ensureElementIsVisible if there's no popup open and correct reset maxRows

MozReview-Commit-ID: 8SelC4HZl0m

Comment 18

[Marco Bonardo [:mak]]

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.

Comment 19

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&tochange=c868e0a0fd54dce64df0df06bf714479ac7bdd72

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95f2e76cdc8b
Avoid ensureElementIsVisible if there's no popup open and correct reset maxRows. r=mak

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/95f2e76cdc8b

Comment 22

[Florian Quèze [:florian]]

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.