JDK-8318854: [macos14] Running any AWT app prints Secure coding warning #16569
Conversation
|
👋 Welcome back honkar! A progress list of the required criteria for merging this PR into |
|
@honkar-jdk The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
|
@prrace and @kevinrushforth Please review the changes and the PR description. |
Webrevs
|
There was a problem hiding this comment.
The code changes look good. So does my testing.
I tested all four combinations of:
(JDK 22 with & without the fix in this PR) X (JavaFX 22 with & without the fix in PR jfx#1280)
I ran the following tests with each of the above four combinations on macOS 13 and macOS 14:
- Pure Swing app -- AWT toolkit creates NSApplication (FX toolkit not started)
- JFXPanel app -- AWT toolkit creates NSApplication
- Pure JavaFX app --FX toolkit creates NSApplication (AWT toolkit not started)
- SwingNode app -- FX toolkit creates NSApplication
Everything works as expected.
For the macOS 14 specific problems:
- The secure warning is gone for Swing apps (the first two test apps) when running a JDK 22 with this patch, regardless of whether or not FX has the fix
- The secure warning is gone for JavaFX apps (the last two test apps) when running a JavaFX 22 with the patch forjfx#1280, regardless of whether or not the JDK has the fix
- The NSMenu assertion error reported in JDK-8318129 is gone when running either the patched JDK or the patched FX (or both)
| [delegate.fProgressIndicator setHidden:NO]; | ||
| } else { | ||
| [delegate.fProgressIndicator setHidden:YES]; | ||
| if ([ApplicationDelegate sharedDelegate] != nil) { |
There was a problem hiding this comment.
more logical here to check the local variable you just defined.
|
@prrace There are other references to shared delegate within ApplicationDelegate.m in JNI calls such as here #L820. I haven't added null checks here because the references are within JNI calls and when this code is reached a shared delegate is installed because of the following condition -
Do you anticipate any circumstance that can still result in a null shared delegate in this case and recommend that null checks be added to these locations (within JNI calls) too? |
| @@ -841,7 +841,7 @@ - (void) activateWindowMenuBar { | |||
| isDisabled = !awtWindow.isEnabled; | |||
| } | |||
|
|
|||
| if (menuBar == nil) { | |||
| if (menuBar == nil && [ApplicationDelegate sharedDelegate] != nil) { | |||
There was a problem hiding this comment.
Can you please provide details on how the delegate can be null here and there if it should be set at the startup of any awt application?
There was a problem hiding this comment.
It can be null if the NSApplication was created by code outside of AWT. Specifically, in the case of an FX or SWT toolkit, but in theory a native application using JAWT could do this.
In practice, since we haven't even seen a crash as a result of this, I doubt that this code is ever called if AWT isn't running the event loop and hasn't installed their delegate.
There was a problem hiding this comment.
To access any native code in AWT the app should initially init the LWCToolkit which sets that application delegate if it is not set already. Do we know how the FX can bypass that initialization?
There was a problem hiding this comment.
In the case when AWT is embedded AWT does NOT start an NSApplication and does not own it.
AWT overwriting FX's delegate, or any one else's is just plain wrong.
We can turn around what you said
"To access any native code in JavafX, the app should initialise GlassToolkit which sets its delegate, if not already set".
But you can't have both of these.
Meaning AWT isn't special here. It has to play by the same rules.
The toolkit that starts the NSApplication is the one that should be setting the delegate.
If you are embedded in someone else's application, it seems most logical that you defer to their application
and if this means some things aren't possible, so be it.
There was a problem hiding this comment.
Can you please provide details on how the delegate can be null here and there if it should be set at the startup of any awt application?
I commented on these in https://bugs.openjdk.org/browse/JDK-8319255 which is now closed as a dup.
There I wrote (in part)
#######
I am not sure if these are all cases that can provably be in un-reachable code in the "nil" case, which
means some embedded case. But if we ever do reach them, it seems we'd crash the app.
And if we need to make the delegate "nil" in the non-subclassed NSApplication case (ie like FX)
then we are more likely to cause a crash.
[list of places elided]
All these call sites need to be examined to understand that, and if necessary a test developed to
confirm it.
And even then, it might be prudent to add checks for nil.
So the request was to investigate and understand if they were needed, not just add them
@honkar-jdk can comment on what she did there.
There was a problem hiding this comment.
The reason has to why we don't observe NPE or crash when sharedDelegate is null is
But why it is null? The OSXAPP_SetApplicationDelegate is called when the LWCToolkit is initialized in non-headless mode and that code does not depend on "NSApplicationAWT". Does it mean that the toolkit is not initiated? Then how we can call peer-menu-related code? if that code path is not executed probably we should change that?
There was a problem hiding this comment.
I found a reason in one comment above.
There was a problem hiding this comment.
I guess the code ApplicationDelegate.m should be changed. Right now it has some checks to prevent "double initialization", but it should take care about the case if the delegate is set already by some external lib.
Leaving that as null and skip it in the code means that part of the API will stop working, mostly java.awt.desktop package?
There was a problem hiding this comment.
I don't think any further changes are needed to this PR. It's already the case that if another toolkit or the app itself initializes the NSApplication, certain things won't be available in AWT. The system menu bar for example will belong to the toolkit that initializes the NSApplication. That was true before this change and it is still true.
There was a problem hiding this comment.
The question as to whether sharedDelegate can be null is answered as part of #16569 (comment).
I guess the code ApplicationDelegate.m should be changed. Right now it has some checks to prevent "double initialization", but it should take care about the case if the delegate is set already by some external lib.
Leaving that as null and skip it in the code means that part of the API will stop working, mostly java.awt.desktop package?
As Kevin explained it earlier, it would be the responsibility of the toolkit that initialized NSApp to add system menu bar, dock menu (if any). This has been tested using variations of sample programs added to JDK-8318854.
|
@honkar-jdk 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: 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 393 new commits pushed to the
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 |
|
/reviewers 2 reviewers |
| BOOL supportsSecureState = YES; | ||
| if (getenv("AWT_DISABLE_NSDELEGATE_SECURE_SAVE") != NULL) { | ||
| supportsSecureState = NO; | ||
| } |
There was a problem hiding this comment.
I think that (and in the other case), it is best to read the env. var only once, not keep checking it on ever call to this method. Theoretically if someone used putenv/setenv (yes they'd need native code), it would cause us to change the answer.
Yes, this makes the code a bit more complicated but it will be safer.
maybe you could maybe use static locals
(BOOL)applicationSupportsSecureRestorableState:(NSApplication *)app {
static BOOL checked = NO;
static BOOL supportsSecureState = YES;
if (checked == NO) {
checked = YES;
if (getenv("AWT_DISABLE_NSDELEGATE_SECURE_SAVE") != NULL) {
supportsSecureState = NO;
}
return supportsSecureState;
}
There was a problem hiding this comment.
@prrace I think using static variables to read env variables only once is a good idea since I see applicationSupportsSecureRestorableState() being called every time the window loses focus.
|
The latest update has the following changes -
|
|
/integrate |
|
Going to push as commit 940f67c.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@honkar-jdk Pushed as commit 940f67c. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
With Xcode upgraded to 14.3.1 for macOS builds secure coding warning message was seen in the logs as below:
"WARNING: Secure coding is not enabled for restorable state! Enable secure coding by implementing NSApplicationDelegate.applicationSupportsSecureRestorableState: and returning YES."
which requires AppDelegate to explicitly implement applicationSupportsSecureRestorableState() to return true as mentioned here in Apple Release notes.
While investigating JFX embedded scenario (Swing components in FX window) another issue observed was that the AWT was overriding the FX delegate causing the app to crash in certain scenarios. This issue is also being fixed in this PR and also as part of JDK-8319669 , openjdk/jfx#1280.
The fix for JDK-8318854 involves:
implementing applicationSupportsSecureRestorableState() in ApplicationDelegate.m & QueuingApplicationDelegate.m to return YES by default, unless the env var - AWT_DISABLE_NSDELEGATE_SECURE_SAVE is defined.
Fix added to stop AWT toolkit from overriding a delegate set by another NSApplication by default. There is an option to restore the old behavior by defining the env var - AWT_OVERRIDE_NSDELEGATE.
Null checks are added for shared delegate in cases where it can be null and cause issues in AWTWindow.m, CMenuBar.m, ApplicationDelegate.m
Test scenarios involving JDK and JFX fixes is documented here
Updated_Test_Scenarios.xlsx
PLEASE NOTE !! The environment variables being added as part of this fix are for debugging only and should NOT be used for application purpose. As such they will NOT be documented.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16569/head:pull/16569$ git checkout pull/16569Update a local copy of the PR:
$ git checkout pull/16569$ git pull https://git.openjdk.org/jdk.git pull/16569/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16569View PR using the GUI difftool:
$ git pr show -t 16569Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16569.diff
Webrev
Link to Webrev Comment