On Mon, 20 Nov 2023 23:51:09 GMT, Kevin Rushforth <[email protected]> wrote:
>> @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. >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](https://github.com/openjdk/jdk/blob/9a6ca233c7e91ffa2ce9451568b3be88ccd04504/src/java.desktop/macosx/native/libawt_lwawt/awt/LWCToolkit.m#L355) 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? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16569#discussion_r1408823737
