On Sat, 24 Aug 2024 00:26:47 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:

>>> I'm also not sure that this is the right fix. It seems like it will fix one 
>>> specific place where AWT could do an upcall on the AppKit thread after FX 
>>> has detached without addressing the problem as a whole.
>>> 
>>> Another possible fix would be to have AWT not use the cached JNI Env when 
>>> it is not the NSApplication owner. I don't know how viable this fix would 
>>> be, or whether it is the right fix, but it would prevent the crash in all 
>>> possible places.
>> 
>> That sounds like lots of places would need to be updated to do this.
>>> 
>>> > Also : does AWT detach the AppKit thread from the VM on shutdown ?
>>> 
>>> I doubt it, since there isn't AFAIK an explicit toolkit shutdown in AWT 
>>> like there is in FX.
>>> 
>>> > IF not why does FX have to do it ? If it does, then could FX also crash 
>>> > if FX is embedded in an AWT app.
>>> 
>>> FX only detaches the AppKit thread when it is the NSApplication owner, and 
>>> thus attached the thread in the first place. In the case of FX embedded in 
>>> an AWT app, it won't detach the thread.
>> 
>> I didn't expect it to, but I was trying to understand why an AWT exit 
>> doesn't cause FX to crash. System.exit() is the AWT equivalent and AWT 
>> *could* have a shutdown hook to do the same. It just happens not to.
>> 
>>> 
>>> We could explore what it would take for FX to not detach the AppKit thread 
>>> from Java. At a minimum, it would require attaching the AppKit thread as a 
>>> daemon (which AWT already does), and then creating a non-daemon Java thread 
>>> that we would then terminate at FX toolkit shutdown. This would need to be 
>>> tested carefully, but might be the cleanest solution.
>> 
>> I would like to see that explored. What does SWT do ? 
>> 
>>> 
>>> One thing to note is that anything we do here will be about avoiding a 
>>> crash in AWT, not about having the UI remain functional. In this case FX is 
>>> the "owner" of the native Window system event loop (and NSApplication on 
>>> macOS). Applications should not shutdown the FX toolkit (meaning, not call 
>>> `Platform.exit`) and expect AWT to continue to work.
>> 
>> I guess Platform.exit for FX embedded inside AWT is a no-op ?
>
> We probably should replace the usage of getJNIEnv.
> 
> - JNIEnv *env = [ThreadUtilities getJNIEnv];
> + JNIEnv *env = [ThreadUtilities getJNIEnvUncached];
> 
> 
> The getJNIEnv seems unsafe if some 3p libs like FX can detach the thread.

> > Another possible fix would be to have AWT not use the cached JNI Env when 
> > it is not the NSApplication owner. I don't know how viable this fix would 
> > be, or whether it is the right fix, but it would prevent the crash in all 
> > possible places.
> 
> That sounds like lots of places would need to be updated to do this.

No, only in one place. The idea I had -- and did a quick prototype of -- was to 
change the implementation of `[ThreadUtilities getJNIEnv]` to check a flag to 
see whether or not AWT was the owner of the NSApplication and, if not, 
effectively do the same thing as `getJNIEnvUncached`. What that would then do 
would be to re-attach the AppKit thread to the JVM if FX had detached it. This 
will avoid the crash in all places without having to change the callers of 
`getJNIEnv`

 > We could explore what it would take for FX to not detach the AppKit thread 
 > from Java. At a minimum, it would require attaching the AppKit thread as a 
 > daemon (which AWT already does), and then creating a non-daemon Java thread 
 > that we would then terminate at FX toolkit shutdown. This would need to be 
 > tested carefully, but might be the cleanest solution.
> 
> I would like to see that explored. What does SWT do ?

I'll explore this for FX. I think it will be pretty easy. No idea what SWT does.

> > One thing to note is that anything we do here will be about avoiding a 
> > crash in AWT, not about having the UI remain functional. In this case FX is 
> > the "owner" of the native Window system event loop (and NSApplication on 
> > macOS). Applications should not shutdown the FX toolkit (meaning, not call 
> > `Platform.exit`) and expect AWT to continue to work.
> 
> I guess Platform.exit for FX embedded inside AWT is a no-op ?

Not a no-op, since it still shuts down the FX toolkit and renderer, but it 
doesn't detach the AppkitThread from the JVM.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20688#discussion_r1729953756

Reply via email to