-
Notifications
You must be signed in to change notification settings - Fork 437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8285893: Hiding dialog and showing new one causes dialog to be frozen #1324
base: master
Are you sure you want to change the base?
The head ref may contain hidden characters: "JDK-8285893-dialog-freezing-\u{1F976}"
Conversation
👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into |
f3a78ce
to
a5a16a7
Compare
a5a16a7
to
98a97b0
Compare
Webrevs
|
Hmm. There was a reason we added a return value to /reviewers 2 |
@kevinrushforth |
@Maran23 Please merge in the latest upstream master, since your branch is out of date. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
No, I checked the example in the ticket as well as the handling of dialogs in general on Mac. Regarding the system tests, I checked the nested event loop ones on Windows and they work for me. I just pushed a change which may fixes this for Mac as well. I also will do a much bigger testing soon, I have an application that relies heavily on nested event loops for blocking the UI while doing background work. |
I don't know of any supported way to run macOS in a VM on a Windows host, but perhaps others will be able to help.
It worked for me on Windows, too (and on Linux in a headful test run). I would want to make sure that we know why they still work on Windows and Linux and fail on Mac, but I only saw the failures on Mac.
I can also fire off a headful test run this afternoon, and post results tomorrow.
The results of that testing would be good information. |
I see the same failure with the most recent patch as I did yesterday. I looked over the changes, and I'm not convinced that this is the right approach. One comment on your proposal:
This changes one of the fundamental aspects of entering and exiting a nested event loop. This will need a lot more analysis to show why this is the right approach. |
Yes, this is right and I also thought about documenting that as soon as we think that this is the right approach. Maybe there is a better way as well, I just don't figured out one.
Thanks for testing. As soon as my colleague is available, I will debug this issue. I don't get why only MacOS is failing, since the low level code is pretty much the same, we just continue right after we left the nested event loop.
That was also my understanding the last time I checked. AFAIK, there is no official way from Apple, but VirtualBox may offer that. Will check as soon as I have more time. |
The Mac is failing on the simplest NestedEventLoop test, the one that verifies that we can enter and exit a nested loop and correctly retrieve the return value. I started looking into this but find what's happening in Glass a bit confusing. I'll take a closer look in the next day or two. I don't understand the original failure case. Events aren't blocked and you can type into the modal's text field, it just doesn't repaint. If you resize the window you will see your edits. |
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:
When the invokeLaterDispatcher is told that the innermost loop is exiting it currently sets |
Well, yes and no. My testing was showing that |
…893-dialog-freezing-🥶
So I could finally look into the issue. Unfortunately, the problem is much deeper than I thought.
What I would expect:
New behaviour:
Old behaviour:
Application hangs (Makes sense, as it is the same logic as if we hide a dialog and show a new one) So it is better now, since we do not stall the application anymore, but we never return back from the first Maybe, the approach needs to be a little bit different. |
The first
There is that bug in the invokeLaterDispatcher which will prevent the second By the way, when I grab the code in the original bug report and run it agains this PR the innermost modal is not blocked and everything seems fine. But if I press the Close button exceptions start getting thrown. This is happening on both Mac and Windows. |
You are right, this sounds correct.
Can you post the exception? I may know which one and I already fixed one locally, testing it currently. Technically speaking, we have 2 issues here.
|
The top of the call stack when the exception is thrown:
|
Yeah, had the same issue and fixed it locally. Now pushed. Problem as stated above remains though |
508e256
to
0bcda3a
Compare
@Maran23 Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
I may found a solution which fixes the dialog bug and passes my test above.
|
The Mac is still failing the NestedEventLoop test. At the top of InvokeLaterDispatcher.java there's a long explanation on how it's trying to coordinate with the event loop. To do this it needs to have an accurate picture of what's happening at the platform level. In this PR you're telling the dispatcher that an event loop has exited before the platform has actually exited the loop (before enterNestedEventLoop has returned). This is causing the dispatcher to send runLater runnables to the platform earlier than expected. On Windows the runnables will get deferred to the next event loop cycle but on Mac the runnables might get executed in the current event loop cycle. I think this is one of the "native system limitations" mentioned in the comment in InvokeLaterDispatcher.java. I've created a branch with my proposed fix for this problem (https://github.com/beldenfox/jfx/tree/eventloopjam). The fix prevents the dispatcher from jamming when the event loop it thought was leaving enters a new loop instead. Over in the Mac Glass code I also added a comment with a few more details on what's going on. |
Thanks for testing this on Mac. I always need to ask someone in order to test the changes, so really appreciating that.
I tested your code and it looks good! So this seems to be the better approach here. While mine works for Windows and maybe Linux, it does not for Mac as you also specified. The return value of the native nested event loops are still not needed, so we can still think about keeping this change, but we should probably revert the EventLoop changes and take your fix instead. |
/contributor add @beldenfox Thanks! I feel like we are coming closer to the fix of this problem. I hopefully have time to test this much more tomorrow - I have at least "booked" myself a time slot for JavaFX. 😄 |
@Maran23 |
❗ This change is not yet ready to be integrated. |
@Maran23 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
Please don't close this pull request. I really need this fix. |
I just submitted my proposed fix for this issue as PR #1449. |
…893-dialog-freezing-🥶
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
andgtk_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.
_leaveNestedEventLoop
is called.gtk_main_quit
._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 fromgtk_main
on Linux.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 callnotifyLeftNestedEventLoop
in_leaveNestedEventLoop
, after the native code was called.See below for more information. To recover the UI (and in general nested nested event loops ;) we need to set a flag for the
InvokeLaterDispatcher
.Lets see if this is the right approach (for all platforms).
Testing appreciated.
Progress
Issue
Contributors
<mfox@openjdk.org>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1324/head:pull/1324
$ git checkout pull/1324
Update a local copy of the PR:
$ git checkout pull/1324
$ git pull https://git.openjdk.org/jfx.git pull/1324/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1324
View PR using the GUI difftool:
$ git pr show -t 1324
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1324.diff
Webrev
Link to Webrev Comment