1483354 - 1.06ms uninterruptible reflow at get_scrollClientRect@chrome://global/content/bindings/scrollbox.xml:112:18

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P5)

Description

[Brian Carpenter [:geeknik]]

Here's the stack:

get_scrollClientRect@chrome://global/content/bindings/scrollbox.xml:112:18
_notifyBackgroundTab@chrome://browser/content/tabbrowser.xml:739:15
_handleNewTab@chrome://browser/content/tabbrowser.xml:859:13
onxbltransitionend@chrome://browser/content/tabbrowser.xml:1024:13

Comment 1

[Hani Yacoub, Desktop QA]

Not sure how to reproduce this issue. I am going to assign the "Firefox: Tabbed Browser" component for it.

Comment 2

[:Gijs (he/him)]

This only happens when opening tabs in the background, which isn't a super common operation for most non-technical users, and it's not super-easy to fix, so marking as fxperf:P2 for now.

STR for clarity:

1. open a page with a link on it.
2. middle-click (or ctrl-click on win/linux, cmd-click on mac) the link to open it in a background (new) tab



At a glance, this seems tricky to fix. However, I think we can probably avoid it in the case where the tabstrip isn't scrolling, and bail out early from _handleNewTab in that case. The other option I'm wondering about is whether, given this code runs off the end of a `transitionend` handler, we could just fetch the size of the tab and scrollbox using dwu / lazy bound getters, instead of forcing a reflow. Finally, of course we could make this async and wait for the next flush before doing anything. Given this code already fires off the end of the transition notification (which is obviously async) naively that seems like it should work...

Dão, any thoughts about bailing out early and/or using promiseDocumentFlushed() here?

Emilio, does the firing of transitionend happen after, before, or independent from the element that's been transitioned having been laid out fully at the end of the transition?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to :Gijs (he/him) from comment #2)
> Emilio, does the firing of transitionend happen after, before, or
> independent from the element that's been transitioned having been laid out
> fully at the end of the transition?

301 Hiro / Birtles, but I'd expect that.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to :Gijs (he/him) from comment #2)

> Emilio, does the firing of transitionend happen after, before, or
> independent from the element that's been transitioned having been laid out
> fully at the end of the transition?

Yes, as far as I can tell all transition events are dispatched before styling, just before fullscreen events (and before rAF callbacks).

https://html.spec.whatwg.org/multipage/webappapis.html#processing-model-8

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Gijs (he/him) from comment #2)
> Dão, any thoughts about bailing out early and/or

Sounds good. I thought we were already doing this, but apparently this is only true for _handleTabSelect.

> using promiseDocumentFlushed() here?

We sometimes have multiple focus calls in a row from different places and the last one is supposed to win, so I'm skeptical about making some focus calls async.

Comment 6

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #5)
> (In reply to :Gijs (he/him) from comment #2)
> > Dão, any thoughts about bailing out early and/or
> 
> Sounds good. I thought we were already doing this, but apparently this is
> only true for _handleTabSelect.
> 
> > using promiseDocumentFlushed() here?
> 
> We sometimes have multiple focus calls in a row from different places and
> the last one is supposed to win, so I'm skeptical about making some focus
> calls async.

Sorry, I'm a bit confused. Did you mean tab selection / scroll-into-view calls? Because I don't see any focus calls in the _handleNewTab / _notifyBackgroundTab code - just calls to scroll the scrollbox if it's scrolling.

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Gijs (he/him) from comment #6)
> (In reply to Dão Gottwald [::dao] from comment #5)
> > (In reply to :Gijs (he/him) from comment #2)
> > > Dão, any thoughts about bailing out early and/or
> > 
> > Sounds good. I thought we were already doing this, but apparently this is
> > only true for _handleTabSelect.
> > 
> > > using promiseDocumentFlushed() here?
> > 
> > We sometimes have multiple focus calls in a row from different places and
> > the last one is supposed to win, so I'm skeptical about making some focus
> > calls async.
> 
> Sorry, I'm a bit confused. Did you mean tab selection / scroll-into-view
> calls? Because I don't see any focus calls in the _handleNewTab /
> _notifyBackgroundTab code - just calls to scroll the scrollbox if it's
> scrolling.

I was probably thinking I was commenting in bug 1462374... But yes, there could potentially be a similar issue with scroll calls...

Comment 8

[:Gijs (he/him)]

Attachment: Bug 1483354 - avoid flushing layout for new background tabs when the tabstrip isn't overflowing, r?dao

Comment 9

[:Gijs (he/him)]

Attachment: Bug 1483354 - use promiseDocumentFlushed to execute tabstrip scrolling after creating a background tab, r?dao

Depends on D13675

Comment 10

[:Gijs (he/him)]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=29b741f715230f1526c3ea86684fa5000822cfec

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4fb8cba13911
avoid flushing layout for new background tabs when the tabstrip isn't overflowing, r=dao
https://hg.mozilla.org/integration/autoland/rev/0cfce8efa53e
use promiseDocumentFlushed to execute tabstrip scrolling after creating a background tab, r=dao

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/4fb8cba13911
https://hg.mozilla.org/mozilla-central/rev/0cfce8efa53e