8371370: [macOS] Crash after main stage with modal dialog is moved to full screen

[jperedadnr]

This PR adds a fix to prevent a crash on macOS after exiting full screen mode when a modal dialog is showing (https://bugs.openjdk.org/browse/JDK-8371370).

At the same time, it prevents non-resizable windows from entering full screen mode (https://bugs.openjdk.org/browse/JDK-8379315), given that the changes for both issues were interrelated.

While no tests have been added, manual tests have been run, checking that the style mask, the window behavior and the standard window buttons state, remained consistent in different scenarios.

Also, according to https://developer.apple.com/documentation/appkit/nswindow/showsresizeindicator, the showsResizeIndicator property has been removed.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8371370: [macOS] Crash after main stage with modal dialog is moved to full screen (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2098/head:pull/2098
$ git checkout pull/2098

Update a local copy of the PR:
$ git checkout pull/2098
$ git pull https://git.openjdk.org/jfx.git pull/2098/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2098

View PR using the GUI difftool:
$ git pr show -t 2098

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2098.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jpereda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[Copilot]

Pull request overview

Fixes macOS Glass window state handling around fullscreen transitions, particularly when a modal dialog disables the main stage, to avoid crashes and prevent non-resizable windows from entering fullscreen.

Changes:

• Defers fullscreen collection behavior enabling to the point when a window becomes resizable, preventing non-resizable ownerless windows from entering fullscreen.
• Tracks/restores miniaturizable/resizable style-mask bits more carefully during enable/disable transitions.
• Reapplies style-mask, collection behavior, and standard button enabled states on fullscreen exit.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
modules/javafx.graphics/src/main/native-glass/mac/GlassWindow.m	Adjusts enabled/disabled handling to preserve/restore miniaturizable/resizable style bits and standard button enabled states.
modules/javafx.graphics/src/main/native-glass/mac/GlassWindow+Overrides.m	Restores style mask, collection behavior, and button states when exiting fullscreen; adds a failure-to-exit handler.
modules/javafx.graphics/src/main/native-glass/mac/GlassWindow+Java.m	Updates resizable toggling to manage fullscreen eligibility via collection behavior and to defer style changes until fullscreen exit completes.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mlbridge]

Webrevs

• 02: Full - Incremental (04f0ef20)
• 01: Full - Incremental (ebcafd13)
• 00: Full (dc680865)

[andy-goryachev-oracle]

/reviewers 2

[openjdk]

@andy-goryachev-oracle
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[kevinrushforth]

This will need a lot of testing in various modes to ensure no regressions or unintended changes in behavior.

I did a fair bit of testing with an preliminary patch last year, but will need to refamiliarize myself with it.

@beldenfox Would you have time to take a look at this as well?

[kevinrushforth]

As a sanity check, I submitted a headful test run to our CI and all existing tests passed.

[beldenfox]

I will take a look at this. At some point I will also run the tests in PR #1789 which cover more ground than the existing system tests.

[beldenfox]

This PR is causing some of the tests in PR #1789 to fail. I think the tests are timing out while trying to put UNDECORATED stages into fullscreen mode.

This is another murky area in the spec. The description of setResizable() states that turning the flag off doesn't prevent API calls from changing the window size. Historically it seems we've interpreted that to mean that non-resizable stages can be maximized and enter fullscreen mode. There's code on the macOS side to handle the fullscreen case (setResizableForFullscreen:). That assumption has made its way into the tests.

The tests in #1789 are FullScreenTest and StageAttributesTest (testFullScreenStage and testFullScreenStageBeforeShow). The timeouts seem to be preventing proper cleanup so I'm also seeing some bogus failures after the first one.

[beldenfox]

This is minor but I've never understood the coding conventions in the Mac glass code. I have no idea why there's all the references to self and why no one uses property notation. In any other codebase this line would just be

nsWindow.collectionBehavior = behavior;

I wouldn't change anything in this PR since it does follow the prevailing conventions but I urge anyone writing new code to at least drop all the self references.

[jperedadnr]

About Undecorated windows:

Currently (before this PR), you can indeed have:

    primaryStage.initStyle(StageStyle.UNDECORATED);
    primaryStage.setFullScreen(true);

and even:

    primaryStage.initStyle(StageStyle.UNDECORATED);
    primaryStage.setResizable(false);
    primaryStage.setFullScreen(true);

and on macOS the window will enter full screen mode, as a regular decorated window, and be resized, even if it is marked as non resizable.

With this PR, that is no longer the case.

The JavaFX definition of Undecorated does mention it supports fullscreen mode, and for non-resizable windows, it mentions that it can be resized programmatically, but it doesn't specify if it can enter fullscreen mode.

However:

• Native macOS applications that are non resizable can't enter fullscreen mode, unless programmatically enabling the NSWindowStyleMaskResizable flag, which is the same as making them resizable.
• Native macOS applications that are undecorated (without the title bar) can't enter fullscreen mode either, without the NSWindowStyleMaskTitled flag. The alternative for this would be keeping the title bar, but hiding the title text and making a transparent title bar, but that will keep the buttons, so that wouldn't match the undecorated definition.

With this PR, I tried to follow those macOS conventions, (so now neither undecorated nor non-resizable windows can't enter full screen on macOS), but I do realize that it implies some inconsistencies for the Undecorated stages.

The question I have: do we want to follow this path (keep macOS conventions) and deal with the required changes for undecorated windows? Or do we keep it as it was, allowing undecorated windows entering full screen mode?

[kevinrushforth]

The question I have: do we want to follow this path (keep macOS conventions) and deal with the required changes for undecorated windows? Or do we keep it as it was, allowing undecorated windows entering full screen mode?

We need to keep current behavior for this. Preventing undecorated windows from programmatically entering full screen would be a regression in behavior.

[jperedadnr]

Right, that makes sense.

I've updated the PR, restoring the behavior for undecorated screens. I've also checked the tests under #1789 and all pass now.

[bridgekeeper]

@jperedadnr This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[jperedadnr]

/keepalive

[openjdk]

@jperedadnr The pull request is being re-evaluated and the inactivity timeout has been reset.

[beldenfox]

I've updated the original bug report with some additional information.

There are two separate systems trying to save and restore the NSWindowStyleMaskResizable flag, one for when the window is disabled and re-enabled and another for entering and existing fullscreen. This is fragile since it only works if things happen in a very specific order. I don't think this bug can be truly fixed until these two save/restore mechanisms are either unified into one mechanism or removed. I vote for removing them and added some notes to the bug report on how I think that would work.

In this bug the OS is providing a round-about way of entering fullscreen mode while the window is disabled. To prevent that this PR is dynamically adding and removing the fullscreen collection behavior. That's a good idea but needs to be followed up with some additional code to bullet-proof any calls to toggleFullScreen. If the collection behavior isn't set when we call toggleFullScreen Glass will enter the fullscreen exiting loop and never leave it. We need to ensure that never happens.

In the past a client has always been able to call Stage.setFullScreen(true) to put a stage into fullscreen mode regardless of StageStyle and even if it has been made non-resizable using Stage.setResizable. To keep that behavior we need to ensure that Stage.setFullScreen always adds the fullscreen collection behavior to the NSWindow the same way it always adds the NSWindowStyleMaskResizable.

Personally I think we should do a localized fix to avoid the crash; when restoring the styleMask avoid altering the NSWindowStyleMaskFullScreen bit. Then we can do a separate PR to remove the two save/restore systems that are fighting over the Resizable style mask bit. And then another PR to add and remove the full screen collection behavior dynamically.