Skip to content
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

8273485: Deadlock when also using Swing and exiting Fullscreen on Mac #622

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

@FlorianKirmaier
Copy link
Member

@FlorianKirmaier FlorianKirmaier commented Sep 8, 2021

When using Swing it's possible to generate a Deadlock.
It's related to the nested eventloop started in enterFullScreenExitingLoop - and the RenderLock aquired when using setView in Scene.
Sample Programm and Threaddump are added to the ticket.

Removing the nested loop fixes the Problem.
I hope this doesn't have any side effect - so far i don't know of any.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8273485: Deadlock when also using Swing and exiting Fullscreen on Mac

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/622/head:pull/622
$ git checkout pull/622

Update a local copy of the PR:
$ git checkout pull/622
$ git pull https://git.openjdk.java.net/jfx pull/622/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 622

View PR using the GUI difftool:
$ git pr show -t 622

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/622.diff

Fixing deadlock when switching to fullscreen, when also swing is used.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 8, 2021

👋 Welcome back fkirmaier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Sep 8, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 8, 2021

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 8, 2021

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Sep 8, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

Copy link
Member

@kevinrushforth kevinrushforth left a comment

I am almost certain that this proposed fix will cause problems. At the very least we will now have mismatched enter / exit nested loop calls. The nested loop for full screen on Mac was done for a reason, so you will need to track down what that reason is and either verify that it is no longer needed or else solve that some other way.

Also, you don't explain why removing the nested event loop is the right fix for the deadlock.

Finally, once you address the above two points, you will need to turn the test program attached to the JBS bug into an automated test.

@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Sep 9, 2021

Thank you for the feedback - I will start writing an unit test for it because it will help on all further development regarding this issue.

Added unit-test
Removed the enter/leave nested event loop logic, for mac fullscreen
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Sep 13, 2021

I've now added a unit test.

I've adapted the solution to entirely remove the nested event loop, for the fullscreen. So far everything seems to work.
I've tested it with a simple application to check how the fullscreen behaves.
The logic seems to be added before the beginning of the VCS / 2013
Is there away to look into the history before 2013? Maybe there is a hint, why this was added.
Otherwise i would guess that it is not too important, and it is just an unnecessary overcomplicated solution.

@kevinrushforth kevinrushforth self-requested a review Sep 16, 2021
small cleanup of the changes.
Copy link
Member

@kevinrushforth kevinrushforth left a comment

I've adapted the solution to entirely remove the nested event loop, for the fullscreen. So far everything seems to work.
I've tested it with a simple application to check how the fullscreen behaves.
The logic seems to be added before the beginning of the VCS / 2013
Is there away to look into the history before 2013? Maybe there is a hint, why this was added.

I tracked down the bug fix that added the nested event loop (looking at the history of the closed-source changes, since this fix was done prior to open-sourcing glass). It was added to fix JDK-8126842.

As I suspected, the nested even loop is still necessary. If you run the test program attached to JDK-8126842 with your patch applied, make the window full-screen using the green button, then click the close button, the program will terminate with an exception.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 17, 2021

You can also see the exception with any JavaFX program: enter full-screen with the green button, move the mouse to the upper-left corner to reveal the window decorations, and press the red button to exit the program.

I think you need to look for a different solution to your deadlock, one that preserves the existing nested event loop.

@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Sep 21, 2021

Thank you for searching the original reason for the nested event loop!

I've now tested the class from the ticket, and for mit it works. The only strange thing is, that i get a "beep" sound when closing. Which isn't optimal but not the reported crash. Which is kinda unfortunate, because otherwise it might be easy to write a unit test.
Did you test the latest version of the PR?

I guess now i have to think of another fix, or on how do adapt this one.

@kevinrushforth kevinrushforth self-requested a review Sep 23, 2021
Fixed Beep sound when closing a fullscreen window
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Sep 28, 2021

@kevinrushforth
Did you test really test my change and was able to reproduce the JDK-8126842 with it?
On my system everything works - except the small "beep" sound.

After investigating it, I've made another small change - which fixes the beep sound. It also makes it closer to the implementation for iOS. I've changed the waitUntilDone value to true, when closing the window.

I think removing the nestedEventLoops should be preferred over another fix - in my experience the nested event loops are the cause of so many rare and very complicated bugs.

removed the toggle fullscreen before closing - to avoid the beep and improve the user experience
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Sep 28, 2021

I've just noticed that I've missed to commit a change, to prevent the beep - which I've now added.

The changes I've now committed also have another welcome side effects:
JavaFX behaves more like "normal" Mac apps.

Before:

  • Animation out of fullscreen
  • Closing the window

Now:

  • Closing the window

This is how other mac applications behave - and should probably therefore be preferred. I've tested it with apps like Finder, Textedit, Chrome, Safari.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 14, 2021

The latest version of the fix looks better. There is only one remaining problem that I see: If an application calls Platform.exit() while in full-screen mode the following exception will be thrown and printed to the console:

Exception in thread "JavaFX Application Thread" java.lang.NullPointerException: Cannot invoke "com.sun.glass.ui.Application.staticScreen_getScreens()" because the return value of "com.sun.glass.ui.Application.GetApplication()" is null
	at javafx.graphics@18-internal/com.sun.glass.ui.Screen.initScreens(Screen.java:410)
	at javafx.graphics@18-internal/com.sun.glass.ui.Screen.notifySettingsChanged(Screen.java:379)

This will happen regardless of whether full-screen was entered via the green full-screen button, or by a call to Stage::setFullScreen(true). The easiest way to reproduce this is with the HelloFullscreen test program, which is in apps/toys/Hello:

  1. Run java hello.HelloFullscreen
  2. Press the Exit button.

If instead you either press the Close button (which hides the stage) or use the red X in the window decoration, the application closes without throwing the exception.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 14, 2021

I spoke too soon. I was running a few other manual tests, and your latest fix completely breaks the normal exit from full screen mode. To reproduce this:

  1. Run java hello.HelloFullscreenToggle
  2. Press the Toggle Fullscreen button to enter full-screen mode
  3. Try to exit full screen mode, either by pressing the ESC key or by pressing the Toggle Fullscreen button again.
    BUG: you can't exit full-screen mode.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

I think that removing the call to toggleFullScreen in exitFullscreenWithAnimate is what's causing the failure to exit from fullScreen.

[self->nativeFullScreenModeWindow performSelector:@selector(toggleFullScreen:) withObject:nil];
// wait until the operation is complete
[GlassApplication enterFullScreenExitingLoop];
Copy link
Member

@kevinrushforth kevinrushforth Oct 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need to restore the call to toggleFullScreen in order to support exiting from full-screen.

Fixing toggle fullscreen!
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Oct 29, 2021

not a real regression

I think I've hunted a regression, which was none. When closing the application with System.exit the screen gets red, but this isn't caused by my change.
I've even got a fix for it, but it requires changing the lifecycle slightly, by closing all windows on shutdown. (And some other minor changes to fix some exceptions)

        Runtime.getRuntime().addShutdownHook(new Thread(() -> {
            System.out.println("tkExit shutdown hook!");
            PlatformImpl.tkExit();
        }));

But because this isn't caused by my changes, we can therefore ignore it. But I had to mention it because I invested too much time into it.

toggle

I've readded the toggle code. It's worth mentioning that the code is different than before.

The original version caused a "beep sound" when closing the window [self->nativeFullScreenModeWindow performSelector:@selector(toggleFullScreen:) withObject:nil];

The new version doesn't have this problem - but it's also fixing the problem from the previous version. [[self->nsView window] toggleFullScreen:self];

exception

With the current version, I also cannot reproduce the problem, with the exception.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 29, 2021

I still see the exception described in this comment when the application calls Platform.exit() while in full screen. I am running on macOS 10.15.7. I'll try it on macOS 11.x as well.

@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Oct 29, 2021

I will also recheck for the Exception. I've seen it multiple times, especially when investigating the "red screen", but I haven't seen it with the latest version. But maybe I'm testing it now somehow differently.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

What version of macOS did you test on? I was running on 10.15.7 (Catalina). I just tried it on macOS 11.5 (Big Sur) and the exception does not happen, and on 10.14.6 (Mojave) where is does. To summarize, when calling Platform.exit while in full-screen mode:

macOS 10.14.6 : exception
macOS 10.15.7 : exception
macOS 11.5 : OK

[self->nativeFullScreenModeWindow performSelector:@selector(toggleFullScreen:) withObject:nil];
// wait until the operation is complete
[GlassApplication enterFullScreenExitingLoop];
[[self->nsView window] toggleFullScreen:self];
Copy link
Member

@kevinrushforth kevinrushforth Oct 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you added the call to toggleFullScreen back in, but you now pass self to the method, where the previous code passed nil. Unless there is a compelling reason why you need to change it, I recommend to restore exactly the former code, such that there are no diffs:

            [self->nativeFullScreenModeWindow performSelector:@selector(toggleFullScreen:) withObject:nil];

Copy link
Member Author

@FlorianKirmaier FlorianKirmaier Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the previous version, I get a "beep" sound when closing a window.
But with the changed line, it works for me and I don't get any issues.
The beep sound also was the original reason, why I've removed the line.

@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Nov 2, 2021

I've been using macOS 11.6. It is also not so easy for me to test another version.

I guess the exception can easily be "fixed" by checking whether the application is null, in the Screen.initScreens method. Should I change it this way?

@kevinrushforth kevinrushforth self-requested a review Nov 2, 2021
Added check for null when calling initScreens
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Nov 15, 2021

I've now added a check for null when calling the initScreens method. This should fix the null pointer exception.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 participants