1883132 - Firefox janks doing some a11y work when I hover the maximize/restore button (as part of the Windows "snap layouts" popup)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Daniel Holbert [:dholbert]]

STR:

1. Visit https://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp
2. Start profiling.
3. Hover the maximize/restore button in the Firefox window's toolbar.
4. Wait another 10 seconds or so, and then capture the profile.

ACTUAL RESULTS:
Here's the profile I get: https://share.firefox.dev/3P2jFyJ -- it shows the following:

• When I hovered the maximize/restore button, the searchfox.org process janked for 4.5 seconds (which corresponds to a profile marker for DocAccessible::DoInitialUpdate)
• Right near the end of that 4.5sec jank period, the parent process starts a ~1.1 second jank period (actually two consecutive janks), where we're spending all our time in mozilla::a11y::PDocAccessibleParent::OnMessageReceived

EXPECTED RESULTS:
Less jank, given that the initiating action (my mouse crossing over a button) has no obvious user-facing reason for any stalling to happen.

(Thanks to Jan in bug 1866066 comment 15 for identifying this method of activating a11y, which was new to me at least, though maybe familiar to our a11y team. This bug here is a partial explanation for bug 1866066, but it doesn't explain/account-for the occasional never-ending hangs noted there, which is why I'm spinning this off separately.)

Comment 1

[Daniel Holbert [:dholbert]]

I just (I think) reproduced something like bug 1866066, by performing the STR here, except now opening 10 nsCSSFrameConstructor.cpp tabs (rather than just one) in a single Firefox window; and then starting logging/profiling using the "Windows" logging-preset at about:logging; and then hovering the maximize/restore button.

That Firefox Nightly 125.0a1 (2024-03-01) session has now been hanging for several minutes, apparently unresponsive.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: screenshot of profiler for a Firefox session with 10 searchfox tabs that went off the rails and janked for 5 minutes when I hovered the maximize/restore tab

(In reply to Daniel Holbert [:dholbert] from comment #1)

I just (I think) reproduced something like bug 1866066, by performing the STR here, except now opening 10 nsCSSFrameConstructor.cpp tabs [...]
That Firefox Nightly 125.0a1 (2024-03-01) session has now been hanging for several minutes, apparently unresponsive.

This Firefox session eventually stopped hanging, after 5 minutes. I did manage to capture it in the profile, but unfortunately
(a) the profiler discarded or missed the first half of that time (only showing me samples/tracks for the final 2.5min of the 5min period)
(b) the profiler wouldn't let me upload (or even save/export-JSON) for the profile, because it was too large -- possibly because I had logging enabled, and that substantially bloated the amount of profiler data.

I did capture a screenshot of the profile (apologies this screenshot isn't itself accessible, but this is the best I was able to do to capture the unforutnately-un-exportable profile).
In this screenshot, I'm hovering what seems to be the most relevant thing for that final 2.5min which is a 2min 39sec long (but really maybe 5min long, with the first half truncated?) profile marker in the ResizeObserver track, for "Accessibility Notifications (Display)", with the marker's stack (for the AddRefreshObserver call) going up to mozilla::a11y::FocusManager::NotifyOfDOMBlur, several stack-layers inside from nsFocusManager::WindowLowered)

Comment 3

[Daniel Holbert [:dholbert]]

I've been trying to trigger another 5minute hang that I might actually be able to capture in the profile, but so far I've been unable to. Comment 1 was me "getting lucky" to some extent I suppose.

Here's the closest I've come: https://share.firefox.dev/3wGglDe

In this session, I had 10 searchfox tabs open, and then I started profiling, and then I hovered the maximize/restore button, and then I tried to interact with my foreground tab (hovering & mousewheel scrolling) in case that mattered. And then I changed tabs a few times, by clicking tab-titles. Firefox was reasonably responsive during most of this process, but one of the tab-switches caused things to lock up for ~8 seconds, during which time I saw a tab-spinner animation (from t=26s to t=34s in the profile).

That tab-spinner happens to correspond to a period where all of my searchfox processes had jank for 5-8 seconds, and this also seems to have been the start of a ~45second period of continuous activity (though not jank) in the parent process, which has a smattering of PDocAccessibleParent in the callstack.

Comment 4

[Daniel Holbert [:dholbert]]

Here's another profile I just captured, very similar to the one in comment 3:
https://share.firefox.dev/430d8u8

In this case the Firefox window was in the background, and I was just repeatedly hovering/unhovering the maximize/restore button.

Looking at this profile and the one in comment 3, I'm noticing that the sorta-delayed 8-second jank seems to be triggered by a FontListChanged notification, which forces us to restyle/reframe every documet. That seems to reliably always happen shortly (within 15 seconds) after I first trigger accessibility to be instantiated (by hovering the maximize button). (Not sure why that connection is there; maybe it's because a11y forces us to finish loading some font data that we otherwise handle in the background lazily?)

Anyway, that FontListChanged all-content-processes-at-once restyle/reframe is responsible for part of the jank here, but I think that only happens once per session (and maybe/hopefully in the background in most cases, within the first few minutes?). So I don't want to focus on that particular bit of jank here too much.

Comment 5

[James Teh [:Jamie]]

I vaguely recalled Chromium hit this problem with Snap Layouts as well. I don't know how they mitigated it, but I just found this bug:
https://issues.chromium.org/issues/327558597

In April 2023, we learned that the maximize button in Windows 11 was using an accessibility API. We then learned directly from the Windows team that they were doing this to get the bouding box of the maximize button, to know where to position the snap window layout view in the screen. However, they confirmed they only used the accessibility API as a last resort, and that there's a way we (chromium) could expose the right information to the OS that would make them take a different path than the fallback solution relying on UIA.

The solution is to "respond to the WM_GETTITLEBARINFOEX on your frame window and populate the TITLEBARINFO struct with the RECT of your maximize button.". This sounds like a fairly scoped change that should improve performance quite a bit, since it wouldn't enable the kNativeAPIs accessibility mode.

Comment 6

[James Teh [:Jamie]]

WM_GETTITLEBARINFOEX
TITLEBARINFOEX
The thing I'm not sure about is how to get the bounds of those controls from widget code, since they're XUL elements in browser code. Seems like we might have to do something fairly hacky using element ids or something to find those elements so we can get their screen bounds.

Comment 7

[James Teh [:Jamie]]

Ah. It looks like we already have these rects in nsWindow::mWindowBtnRect. That should make this easier at least.

Comment 8

[James Teh [:Jamie]]

Attachment: Bug 1883132 WIP: Return rects for custom minimize, maximize and close buttons in response to WM_GETTITLEBARINFOEX.

This prevents the Windows Snap Layouts feature from falling back to accessibility APIs when trying to determine the screen locations of these buttons.
We don't want to instantiate the entire accessibility engine just because a user tried to adjust their browser window.

Comment 9

[James Teh [:Jamie]]

Being totally blind realistically makes it infeasible for me to test this. :) Nevertheless, I've attached a patch which might fix this (or at least serve as a starting point for someone else). In particular, I'm not entirely sure that I entirely understand the coordinate spaces here.

Comment 10

[Emilio Cobos Álvarez [:emilio]]

FWIW I tried the patch and I don't even see a WM_GETTITLEBARINFOEX message sent to us...

Comment 11

[James Teh [:Jamie]]

Do you experience this instantiation of a11y, though? I wonder if you need to do something else in order for this Snap Layouts feature to trigger? I honestly don't even know what it does really.

Comment 12

[Emilio Cobos Álvarez [:emilio]]

I do. What I see in the profiler is:

• A WM_NCHITTEST message, which returns the maximize button (HTMAXBUTTON).
• A bunch of WM_GETOBJECT calls instantly, one of those causing a11y to get enabled.

Comment 13

[James Teh [:Jamie]]

Terrific. So either Windows just isn't sending WM_GETTITLEBARINFOEX or I've missed somewhere else in the widget code I need to handle this.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1883132 - Respond to WM_GETTITLEBARINFOEX. r=#win-reviewers

Based on D203391, just for reference since I wrote a bit more code to
fill all relevant buttons.

Comment 15

[James Teh [:Jamie]]

I guess if worse comes to the worst, we can do another suppress hack similar to the clipboard stuff which uses WM_NCHITTEST as the trigger. Yuck, but possible.

Comment 16

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1883132 - Suppress a11y instantiation on Win11 for the snap layouts feature. r=Jamie

This is not ideal, because they fall back to position the popup under
the cursor, but it's probably better.

The right thing to do would be for Windows to use the TITLEBARINFOEX
message. We should probably still land that code just so they can
eventually use it, seems worth doing anyways.

Comment 17

[James Teh [:Jamie]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #16)

The right thing to do would be for Windows to use the TITLEBARINFOEX
message.

I wonder if it's worth pinging Microsoft about this? We might learn there's something else we're missing or something.

Comment 18

[James Teh [:Jamie]]

Interestingly, this Microsoft article mentions WM_NCHITTEST, but not WM_TITLEBARINFOEX.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f249edb2247b
Suppress a11y instantiation on Win11 for the snap layouts feature. r=Jamie

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5cfb74b3a23
Respond to WM_GETTITLEBARINFOEX. r=win-reviewers,rkraesig

Comment 21

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/f249edb2247b
https://hg.mozilla.org/mozilla-central/rev/c5cfb74b3a23