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

8256465: [macos] Java frame and dialog presented full screen freeze application #3407

Closed
wants to merge 13 commits into from

Conversation

trebari
Copy link
Member

@trebari trebari commented Apr 9, 2021

Hi All,
Please review the following fix for jdk17.

Issue : On MacOS 11 Java Frame and JDialog application is freezing in Full Screen when the System Preference -> General -> Prefer Tabs is set to "Full Screen". It is also freezing in normal screen when Prefer Tabs is set to "Always".
It doesn't freeze when the Prefer tabs is set to "never".

Fix : The default value of allowsAutomaticWindowTabbing is 0/NO in MacOS prior to bigSur(11)
(in the AWTWindow.m file), so the issue is not seen in mac os 10.13 10.14 and 10.15.
From MacOS 11 onwards this value is set to 1/YES and the issue is seen.
This issue can also be reproduced in MacOS 10.15 by setting setAllowsAutomaticWindowTabbing to true in the AWTWindow.m file.

Fix is to set allowsAutomaticWindowTabbing to No for all the MacOS release staring from 10.12.
The allowsAutomaticTabbing was introduced in MacOS 10.12 but the default value changed in macos11.

Test : Added a manual test and tested on MacOS 10.15 and 11.
All the internal tests run are green.


Progress

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

Issue

  • JDK-8256465: [macos] Java frame and dialog presented full screen freeze application

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3407

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 9, 2021

👋 Welcome back trebari! 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 Pull request is ready for review label Apr 9, 2021
@openjdk
Copy link

openjdk bot commented Apr 9, 2021

@trebari The following label will be automatically applied to this pull request:

  • awt

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the awt client-libs-dev@openjdk.org label Apr 9, 2021
@mlbridge
Copy link

mlbridge bot commented Apr 9, 2021

@mrserb
Copy link
Member

mrserb commented Apr 9, 2021

Fix is to set allowsAutomaticWindowTabbing to No for all the MacOS release staring from 10.12.
The allowsAutomaticTabbing was introduced in MacOS 10.12 but the default value changed in macos11.

But why the option setAllowsAutomaticWindowTabbing=yes does not work? Why the nswindow in JDK is so specific?

@trebari
Copy link
Member Author

trebari commented Apr 9, 2021

Fix is to set allowsAutomaticWindowTabbing to No for all the MacOS release staring from 10.12.
The allowsAutomaticTabbing was introduced in MacOS 10.12 but the default value changed in macos11.

But why the option setAllowsAutomaticWindowTabbing=yes does not work? Why the nswindow in JDK is so specific?

As per my understanding we don't have support for opening window in a new Tab.And also it is always not a good idea to open in new Tab as the dialog size could be different. Similar fix is done for JavaFX(https://git.openjdk.java.net/jfx/pull/440) and for other application(like SPSS) it is advised to set System Preference -> General -> Prefer tabs to never.

@mrserb
Copy link
Member

mrserb commented Apr 9, 2021

As per my understanding we don't have support for opening window in a new Tab.

But why we unsupport them? Is it depend on the dialog type? For example, if the option is set to "always prefer tabs" then the test case instruction and the test dialog with click button will use tabs and it works great.

Why the dialog hangs after pressing the click button?

@kevinrushforth
Copy link
Member

kevinrushforth commented Apr 9, 2021

Without this fix, owned windows will open in a new window and new top level windows will open in a tab. As I discovered when evaluating JDK-8263169, which @trebari referred to, if you open a modal window that isn't a child window of some other window, it can lead to strange behavior when you try to tab between them. Also, if the size of a new window is different, it will resize the shared frame that both windows use, and not restore it when you click the tab to go back to the other window.

Without some sort of API support to give the application a choice (i.e., to "opt in" to the tabbing behavior), I think it makes the most sense to do what we did for JavaFX and what Tejpal proposed to do here.

@kevinrushforth
Copy link
Member

kevinrushforth commented Apr 9, 2021

It should be possible to create an automated robot test for this. I did this for the JavaFX fix by popping up two top level windows in different locations and verifying that I could click a button in each window.

@mrserb
Copy link
Member

mrserb commented Apr 9, 2021

if you open a modal window that isn't a child window of some other window, it can lead to strange behavior when you try to tab between them.

Yes, this is what I tried to clarify, why we have such "strange behavior", what is the root cause, and how hard it to fix.

Also, if the size of a new window is different, it will resize the shared frame that both windows use, and not restore it when you click the tab to go back to the other window.

That probably can be changed on our side, this looks similar to the case when the window is maximized to the "macOS fullscreen" and then restored .

@kevinrushforth
Copy link
Member

kevinrushforth commented Apr 10, 2021

There is a larger issue here than just a freeze in full-screen mode. A user can set their "Prefer tabs" preference for opening documents to "Always" (rather than the default "in full screen") on either 10.15 or 11.0, and then all top level windows will open up in a tab unless they are child (owned) windows. This leads to a variety of unexpected behavior, which is why I think disabling it (as we did for JavaFX and as is proposed in this PR) is the best option. An application's windows are not always conceptually a set of related documents, so opening up all windows in tabs seems wrong.

@mrserb
Copy link
Member

mrserb commented Apr 10, 2021

There is a larger issue here than just a freeze in full-screen mode. A user can set their "Prefer tabs" preference for opening documents to "Always" (rather than the default "in full screen") on either 10.15 or 11.0, and then all top level windows will open up in a tab unless they are child (owned) windows.

Yes, I have tested that.

This leads to a variety of unexpected behavior, which is why I think disabling it (as we did for JavaFX and as is proposed in this PR) is the best option. An application's windows are not always conceptually a set of related documents, so opening up all windows in tabs seems wrong.

All above are applicable to any applications and all frames/dialogs on the new macOS. For some reason, Apple decided this can be a good design. Why we do not try to fit in it?

  • We may try to configure the correct size when a few windows are merged to the same window
  • We may try to fix a "hang" of some type of the windows
  • And we may try to disable it for some type of the windows by creating "fake" owner.

@prrace
Copy link
Contributor

prrace commented Apr 10, 2021

It is something of a new paradigm (yes its been there since 10.12, that's not what I mean) so
we should make sure its supportable.

The system dialog UI talks of "documents" which might tell us something about the mindset
of the folks who designed it. I also find it surprising it is a global system setting.
It seems like something that is better set per-application. Of course if a Java runtime is used
for multiple applications that wouldn't help but it is moot for now so let's not spend time on that

It would be interesting to have an enumeration of known and suspected problems with this.

  • where is it inappropriate UI - if we have an unowned dialog it is really weird to popup it up in a new tab.
    Are just at odds with the mac desktop where dialogs are always owned windows ?
    Do they imagine that all windows can layout nicely even if they don't get the size they want ?
    I'm having trouble picturing how all this works smoothly
  • where do we have behavioural problems - full screen oddities, application freezes, incorrect behaviour
    of menu bars .. focus ..
  • what scenarios do we need to examine ?

I can imagine it would take some time to properly go through all of these and I think we should
schedule that rather than just disabling this and in all likelihood forgetting about this.
Do we have a go-with-the disabling RFE to support it ?

If applications are freezing then disabling it might be an appropriate short term solution
We could consider a system property to prevent the disabling .. in case some one has an app where they'd
really like to use it and understand the risks.

Maybe new API is needed to make it supportable - which would suggest to me that Apple made a mistake
in the way they implemented this.

Probably this disabling is needed for older releases as new API won't make it back to those.

@mlbridge
Copy link

mlbridge bot commented Apr 10, 2021

Mailing list message from Philip Race on awt-dev:

It is something of a new paradigm (yes its been there since 10.12,
that's not what I mean) so
we should make sure its supportable.

The system dialog UI talks of "documents" which might tell us something
about the mindset
of the folks who designed it. I also find it surprising it is a global
system setting.
It seems like something that is better set per-application. Of course if
a Java runtime is used
for multiple applications that wouldn't help but it is moot for now so
let's not spend time on that

It would be interesting to have an enumeration of known and suspected
problems with this.
- where is it inappropriate UI - if we have an unowned dialog it is
really weird to popup it up in a new tab.
?? Are just at odds with the mac desktop where dialogs are always owned
windows ?
??? Do they imagine that all windows can layout nicely even if they
don't get the size they want ?
??? I'm having trouble picturing how all this works smoothly
- where do we have behavioural problems - full screen oddities,
application freezes, incorrect behaviour
of menu bars .. focus ..
- what scenarios do we need to examine? ?

I can imagine it would take some time to properly go through all of
these and I think we should
schedule that rather than just disabling this and in all likelihood
forgetting about this.
Do we have a go-with-the disabling RFE to support it ?

If applications are freezing then disabling it might be an appropriate
short term solution
We could consider a system property to prevent the disabling .. in case
some one has an app where they'd
really like to use it and understand the risks.

Maybe new API is needed to make it supportable - which would suggest to
me that Apple made a mistake
in the way they implemented this.

Probably this disabling is needed for older releases as new API won't
make it back to those.

-phil.

On 4/10/21 1:30 PM, Sergey Bylokhov wrote:

@kevinrushforth
Copy link
Member

kevinrushforth commented Apr 12, 2021

The system dialog UI talks of "documents" which might tell us something about the mindset of the folks who designed it.

Yes, that was my thinking. This seems like a setting meant for applications whose windows are a set of "documents", and where grouping the individual document windows into a single window as set of tabs make sense. It doesn't really fit for applications that have different top level windows that aren't documents that can be grouped together naturally into a single window with tabs.

I can imagine it would take some time to properly go through all of these and I think we should schedule that rather than just disabling this and in all likelihood forgetting about this. Do we have a go-with-the disabling RFE to support it ?

That seems a good approach to me: make this an opt-in feature that applications could decide to use. And if you go that route, filing the RFE now is a good idea. In any case, I'll file an RFE for JavaFX as well.

@trebari
Copy link
Member Author

trebari commented Apr 27, 2021

It would be interesting to have an enumeration of known and suspected problems with this.

  • where is it inappropriate UI - if we have an unowned dialog it is really weird to popup it up in a new tab.
    Are just at odds with the mac desktop where dialogs are always owned windows ?

Mac OS Alert opens up in new window no matter what the Prefer Tab setting is.

Do they imagine that all windows can layout nicely even if they don't get the size they want ?
I'm having trouble picturing how all this works smoothly

  • where do we have behavioural problems - full screen oddities, application freezes, incorrect behaviour
    of menu bars .. focus ..
  • what scenarios do we need to examine ?

I can imagine it would take some time to properly go through all of these and I think we should
schedule that rather than just disabling this and in all likelihood forgetting about this.
Do we have a go-with-the disabling RFE to support it ?

I have filed a RFE for this : https://bugs.openjdk.java.net/browse/JDK-8266026.

@prrace
Copy link
Contributor

prrace commented May 7, 2021

@trebari where are we with my (off-line) suggestion to you that this fix be updated such that a System Property "-Djdk.allowTabbedWindows=true" be defined that lets someone over-ride the disabling so that folks can at least try out the effect of this on their apps whilst we figure out a supportable API ?

@trebari
Copy link
Member Author

trebari commented May 10, 2021

I am working on it. Will update the next version soon.

@trebari
Copy link
Member Author

trebari commented May 11, 2021

Is there any place where we specify that "-Djdk.allowTabbedWindows" is a new jdk property which is for the macos automatic window tabbing.
This is off by default and can be set using -Djdk.allowTabbedWindows=true/TRUE/TrUE ... .

@trebari
Copy link
Member Author

trebari commented May 13, 2021

/CSR

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label May 13, 2021
@openjdk
Copy link

openjdk bot commented May 13, 2021

@trebari has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@trebari please create a CSR request for issue JDK-8256465. This pull request cannot be integrated until the CSR request is approved.

(JNIEnv *env, jclass clazz, jboolean allowAutomaticTabbing)
{
JNI_COCOA_ENTER(env);
if (@available(macOS 10.12, *)) {
Copy link
Contributor

@prrace prrace May 13, 2021

Choose a reason for hiding this comment

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

@kevinrushforth said that since we set MIN_SDK (not sure of the exact variable name) to 10.12, that this is compiled down to a no-op .. which means it is useless and doesn't protect you from making the call on 10.11
So you might as well remove it. It won't prevent the crash that will happen on 10.11.
@mrserb also pointed out people might then copy this pattern not realising it does not work, and there's a better way ... apparently ...

Copy link
Member

@kevinrushforth kevinrushforth May 13, 2021

Choose a reason for hiding this comment

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

Right. @johanvos discovered this fun fact about @available when he got a crash report from a user. He filed JDK-8266743, which describes this problem.

The setting of minimum version of macOS is controlled by the -mmacosx-version-min compile and link flag. The minimum version is defined in make/autoconf/flags.m4 and used in make/autoconf/flags-cflags.m4.

One thing I don't know (and can't try, since I don't have access to a macOS system that old) is whether the JDK will fail somewhere else anyway (e.g., if they check for a minimum OS at start up). So this might be a moot point, but as it stands, I think @mrserb is right that we should avoid this pattern. I would probably just remove it, but you could decide to use something like respondsToSelector (which is what I think Sergey was suggesting).

Copy link
Contributor

@prrace prrace May 13, 2021

Choose a reason for hiding this comment

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

Since OpenJFX does not have its own launcher and IIRC JDK only recently (JDK 17 b08 https://bugs.openjdk.java.net/browse/JDK-8260518 ) set the minimum to 10.12 it is possible that the submitter of the FX crash was using A JDK prior to that, in which case I am sure the Java Launcher would start up fine and you'd crash only when calling this code. So also I think very aguably library code has another reason to avoid this pattern.
And verifying what happens on 10.11 might be best done with a launcher from JDK 17 b07 or later .. also @kevinrushforth you might want to add some of these thoughts to the FX bug.

@prrace
Copy link
Contributor

prrace commented May 13, 2021

Is there any place where we specify that "-Djdk.allowTabbedWindows" is a new jdk property

That's what the CSR is for.

@trebari
Copy link
Member Author

trebari commented May 17, 2021

@trebari
Copy link
Member Author

trebari commented May 20, 2021

Please review the CSR https://bugs.openjdk.java.net/browse/JDK-8267238 as well.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

The fix looks fine, although you need to merge in the latest changes from the jdk master and add the now-required annotation to ignore the security manager deprecation warning (else it will fail to compile). I left a couple comments on the test as well.

test/jdk/java/awt/Window/TestAppFreeze.java Outdated Show resolved Hide resolved

private static void testApp() {
testFrame = new JFrame("TestFrame");
testFrame.setBounds(600,0,1000,200);
Copy link
Member

@kevinrushforth kevinrushforth Jun 3, 2021

Choose a reason for hiding this comment

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

Minor: add spaces after the commas.

test/jdk/java/awt/Window/TestAppFreeze.java Outdated Show resolved Hide resolved
@trebari trebari changed the title 8256465: [macos11] Java frame and dialog presented full screen freeze application 8256465: [macos] Java frame and dialog presented full screen freeze application Jun 3, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

The updates look good. I noted one more test issue and otherwise I think this is ready to go.

test/jdk/java/awt/Window/TestAppFreeze.java Outdated Show resolved Hide resolved
@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Jun 4, 2021
mrserb
mrserb approved these changes Jun 5, 2021
@openjdk
Copy link

openjdk bot commented Jun 5, 2021

@trebari This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8256465: [macos] Java frame and dialog presented full screen freeze application

Reviewed-by: kcr, serb, prr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 39 new commits pushed to the master branch:

  • b05c40c: 8266951: Partial in-lining for vectorized mismatch operation using AVX512 masked instructions
  • f768fbf: 8268286: ProblemList serviceability/sa/TestJmapCore.java on linux-aarch64 with ZGC
  • b2e9eb9: 8268087: Update documentation of the JPasswordField
  • 91f9adc: 8268139: CDS ArchiveBuilder may reference unloaded classes
  • 36bff6f: 8066694: Strange code in JavacParser.java
  • 6c838c5: 8266846: Add java.time.InstantSource
  • 7f55dc1: 8179880: Refactor javax/security shell tests to plain java tests
  • 7e41ca3: 8266957: SA has not followed JDK-8220587 and JDK-8224965
  • 6ff978a: 8267204: Expose access to underlying streams in Reporter
  • 76b54a1: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native
  • ... and 29 more: https://git.openjdk.java.net/jdk/compare/29ab16284a4f1ac7ed691fd12cb622b0440c04be...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 5, 2021
prrace
prrace approved these changes Jun 5, 2021
@trebari
Copy link
Member Author

trebari commented Jun 6, 2021

/integrate

@openjdk openjdk bot closed this Jun 6, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 6, 2021
@openjdk
Copy link

openjdk bot commented Jun 6, 2021

@trebari Since your change was applied there have been 40 commits pushed to the master branch:

  • 8abf36c: 8268289: build failure due to missing signed flag in x86 evcmpb instruction
  • b05c40c: 8266951: Partial in-lining for vectorized mismatch operation using AVX512 masked instructions
  • f768fbf: 8268286: ProblemList serviceability/sa/TestJmapCore.java on linux-aarch64 with ZGC
  • b2e9eb9: 8268087: Update documentation of the JPasswordField
  • 91f9adc: 8268139: CDS ArchiveBuilder may reference unloaded classes
  • 36bff6f: 8066694: Strange code in JavacParser.java
  • 6c838c5: 8266846: Add java.time.InstantSource
  • 7f55dc1: 8179880: Refactor javax/security shell tests to plain java tests
  • 7e41ca3: 8266957: SA has not followed JDK-8220587 and JDK-8224965
  • 6ff978a: 8267204: Expose access to underlying streams in Reporter
  • ... and 30 more: https://git.openjdk.java.net/jdk/compare/29ab16284a4f1ac7ed691fd12cb622b0440c04be...master

Your commit was automatically rebased without conflicts.

Pushed as commit 042f0bd.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awt client-libs-dev@openjdk.org integrated Pull request has been integrated
5 participants