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

RESOLVED FIXED in Firefox 65

Status

()

P5
normal
RESOLVED FIXED
6 months ago
2 months ago

People

(Reporter: geeknik, Assigned: Gijs)

Tracking

Trunk
Firefox 65
Points:
---

Firefox Tracking Flags

(firefox63 wontfix, firefox64 wontfix, firefox65 fixed)

Details

(Whiteboard: [ohnoreflow][fxperf:p2])

Attachments

(2 attachments)

(Reporter)

Description

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

6 months ago
Not sure how to reproduce this issue. I am going to assign the "Firefox: Tabbed Browser" component for it.
Component: Untriaged → Tabbed Browser
Priority: -- → P5
(Assignee)

Comment 2

6 months ago
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?
Flags: needinfo?(emilio)
Flags: needinfo?(dao+bmo)
Whiteboard: [ohnoreflow][fxperf] → [ohnoreflow][fxperf:p2]
(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.
Flags: needinfo?(emilio) → needinfo?(hikezoe)
(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
Flags: needinfo?(hikezoe)
(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.
Flags: needinfo?(dao+bmo)
(Assignee)

Updated

6 months ago
See Also: → bug 1462438
(Assignee)

Updated

6 months ago
See Also: → bug 1462374
(Assignee)

Comment 6

6 months ago
(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.
Flags: needinfo?(dao+bmo)
(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...
Flags: needinfo?(dao+bmo)
(Assignee)

Updated

3 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 8

3 months ago
Created attachment 9029276 [details]
Bug 1483354 - avoid flushing layout for new background tabs when the tabstrip isn't overflowing, r?dao
(Assignee)

Comment 9

3 months ago
Created attachment 9029277 [details]
Bug 1483354 - use promiseDocumentFlushed to execute tabstrip scrolling after creating a background tab, r?dao

Depends on D13675

Comment 11

3 months ago
Pushed by dgottwald@mozilla.com:
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

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4fb8cba13911
https://hg.mozilla.org/mozilla-central/rev/0cfce8efa53e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Depends on: 1513007
status-firefox63: affected → wontfix
status-firefox64: --- → wontfix
You need to log in before you can comment on or make changes to this bug.