RFR: JDK-8318854: [macos14] Running any AWT app prints Secure coding warning [v2]

[Kevin Rushforth]

On Mon, 20 Nov 2023 18:41:37 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:

>>> 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.
>
> @prrace @kevinrushforth @mrserb 
> 
> The reason has to why we don't observe NPE or crash when sharedDelegate is null is because in Objective C when a method is called on a nil object it is treated as no-op and returns nil instead of NPE, contrary to what we would usually expect. Details [here](https://stackoverflow.com/questions/2696891/calling-a-method-on-an-uninitialized-object-null-pointer).
> 
> For example executing the following line when shared delegate is null and accessing defaultMenuBar on null delegate returns null and not NPE.
> 
> `[[ApplicationDelegate sharedDelegate] defaultMenuBar];`
> 
> Additionally we are not seeing any difference with defaultMenuBar case with unpatched JDK + unpatched JFX (Non-Nil shared delegate) vs patched JDK + patched JFX (Nil shared delegate) at [AWTWindow.m](https://github.com/openjdk/jdk/blob/5493f5f92d569d8e94d1f271480f11c48257e896/src/java.desktop/macosx/native/libawt_lwawt/awt/AWTWindow.m#L844) and [CMenubar.m](https://github.com/openjdk/jdk/blob/5493f5f92d569d8e94d1f271480f11c48257e896/src/java.desktop/macosx/native/libawt_lwawt/awt/CMenuBar.m#L213)  because in both cases defaultMenuBar returned is nil. This might be the case at other places too and are being checked if they worked differently without and with this JDK patch.

So in  either case -- with or without the delegate being overridden -- the defaultMenuBar is returning `nil`. This makes sense, since in the case of a JavaFX app, JavaFX has already initialized the `NSApplication` and set the system menu bar.

I do recommend leaving the `sharedDelegate` null checks that you added as part of this PR, since that way we short-circuit any other logic (e.g., in this specific case you avoid setting `isDisabled = NO` which seems more accurate), and you aren't relying on Objective C to treat this case as a no-op.

You might even consider making the `sharedDelegate` null check (and bailing out) earlier, but maybe there is a good reason to leave the check where it is.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16569#discussion_r1399875074