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

RESOLVED INCOMPLETE

Status

()

defect
P5
normal
RESOLVED INCOMPLETE
a year ago
8 months ago

People

(Reporter: geeknik, Assigned: Gijs)

Tracking

(Blocks 1 bug)

Trunk
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 affected)

Details

(Whiteboard: [ohnoreflow][qf:p1:f64][fxperf:p2])

Reporter

Description

a year ago
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
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf][fxperf]
Whiteboard: [ohnoreflow][qf][fxperf] → [ohnoreflow][qf:p1][qf:f64][fxperf]
Component: Untriaged → Tabbed Browser
This appears to be a (necessary) focus call. Not sure this bug is actionable.
Priority: -- → P5
Assignee

Comment 2

a year ago
(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?
Flags: needinfo?(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.
Flags: needinfo?(mconley)

Updated

a year ago
Whiteboard: [ohnoreflow][qf:p1][qf:f64][fxperf] → [ohnoreflow][qf:p1:f64][fxperf]
Assignee

Updated

11 months ago
Whiteboard: [ohnoreflow][qf:p1:f64][fxperf] → [ohnoreflow][qf:p1:f64][fxperf:p3]
Re-queuing for triage by fxperf to reconcile with qf evaluation.
Whiteboard: [ohnoreflow][qf:p1:f64][fxperf:p3] → [ohnoreflow][qf:p1:f64][fxperf]
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.
Whiteboard: [ohnoreflow][qf:p1:f64][fxperf] → [ohnoreflow][qf:p1:f64][fxperf:p2]
See Also: → 1358687
Assignee

Comment 6

9 months ago
I'll look at this for a bit.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee

Comment 7

9 months ago
Note that bug 1483354 comment 5 suggests making focus async might have unintended consequences.
See Also: → 1483354
Assignee

Comment 8

9 months ago
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?
Flags: needinfo?(enndeakin)
Assignee

Comment 9

9 months ago
(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?
(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
Assignee

Updated

9 months ago
See Also: → 1358813
Assignee

Comment 11

9 months ago
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.
Blocks: 1371371
No longer blocks: photon-performance-triage
Assignee

Comment 12

9 months ago
(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.)
Flags: needinfo?(mconley)

Comment 13

8 months ago
> 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.
Flags: needinfo?(enndeakin)
Flags: needinfo?(mconley)
Assignee

Comment 14

8 months ago
(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.
Assignee

Comment 15

8 months ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.