On Mon, 20 Nov 2023 18:41:37 GMT, Harshitha Onkar <[email protected]> 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
