On Mon, 15 Jul 2024 22:08:12 GMT, Andy Goryachev <ango...@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
