2038980 - Set fullscreen collection behavior at window init to prevent NSViewUpdateVibrancyForSubtree crash

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[vlopezgarcia]

After investigating bug 2031249, wanted to create a bug here for traceability as the change will be done on mozilla central.

We have seen crashes on startup on the CI on Firefox Enterprise with the following exception message: Crashing on exception: *** Collection <__NSArrayM: 0x13d787820> was mutated while being enumerated. , after investigating we saw the reason is internally appkit (at least on 15.3) can call a function _implicitlyAllowsFullScreenPrimary which might change its value in the middle of display, this then can trigger a mutation of views in the middle of enumeration creating a crash.

We want to set the following flags at window creation before any display code is called to ensure that function is not called and avoid reaching that scenario.

NSWindowCollectionBehaviorFullScreenPrimary | NSWindowCollectionBehaviorFullScreenAllowsTiling
NSWindowCollectionBehaviorFullScreenAuxiliary | NSWindowCollectionBehaviorFullScreenDisallowsTiling

And we want to make sure also that the function SetSupportsNativeFullscreen also sets the same flags.

This change is nonbackwards-compatible, if a user of the code was relying on the behaviour of _implicitlyAllowsFullScreenPrimary instead of setting always macnativefullscreen="true". That means we need to make sure all .xhtml that are full screen should behave correctly and they have the flag set.

Comment 1

[vlopezgarcia]

Attachment: Screenshot.png to illustrate what would actually "break"

Comment 2

[vlopezgarcia]

I put an image to illustrate for instance with pageInfo.xhtml how with the proposed change the zoom button would become + if macnativefullscreen is not set.

Comment 3

[vlopezgarcia]

Any ideas on how we should proceed? I just have the feeling SetSupportsNativeFullscreen is not really working and we have most of the times NSWindowCollectionBehaviorFullScreenPrimary ( the zoom button with 2 arrows) just set by appkit internally

Comment 4

[Gian-Carlo Pascutto [:gcp]]

If I understand correctly, this patch would mean that any window we don't explicit set macnativefullscreen on would now show the + icon and not maximize fully (over the dock etc), similar to how our old pre-native fullscreen mode used to work. Because there is built-in code in macOS that guesses when a window might want to do native full-screen, we could regress some windows/dialogs that used to be native fullscreen capable (but not marked by us as such) that won't be any more, but the green button UX will correctly reflect that.

We need this to work around a crash due to a bug in the macOS internal machine, because we end up changing the state of these windows after creation due to how our window construction/property setting works. Firefox Enterprise is especially prone to triggering this bug.

Comment 5

[Stephen A Pohl [:spohl]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #4)

We need this to work around a crash due to a bug in the macOS internal machine, because we end up changing the state of these windows after creation due to how our window construction/property setting works. Firefox Enterprise is especially prone to triggering this bug.

Hi Gian-Carlo, I'm wrapping my mind around this (and related) bugs, but do you have an easy way for me to reproduce the crash that you are referring to?

Keeping n-i set.

Comment 6

[:gerard-majax]

(In reply to Stephen A Pohl [:spohl] from comment #5)

(In reply to Gian-Carlo Pascutto [:gcp] from comment #4)

We need this to work around a crash due to a bug in the macOS internal machine, because we end up changing the state of these windows after creation due to how our window construction/property setting works. Firefox Enterprise is especially prone to triggering this bug.

Hi Gian-Carlo, I'm wrapping my mind around this (and related) bugs, but do you have an easy way for me to reproduce the crash that you are referring to?

Unfortunately, I think the only reliable way is hammering macOS CI on enterprise-main

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Bug 2038980: Set explicit fullscreen collection behavior at window creation to avoid AppKit's non-deterministic heuristic on macOS 15.3. r?#mac-reviewers!

Default resizable titled top-level windows to FullScreenPrimary +
FullScreenAllowsTiling so the green window-control button keeps its
fullscreen-enter arrows; non-resizable titled windows default to
FullScreenAuxiliary + FullScreenDisallowsTiling. SetSupportsNativeFullscreen
sets all four bits consistently so AppKit never falls back to the
non-deterministic _implicitlyAllowsFullScreenPrimary heuristic that
caused the bug 2031249 crash.

Comment 8

[Stephen A Pohl [:spohl]]

I took a stab at a patch that tries to avoid the crash without the UX regression noted in comment 2. The idea is to set an explicit NSWindowCollectionBehaviorFullScreen* value at window creation, defaulting resizable titled top-level windows to Primary | AllowsTiling so the green window-control button keeps its fullscreen-enter arrows.

I may have missed cases, though. Could you check whether this actually avoids the CI crash, and let me know if there are any undesirable consequences I haven't thought through?

Comment 9

[vlopezgarcia]

Hi Stephen, regarding the two topics:

First one, I was not able to reproduce this locally, I tried some reproducers... but without any luck.

Regarding the patch, the backwards-incompatible behavior comes from SetSupportsNativeFullScreen,
SetSupportsNativeFullScreen is called always, with true if the attribute macnativefullscreen is there (does not matter which value), otherwise it is called with false.

Only 2 .xhtml use this attribute, in your patch things like: devtools in window mode, more site info, the enterprise login screen(which we can set the macnativefullscreen of course), will show the + button.

I did 500 runs, we had 6 tests failures (unrelated) and 0 crashes.
https://firefox-ci-tc.services.mozilla.com/tasks/groups/D5xQcsafQtSvD0wFSIEvHQ#marionette

Comment 10

[Stephen A Pohl [:spohl]]

Thank you for testing the patch. Is a 500-run try push without crashes enough confidence that the crash is indeed fixed with this patch?

You're right that calling SetSupportsNativeFullscreen(false) for every window without macnativefullscreen would regress UX. I've amended the patch with a small tweak to AppWindow.cpp so we only call SetSupportsNativeFullscreen when the attribute is actually present, and we read its value as a boolean (so macnativefullscreen="false" correctly turns native fullscreen off, instead of just HasAttribute checking presence). When the attribute is absent, the creation-time default in nsCocoaWindow::CreateNativeWindow wins, which means resizable titled top-level windows keep their fullscreen-enter arrows without needing per-window .xhtml edits.

The crash protection shouldn't be affected by this. The collectionBehavior is still always explicit before the first display happens, so AppKit should never consults _implicitlyAllowsFullScreenPrimary. The new code path only changes the post-XUL-parse behavior for the "attribute absent" case (from "force off" to "leave at creation default").

Would you mind giving this updated patch another try when you have a chance? It would be great to check that it still avoids the crash and that the UX matches what we'd expect.

Comment 11

[vlopezgarcia]

Thank you for testing the patch. Is a 500-run try push without crashes enough confidence that the crash is indeed fixed with this patch?
Yes and no, usually I was doing runs of 50/100, and that would give me 1-2 crashes. For verifying then I was running 500.
If we plan to land this I would run 5k or 10k on the weekend unless you have other ideas, in the other bug I had this function _implicitlyAllowsFullScreenPrimary swizzled to check it was not called also with the fix.

The patch from UX perspective looks good now, still will have another look if I find something unexpected.

Comment 12

[Stephen A Pohl [:spohl]]

Attachment: Bug 2038980: Set explicit fullscreen collection behavior at window creation to avoid AppKit's non-deterministic heuristic on macOS 15.3. r?#mac-reviewers!

Default resizable titled top-level windows to FullScreenPrimary +
FullScreenAllowsTiling so the green window-control button keeps its
fullscreen-enter arrows; non-resizable titled windows default to
FullScreenAuxiliary + FullScreenDisallowsTiling. SetSupportsNativeFullscreen
sets all four bits consistently so AppKit never falls back to the
non-deterministic _implicitlyAllowsFullScreenPrimary heuristic that
caused the bug 2031249 crash.

In AppWindow.cpp, only call SetSupportsNativeFullscreen when the XUL
macnativefullscreen attribute is actually present, and read its value as
a boolean. With HasAttribute as the previous gate, windows without the
attribute were forced to FullScreenAuxiliary, which would regress the
zoom button to '+' on every XHTML window that doesn't carry the
attribute (only two do today). Leaving them at the creation-time default
preserves the existing fullscreen UX without requiring per-window XHTML
edits.

Comment 13

[Stephen A Pohl [:spohl]]

I'm glad to hear it! I'll go ahead and request review of the patch for now, but do let me know if anything else comes up.

Comment 14

[vlopezgarcia]

I ran 500 more with 0 crashes (some tests failures unrelated to these changes). I will run the test on the weekend
https://firefox-ci-tc.services.mozilla.com/tasks/groups/A2yWcDwVTmap5PQQ-VGW5w#marion

Comment 15

[agoloman]

Backed out for causing mochitest failures @test_bug522217.xhtml.

• Backout link
• Push with failures
• Failure Log

Comment 16

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/3cd1ce2d0177

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:spohl, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox152 to wontfix.

For more information, please visit BugBot documentation.

Comment 18

[Stephen A Pohl [:spohl]]

Redirecting n-i. Is this urgent enough to uplift?

Comment 19

[vlopezgarcia]

There were no reports on regular Firefox about this crash, and currently we have our own release process for Firefox Enterprise to pick this change.

Based on the crash reports (none for Firefox) this change does not need to be uplifted.

Comment 20

[Stephen A Pohl [:spohl]]

Thanks for confirming!

Comment 21

[vlopezgarcia]

Run 2.5k wth 0 crashes. looks good.

https://firefox-ci-tc.services.mozilla.com/tasks/groups/JGW0uDETSpCqwRk5HjJ60Q#marionette