Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8319669: [macos14] Running any JavaFX app prints Secure coding warning #1280

Conversation

kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 8, 2023

Fix JDK-8319669 as follows:

  1. Override the NSApplicationDelegate method applicationSupportsSecureRestorableState in GlassApplication and return YES. This silences the warning that FX applications now get on macOS 14.
  2. Create and initialize an NSApplicationFX subclass of NSApplication with no additional functionality. This stops AWT from overwriting the NSApplicationDelegate (GlassApplication) that JavaFX sets during toolkit initialization in the case where the AWT toolkit is used from a JavaFX Application (e.g., when using SwingNode or other Swing functionality), and is necessary in order to safely do the above. It also fixes other problems that can result from the delegate being overwritten.

As noted in the bug report, this PR solves the following problems:

  • Eliminates the "Secure coding is not enabled for restorable state" warning on macOS 14

  • The assertion error reported in JDK-8318129

  • The FX application stops getting messages when the application is hidden, deactivated, reactivated, etc. We currently don't do anything with these messages once the application is running, but we might do so in the future.

  • Probably related to the above, we sometimes get an odd behavior when trying to hide an application on macOS 13 using the CMD-H key after the AWT Toolkit has been initialized. Instead of hiding the window, it pops up a finder window with a folder icon and a label that shows the version of Java.

  • If AWT and FX return a different answer from their delegate's applicationSupportsSecureRestorableState method, it will crash on macOS 13.x.

This is the FX equivalent of JDK-8318854 in AWT.


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-8319669: [macos14] Running any JavaFX app prints Secure coding warning (Bug - P2)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1280

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

Using diff file

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

Webrev

Link to Webrev Comment

@kevinrushforth
Copy link
Member Author

I will do some additional testing and then take this PR out of Draft mode, making it "rfr".

/reviewers 2

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 8, 2023

👋 Welcome back kcr! 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
Copy link

openjdk bot commented Nov 8, 2023

@kevinrushforth
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 kevinrushforth marked this pull request as ready for review November 8, 2023 18:35
@openjdk openjdk bot added the rfr Ready for review label Nov 8, 2023
@kevinrushforth
Copy link
Member Author

Reviewers: @prrace @honkar-jdk @azuev-java

@mlbridge
Copy link

mlbridge bot commented Nov 8, 2023

Webrevs

@kevinrushforth
Copy link
Member Author

Notes to reviewers:

  • I pushed a commit with debug logging, and then reverted it. If you want to see the debug logging messages, update to the commit just prior to the HEAD commit.
  • I recommend to test this on macOS 14 with the latest JDK 22 EA promoted build. Once @honkar-jdk sends the PR for JDK-8318854, it would be helpful to test with that as well.

@kevinrushforth
Copy link
Member Author

I recommend to test this on macOS 14 with the latest JDK 22 EA promoted build. Once @honkar-jdk sends the PR for JDK-8318854, it would be helpful to test with that as well.

PR openjdk/jdk#16569 has been created. It's still in Draft, but should be "rfr" soon.

@kevinrushforth
Copy link
Member Author

I completed my testing of this patch in combination with the patch from PR jdk#16569. I tested all four cases:

(patched / unpatched JDK) X (patched / unpatched FX)

See this PR comment for details.

@honkar-jdk
Copy link

Tested by applying JavaFX changes + corresponding JDK patch. Testing looks good on older macOS version and macOS 14.1 GA.

All combinations of patched/unpatched JDK and patched/unpatched FX along with different combinations of env vars was tested.

Copy link

@honkar-jdk honkar-jdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix looks good and works well with the corresponding JDK patch

Copy link
Member

@azuev-java azuev-java left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it on both x86 and arm based macOS 14 works fine.

@openjdk
Copy link

openjdk bot commented Nov 14, 2023

@kevinrushforth This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8319669: [macos14] Running any JavaFX app prints Secure coding warning

Reviewed-by: honkar, prr, kizune

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the master branch:

  • d24e96a: 8318984: Update to Xcode 14.3.1 on macOS
  • 8dd3c37: 8318841: macOS: Memory leak with MenuItem when Menu.useSystemMenuBar(true) is used
  • c8641e2: 8284445: macOS 12 prints a warning when a function key shortcut is assigned to a menu

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Nov 14, 2023
@kevinrushforth
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Nov 15, 2023

Going to push as commit 986ec4f.
Since your change was applied there have been 5 commits pushed to the master branch:

  • 2d4b494: 8319762: Update to Visual Studio 2022 version 17.6.5 on Windows
  • c8b44be: 8274967: KeyCharacterCombinations for punctuation and symbols fail on non-US keyboards
  • d24e96a: 8318984: Update to Xcode 14.3.1 on macOS
  • 8dd3c37: 8318841: macOS: Memory leak with MenuItem when Menu.useSystemMenuBar(true) is used
  • c8641e2: 8284445: macOS 12 prints a warning when a function key shortcut is assigned to a menu

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 15, 2023
@openjdk openjdk bot closed this Nov 15, 2023
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Nov 15, 2023
@openjdk
Copy link

openjdk bot commented Nov 15, 2023

@kevinrushforth Pushed as commit 986ec4f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@kevinrushforth kevinrushforth deleted the 8319669-macos-secure-warning branch November 15, 2023 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
4 participants