1462374 - 1.05ms uninterruptible reflow at _adjustFocusAfterTabSwitch@chrome://browser/content/tabbrowser.js:1200:5

Status: RESOLVED INCOMPLETE

(Firefox :: Tabbed Browser, defect, P5)

Description

[Brian Carpenter [:geeknik]]

Here's the stack:

_adjustFocusAfterTabSwitch@chrome://browser/content/tabbrowser.js:1200:5
updateDisplay/<@resource:///modules/AsyncTabSwitcher.jsm:425:15
set_selectedIndex@chrome://browser/content/tabbrowser.xml:2329:13
set_selectedPanel@chrome://global/content/bindings/tabbox.xml:641:13
set_selectedIndex@chrome://global/content/bindings/tabbox.xml:386:15
set_selectedItem@chrome://global/content/bindings/tabbox.xml:411:34
set_selectedTab@chrome://global/content/bindings/tabbox.xml:100:15
set selectedTab@chrome://browser/content/tabbrowser.js:250:5
_blurTab@chrome://browser/content/tabbrowser.js:3035:24
_beginRemoveTab@chrome://browser/content/tabbrowser.js:2754:5
removeTab@chrome://browser/content/tabbrowser.js:2678:10
onxblclick@chrome://browser/content/tabbrowser.xml:1062:11

Comment 1

[Dão Gottwald [:dao]]

This appears to be a (necessary) focus call. Not sure this bug is actionable.

Comment 2

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [::dao] from comment #1)
> This appears to be a (necessary) focus call. Not sure this bug is actionable.

Presumably we could execute the focus call in a callback (rAF or promiseLayoutFlushed, or...) where we know layout isn't dirty and the reflow is essentially a no-op, assuming both focus + the selected tab hasn't changed in the meantime? This would make the focus change async though, and I don't know if that's likely to be acceptable. Mike, what do you think, given this is a tab switching issue?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

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

In my mind, the only way this would be unacceptable is if this results in a regression accessibility-wise, or if becomes possible for user events to be misdirected while we wait for the focus to occur. Otherwise, I think it'd be worth exploring.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Re-queuing for triage by fxperf to reconcile with qf evaluation.

Comment 5

[Alex Thayer [:alexical] (she/her)]

It seems like this shouldn't be too hard to do (unless it breaks a large number of tests), and since it looks like it happens fairly frequently I think it makes sense to promote this to p2, assuming there's no accessibility problems.

Comment 6

[:Gijs (out for now; he/him)]

I'll look at this for a bit.

Comment 7

[:Gijs (out for now; he/him)]

Note that bug 1483354 comment 5 suggests making focus async might have unintended consequences.

Comment 8

[:Gijs (out for now; he/him)]

I've just realized that I don't think we actually have good primitives to solve this.

In particular, even if we (*handwaves*) solve the issue with multiple async focus calls, we can't use promiseDocumentFlushed here.

The reason is that the "whole point" of pDF is that (a) we guarantee that the callback to that method gets fired on a clean (flushed) layout state, and (b) because there may be multiple such callbacks per flush, we can NEVER dirty layout state in the callback.

`.focus()` and/or `Services.focus.setFocus()` (apparently?) needs the layout state to be clean in order to not cause 'real' flushes, but will also dirty layout state because it changes the focused element, and the focused element can obviously be restyled based on that layout change.

So I guess the fundamental question here becomes: can we avoid `.focus()` dirtying layout state here? I assume we're flushing layout because we need to assess whether an element is focusable before directing focus to it. But the focusability of the URL bar / find bar / browser element doesn't actually change (much) here. Can't the focus manager just check if the element is focusable and only flush layout when it isn't? Or do we already do that?

Alternatively, how hard would it be to write a focus manager method that takes the same params and behaves like `.setFocus`, but essentially "promises" to focus that element on/after the next layout flush (without doing an additional flush), iff focus in the document does not change further before it gets a chance to do that?

Neil, do you know the answer to these questions?

Comment 9

[:Gijs (out for now; he/him)]

(In reply to :Gijs (he/him) from comment #8)
> So I guess the fundamental question here becomes: can we avoid `.focus()`
> dirtying layout state here?

Err, I meant: can we avoid `.focus()` flushing layout?

> I assume we're flushing layout because we need
> to assess whether an element is focusable before directing focus to it. But
> the focusability of the URL bar / find bar / browser element doesn't
> actually change (much) here. Can't the focus manager just check if the
> element is focusable and only flush layout when it isn't? Or do we already
> do that?

Comment 10

[Dão Gottwald [:dao]]

(In reply to :Gijs (he/him) from comment #9)
> (In reply to :Gijs (he/him) from comment #8)
> > So I guess the fundamental question here becomes: can we avoid `.focus()`
> > dirtying layout state here?
> 
> Err, I meant: can we avoid `.focus()` flushing layout?

You may want to check out: bug 792297, bug 1369140, bug 1366250, bug 1371371

Comment 11

[:Gijs (out for now; he/him)]

So based on comment #0, the exact stack here was hitting this

https://hg.mozilla.org/mozilla-central/annotate/a80de077d740/browser/base/content/tabbrowser.js#l1200

    fm.setFocus(newBrowser, focusFlags);

call, which focuses the browser for the newly selected tab, after closing a tab. I don't seem to be able to reproduce that causing a layout flush, though I did find some STR for bug 1358813 which is essentially the same problem (but with an earlier .select() call triggered via focusAndSelectUrlBar)... so keeping the needinfo for now to try to figure out what's triggering these API calls sometimes still causing flushes.

Comment 12

[:Gijs (out for now; he/him)]

(FWIW, if someone has STR to reproduce this specific reflow, that would be helpful, as I haven't been able to come up with any that worked for me.)

Comment 13

[Neil Deakin]

> Err, I meant: can we avoid `.focus()` flushing layout?

If I remember, removing it causes test failures and compatibility issues.
 
You could always add a flag passed to SetFocus to avoid flushing in nsFocusManager::CheckIfFocusable for this case.

Comment 14

[:Gijs (out for now; he/him)]

(In reply to Neil Deakin from comment #13)
> > Err, I meant: can we avoid `.focus()` flushing layout?
> 
> If I remember, removing it causes test failures and compatibility issues.
>  
> You could always add a flag passed to SetFocus to avoid flushing in
> nsFocusManager::CheckIfFocusable for this case.

Well, it seems like the extant code sometimes flushes and sometimes doesn't. I'm a little confused anyway, because looking in more detail, it seems the focus code should only ever flush frames/styles, and not flush layout. So it's not clear to me how we get to something that ohnoreflow reports from this setfocus code.

Comment 15

[:Gijs (out for now; he/him)]

Per discussion with the fxperf folks, closing this for now while we don't have STR. If we have STR we can re-prioritize. I'll poke at bug 1358813 instead, given I have STR for that.