RFR: 8335630: Crash if Platform::exit called with fullScreen Stage on macOS 14

[Michael Strauß]

On Mon, 15 Jul 2024 22:08:12 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> This PR solves two related bugs that are caused by events being delivered while the FX runtime is in the process of shutting down. These bugs are related enough that I think they need to be addressed at the same time. While debugging and developing the test, I saw one or the other or both in various test runs. The system test included with this program verifies that there is no crash and no exception, so will catch either failure mode.
>> 
>> This can happen when there is a full-screen window showing during shutdown on macOS, since full screen enter / exit uses a nested event loop that affects the order of operation. This could theoretically happen in other cases, thus the fix does not involve checking whether we are in full-screen or whether there is a nested event loop.
>> 
>> The fix does the following:
>> 
>> 1. `GlassWindow+Overrides.m`: Check that the JVM is still running before calling the Java notification method in `windowDidResignKey` and `windowShouldClose`. This matches what is already done for other similar methods in this class. This is the fix for JDK-8335630.
>> 2. `Window.java` and `View.java`: Check whether the Application instance is null in the event notification callbacks, and skip handling the event if it is. This is the fix for JDK-8299738.
>> 
>> Note that the fix for 2 is in platform-independent code. This should be fine, since we would want to skip the event handling callback on any other platform if it were done during shutdown, in order to avoid the ISE.
>> 
>> I have included a system test that passes on all platforms. On macOS, the test will fail without the fix and pass with the fix.
>> 
>> Details of the fix are below:
>> 
>> `Platform::exit` is called by the application to shutdown the JavaFX toolkit. The implementation of `Platform::exit` calls `Toolkit::exit` on the JavaFX Application thread, which in turn calls glass `Application::terminate` to shut it down. `Application::terminate` will close all the native windows, and call `finishTerminating` to terminate the native event loop and detach the thread from the JVM. This is asynchronous, at least on macOS, and will schedule the termination by posting a native event to the event loop.
>> 
>> on macOS 13, and sometimes on macOS 14, a Window or View event handler can be called between `Toolkit::exit`/`Application::terminate` (which sets the Toolkit's cached thread to null and the glass Application instance to null) and the JVM shutdown. This will make a JNI upcall to the appropriate Window...
>
> modules/javafx.graphics/src/main/java/com/sun/glass/ui/View.java line 533:
> 
>> 531:     private boolean shouldHandleEvent() {
>> 532:         // Don't send any more events if the application has shutdown
>> 533:         if (Application.GetApplication() == null) {
> 
> Should we declare `Application.application` field as volatile?
> 
> I see interleaved multi-threaded access pattern:
> 
> 
> thread=JavaFX-Launcher
> CREATE thread=JavaFX-Launcher
> thread=JavaFX-Launcher
> thread=Thread-2
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX-Launcher
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX-Launcher
> thread=JavaFX Application Thread
> thread=PulseTimer-CVDisplayLink thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=QuantumRenderer-0
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=PulseTimer-CVDisplayLink thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX-Launcher
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=JavaFX Application Thread
> thread=Java...

Other threads may still not see the current value of the `Application.application` field. Is it safe for other threads to use the `Application` instance after the field has been set to `null`?

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1506#discussion_r1678465264