On Thu, 22 Feb 2024 23:13:26 GMT, Marius Hanl <mh...@openjdk.org> wrote:
>> This PR fixes the dialog freeze problem once and for all. >> >> This one is a bit tricky to understand, here is how it works: >> This bug happens on every platform, although the implementation of nested >> event loops differs on every platform. >> E.g. on Linux we use `gtk_main` and `gtk_main_quit`, on Windows and Mac we >> have an own implementation of a nested event loop (while loop), controlled >> by a boolean flag. >> >> Funny enough, the reason why this bug happens is always the same: Timing. >> >> 1. When we hide a dialog, `_leaveNestedEventLoop` is called. >> 2. This will call native code to get out of the nested event loop, e.g. on >> Windows we try to break out of the while loop with a boolean flag, on Linux >> we call `gtk_main_quit`. >> 3. Now, if we immediately open a new dialog, we enter a new nested event >> loop via `_enterNestedEventLoop`, as a consequence we do not break out of >> the while loop on Windows (the flag is set back again, the while loop is >> still running), and we do not return from `gtk_main` on Linux. >> 4. And this will result in the Java code never returning and calling >> `notifyLeftNestedEventLoop`, which we need to recover the UI. >> >> So it is actually not trivial to fix this problem, and we can not really do >> anything on the Java side. We may can try to wait until one more frame has >> run so that things will hopefully be right, but that sounds rather hacky. >> >> I therefore analyzed, if we even need to return from >> `_enterNestedEventLoop`. Turns out, we don't need to. >> There is a return value which we actually do not use (it does not have any >> meaning to us, other that it is used inside an assert statement). >> Without the need of a return value, we also do not need to care when >> `_enterNestedEventLoop` is returning - instead we cleanup and call >> `notifyLeftNestedEventLoop` in `_leaveNestedEventLoop`, after the native >> code was called. >> >> Lets see if this is the right approach (for all platforms). >> Testing appreciated. >> # >> - [x] Tested on Windows >> - [x] Tested on Linux >> - [x] Tested on Mac >> - [ ] Tested on iOS (although the nested event loop code is the same as for >> Mac) (I would appreciate if someone can do this as I have no access to an >> iOS device) >> - [ ] Adjust copyright >> - [ ] Write Systemtest > > Marius Hanl has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Merge remote-tracking branch 'openjfx/master' into > JDK-8285893-dialog-freezing-🥶 > - JDK-8285893: Decrement nestedEventLoopCounter in leaveNestedEventLoop > - JDK-8285893: Hiding dialog and showing new one causes dialog to be frozen I don't think this is a problem with the nested event loop bookkeeping. It looks like a much simpler bug in the invokeLaterDispatcher. When exitNestedEventLoop is called on the innermost loop the invokeLaterDispatcher suspends operation until the loop finishes. But if you immediately start a new event loop the previous one won't finish and the dispatcher will be wedged. If you're already in a nested loop you can get into the wedged state with just two lines of code: `exitNestedEventLoop(innermost, retVal); enterNestedEventLoop(newLoop);` When the invokeLaterDispatcher is told that the innermost loop is exiting it currently sets `leavingNestedEventLoop` to true. When the dispatcher is told that a new event loop has started it is *not* clearing `leavingNestedEventLoop` but it should. Basically it should follow the same logic used in glass; leaving the innermost loop updates a boolean indicating that the loop should exit but if a new loop is started the boolean is set back to a running state since it now applies to the new loop, not the previous one. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1324#issuecomment-1969844034
