On Tue, 9 Jan 2024 21:45:33 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 As I was afraid of, this change causes many test failures (see below for just one example). I think a different approach is needed, one that preserves the synchronous nature of these methods. NestedEventLoopTest > testCanEnterAndExitNestedEventLoop FAILED java.lang.AssertionError at org.junit.Assert.fail(Assert.java:87) at org.junit.Assert.assertTrue(Assert.java:42) at org.junit.Assert.assertFalse(Assert.java:65) at org.junit.Assert.assertFalse(Assert.java:75) at test.javafx.stage.NestedEventLoopTest.lambda$testCanEnterAndExitNestedEventLoop$2(NestedEventLoopTest.java:120) at test.util.Util.lambda$submit$0(Util.java:108) at javafx.graphics@23-ea/com.sun.javafx.application.PlatformImpl.lambda$runLater$10(PlatformImpl.java:456) at java.base/java.security.AccessController.doPrivileged(AccessController.java:400) at javafx.graphics@23-ea/com.sun.javafx.application.PlatformImpl.lambda$runLater$11(PlatformImpl.java:455) at javafx.graphics@23-ea/com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95) at javafx.graphics@23-ea/com.sun.glass.ui.mac.MacApplication._enterNestedEventLoopImpl(Native Method) at javafx.graphics@23-ea/com.sun.glass.ui.mac.MacApplication._enterNestedEventLoop(MacApplication.java:169) at javafx.graphics@23-ea/com.sun.glass.ui.Application.enterNestedEventLoop(Application.java:512) at javafx.graphics@23-ea/com.sun.glass.ui.EventLoop.enter(EventLoop.java:107) at javafx.graphics@23-ea/com.sun.javafx.tk.quantum.QuantumToolkit.enterNestedEventLoop(QuantumToolkit.java:656) at javafx.graphics@23-ea/javafx.application.Platform.enterNestedEventLoop(Platform.java:310) at test.javafx.stage.NestedEventLoopTest.lambda$testCanEnterAndExitNestedEventLoop$0(NestedEventLoopTest.java:112) @Maran23 I see from the description that you tested this on mac. How did you test it? Did you run with `-PFULL_TEST=true`? ------------- Changes requested by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1324#pullrequestreview-1896186878 PR Comment: https://git.openjdk.org/jfx/pull/1324#issuecomment-1959717323