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

8296972: [macos13] java/awt/Frame/MaximizedToIconified/MaximizedToIconified.java: getExtendedState() != 6 as expected. #14226

Closed
wants to merge 7 commits into from

Conversation

alisenchung
Copy link
Contributor

@alisenchung alisenchung commented May 30, 2023

added displayChanged call to CPlatformWindow when frame first needs to deiconify or unmaximize
All client tests passed after change


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8296972: [macos13] java/awt/Frame/MaximizedToIconified/MaximizedToIconified.java: getExtendedState() != 6 as expected. (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14226/head:pull/14226
$ git checkout pull/14226

Update a local copy of the PR:
$ git checkout pull/14226
$ git pull https://git.openjdk.org/jdk.git pull/14226/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14226

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14226.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented May 30, 2023

👋 Welcome back achung! 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 May 30, 2023
@openjdk
Copy link

openjdk bot commented May 30, 2023

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

  • client

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 client client-libs-dev@openjdk.org label May 30, 2023
@mlbridge
Copy link

mlbridge bot commented May 30, 2023

Webrevs

@@ -986,13 +986,15 @@ public void setWindowState(int windowState) {
// let's return into the normal states first
// the zoom call toggles between the normal and the max states
unmaximize();
peer.displayChanged();
Copy link
Member

Choose a reason for hiding this comment

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

Why do you call peer.displayChanged() only when the previous windows state is MAXIMIZED_BOTH? What happens if the previous windows state is NORMAL?

}
execute(CWrapper.NSWindow::miniaturize);
break;
case Frame.MAXIMIZED_BOTH:
if (prevWindowState == Frame.ICONIFIED) {
// let's return into the normal states first
execute(CWrapper.NSWindow::deminiaturize);
peer.displayChanged();
Copy link
Member

Choose a reason for hiding this comment

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

Same as above. What happens if we move from NORMAL to MAXIMIZED_BOTH?

Shall we call peer.displayChanged() when a frame is moved to NORMAL state? I know that cases are not covered by the test but it seems the issue takes place there. Can you investigate, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the main issue is that there is a native event that changes the window state, but when two state changes are done back to back sometimes that native event isn't delivered quickly enough before the second state change occurs, which causes this test to fail. When a frame is moved to a normal state, there are no back to back state changes, since all we do is change the state back to normal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I set a delay between the state changes, the problem will also disappear and the test passes.

@mrserb
Copy link
Member

mrserb commented May 30, 2023

Why this method is not called automatically by our native code? what events are changed since macOS 13.0?

@mrserb
Copy link
Member

mrserb commented May 30, 2023

BTW it is unclear what the call to "displayChanged" will fix. Will it set the correct "graphicsDevice", if yes then what gD was used before the call, if not then what property is updated by the "displayChanged"?

@alisenchung
Copy link
Contributor Author

BTW it is unclear what the call to "displayChanged" will fix. Will it set the correct "graphicsDevice", if yes then what gD was used before the call, if not then what property is updated by the "displayChanged"?

I think the problem is that the first state change (uniconify) causes a native event that changes the frame's state, but that event isn't delivered until after the second state change (maximize) occurs, which is causing problems on the final state of the frame. The displayChanged call between these state changes was intended to flush the native event queue and cause the event to be delivered between the state changes.

@dmarkov20
Copy link
Member

If I got it right the switching to ICONIFIED/MAXIMIZED_BOTH state takes more time on macOS 13 than on previous macOS version. So the test failure is triggered by the lack of synchronisation.
When you added displayChanged() call you introduced some kind of delay and the problem stopped happening but it is still present. I do not think any changes are required in JDK but the test definitely needs some modifications.
In particular I believe the test must check the state of the frame only when the corresponding window state changed event is received.

@honkar-jdk
Copy link
Contributor

honkar-jdk commented Jun 3, 2023

Why this method is not called automatically by our native code? what events are changed since macOS 13.0?

@mrserb FWIW, There might be a new window state in macOS 13 due to a the new Stage Manager feature but I believe this shouldn't affect any existing states and moreover the Stage Manager option is off by default.

@alisenchung
Copy link
Contributor Author

If I got it right the switching to ICONIFIED/MAXIMIZED_BOTH state takes more time on macOS 13 than on previous macOS version. So the test failure is triggered by the lack of synchronisation. When you added displayChanged() call you introduced some kind of delay and the problem stopped happening but it is still present. I do not think any changes are required in JDK but the test definitely needs some modifications. In particular I believe the test must check the state of the frame only when the corresponding window state changed event is received.

I did some testing and I think that without the delay, when the frame moves from iconified to maximized states, the second windowStateChanged event never comes (first being iconified -> normal, second being normal -> maximized), so the test will not pass even if we only check state of the frame on a windowStateChanged event.

Why do you think no changes are required in the JDK? Shouldn't there be some synchronization needed when calling the native deminiaturize and maximize functions in CPlatformWindow?

@mrserb
Copy link
Member

mrserb commented Jun 7, 2023

Possibly some synchronization is needed in the jdk, it needs to be checked why the message is not posted. But it should not be fixed by the "displayChanged" used as a "delay"

@honkar-jdk
Copy link
Contributor

honkar-jdk commented Jun 7, 2023

@alisenchung @mrserb

After reading through the discussion, I think the fix on native side rather than on Java might work better here. I was revisiting a Tray Icon issue which had similar sync problem when I was trying to get updated scale value on Java side by using a DisplayChangedListener [Details here - PR#8441] and there were multiple events occurring back to back. When I added a hard-coded delay set on Java side, the update to tray icon worked fine. I think this is a similar case where introducing a delay solves the issue. But since hard-coded delay is not the ideal solution probably triggering/ calling the required piece of code after everything is updated on native side is the way to go (event/message-driven).

In case of TrayIcon issue, fixing the issue on native side using event-driven wait worked.

@honkar-jdk
Copy link
Contributor

Also noticed this comment in setWindowState() of CPlatformWindow.java#L1011

// NOTE: the SWP.windowState field gets updated to the newWindowState
// value when the native notification comes to us

May be waiting untill the required native notification using an event-driven wait might work here?

@honkar-jdk
Copy link
Contributor

honkar-jdk commented Jun 7, 2023

@alisenchung Before digging deeper into Cocoa Event Handling, please check with someone more experienced with native event handling and if fixing on native side by waiting for an appropriate NSEvent is a viable solution in this case.

A gist of Cocoa Event Handling mechanism -

NSRunLoop in the event loop NSApplicationAWT.m#L303 handled by NSApp.
NSApp converts this event into NSEvent object and dispatches using sendEvent to the appropriate NSView object.

@@ -986,13 +986,16 @@ public void setWindowState(int windowState) {
// let's return into the normal states first
// the zoom call toggles between the normal and the max states
unmaximize();
peer.getLWToolkit().realSync();
Copy link
Member

Choose a reason for hiding this comment

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

Well.. realSync() works here but I am not sure if that invocation really fixes the issue or just hides it by introducing a delay.
I believe we need some function which ensures that the frame is in correct (NORMAL) state right after unmaximize() call and before execute(CWrapper.NSWindow::miniaturize) invocation.

Possible implementation of that method below:

private void waitForWindowState(int state) {
        Object lock = new Object();
        target.addWindowStateListener(new WindowStateListener() {
            public void windowStateChanged(WindowEvent e) {
                synchronized(lock) {
                    if (e.getNewState() == state) {
                        lock.notifyAll();
                    }
                }
            }
        });
        if (peer.getState() != state) {
            synchronized(lock) {
                try {
                    lock.wait();
                } catch(InterruptedException ie) {}
            }
        }
    }

So in code you will call waitForWindowState(Frame.NORMAL) instead of realSync()

Copy link
Contributor

@honkar-jdk honkar-jdk left a comment

Choose a reason for hiding this comment

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

@alisenchung

The current fix works for the cases listed in the test but fails for the following scenarios. To make the test more stable it is better to reassign the static variable lastFrameState = Frame.NORMAL in examineStates after creating a new Frame here , so that each time a new case is run it sets lastFrameState to default state.

// CASE 1:
examineStates(new int[] {Frame.NORMAL, Frame.MAXIMIZED_BOTH, Frame.ICONIFIED});
// CASE 2:
examineStates(new int[] {Frame.NORMAL, Frame.ICONIFIED, Frame.MAXIMIZED_BOTH});

@prrace
Copy link
Contributor

prrace commented Jun 14, 2023

I don't like the idea of a delay in either of the ways discussed here.
And although I do think that its much better to wait for delivery of an event, I'm not sure I understand the windowState listener proposal.
How do you know in this code that the transition of
iconified-> normal will then followed by normal->maximised ?
Perhaps you'll wait for ever ?
But I don't know what the macOS native model is for all of this.
This sequence is kind of odd and I'm curious what happens on older macOS releases - Sergey touched on this but I don't see direct followup.
There seems to be assumptions it was like this previously but now it is slower ?

I suggest explicit verification of what happens on macOS 12.6.
Are we now getting two native events when there used to be only one ?

@alisenchung
Copy link
Contributor Author

I don't like the idea of a delay in either of the ways discussed here. And although I do think that its much better to wait for delivery of an event, I'm not sure I understand the windowState listener proposal. How do you know in this code that the transition of iconified-> normal will then followed by normal->maximised ?

The code here is trying to do a transition from iconified -> maximized, but splits it up into iconified -> normal and normal -> maximized.

@dmarkov20
Copy link
Member

dmarkov20 commented Jun 14, 2023

I don't like the idea of a delay in either of the ways discussed here. And although I do think that its much better to wait for delivery of an event, I'm not sure I understand the windowState listener proposal. How do you know in this code that the transition of iconified-> normal will then followed by normal->maximised ? Perhaps you'll wait for ever ? But I don't know what the macOS native model is for all of this. This sequence is kind of odd and I'm curious what happens on older macOS releases - Sergey touched on this but I don't see direct followup. There seems to be assumptions it was like this previously but now it is slower ?

I suggest explicit verification of what happens on macOS 12.6. Are we now getting two native events when there used to be only one ?

We cannot move a frame directly from ICONIFIED to MAXIMIZED_BOTH and vice versa. We need to go through the NORMAL state, (i.e. ICONIFIED -> NORMAL -> MAXIMIZED_BOTH). If my memory does not fail me we use that approach from the very beginning.

We perform two state-transitions subsequently and don't care of real state of the frame between them. It used to work till macOS 13 where the transition from one state to another takes a bit longer.
In other words we do ICONIFIED -> NORMAL and then immediately NORMAL -> MAXIMIZED_BOTH but the frame is NOT in NORMAL state yet.

@@ -965,6 +966,26 @@ public boolean isFullScreenMode() {
return isFullScreenMode;
}

private void waitForWindowState(int state) {
Copy link
Member

Choose a reason for hiding this comment

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

It would be wise to check the state of the peer in the beginning to avoid unnecessary usage of WindowStateListener and wait-notify mechanism if we are already in the state which is required.

@@ -965,6 +966,26 @@ public boolean isFullScreenMode() {
return isFullScreenMode;
}

private void waitForWindowState(int state) {
Object lock = new Object();
target.addWindowStateListener(new WindowStateListener() {
Copy link
Member

Choose a reason for hiding this comment

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

I think WindoweStateListener should be removed once we are done

@@ -45,7 +45,7 @@

public class MaximizedToIconified
{
static volatile int lastFrameState = Frame.NORMAL;
static volatile int lastFrameState;
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of test change? Can you elaborate, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dmarkov20 Although the test does not explicitly test the cases mentioned below, it fails for the following two cases because the lastFrameState is changed as we progress through the states and should be reassigned when testing a new set of states. @alisenchung is assigning this var within examineStates now. Though, I would prefer to keep the initial assignment as-is and have the reassignment in examineStates.

@alisenchung

The current fix works for the cases listed in the test but fails for the following scenarios. To make the test more stable it is better to reassign the static variable lastFrameState = Frame.NORMAL in examineStates after creating a new Frame here , so that each time a new case is run it sets lastFrameState to default state.

// CASE 1:
examineStates(new int[] {Frame.NORMAL, Frame.MAXIMIZED_BOTH, Frame.ICONIFIED});
// CASE 2:
examineStates(new int[] {Frame.NORMAL, Frame.ICONIFIED, Frame.MAXIMIZED_BOTH});

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see now. I would suggest including that cases into the test.

Copy link
Contributor

@honkar-jdk honkar-jdk left a comment

Choose a reason for hiding this comment

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

The update fix - with the current state check before acquiring the lock and removing the WindowStateListener suggested by @dmarkov20 avoids unnecessary acquiring of lock and looks to be a better solution that before.

} catch (InterruptedException ie) {}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: Remove extra line

@@ -965,6 +966,34 @@ public boolean isFullScreenMode() {
return isFullScreenMode;
}

private void waitForWindowState(int state) {
if(peer.getState() == state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a space after if

@@ -114,6 +116,7 @@ private static void doTest() {
// because Toolkit.isFrameStateSupported() method reports these states
// as not supported. And such states will simply be skipped.
examineStates(new int[] {Frame.MAXIMIZED_BOTH, Frame.ICONIFIED, Frame.NORMAL});
System.out.println("------");
examineStates(new int[] {Frame.ICONIFIED, Frame.MAXIMIZED_BOTH, Frame.NORMAL});

Copy link
Contributor

Choose a reason for hiding this comment

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

@alisenchung Now that the lastFrameState is fixed, I would suggest adding the cases mentioned here too, to make the test comprehensive.

Copy link
Member

@dmarkov20 dmarkov20 left a comment

Choose a reason for hiding this comment

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

@alisenchung The fix looks OK. Did you test it on macOS 12? If not, can you ensure that previous macOS version(s) works fine with the proposed changes?

@alisenchung
Copy link
Contributor Author

@alisenchung The fix looks OK. Did you test it on macOS 12? If not, can you ensure that previous macOS version(s) works fine with the proposed changes?

I've tested on macOS12 and it works fine with the changes.

@openjdk
Copy link

openjdk bot commented Jun 28, 2023

@alisenchung 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:

8296972: [macos13] java/awt/Frame/MaximizedToIconified/MaximizedToIconified.java: getExtendedState() != 6 as expected.

Reviewed-by: dmarkov, honkar

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 459 new commits pushed to the master branch:

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 28, 2023
@alisenchung
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Jun 28, 2023

Going to push as commit f0c2f09.
Since your change was applied there have been 460 commits pushed to the master branch:

  • 9f46fc2: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
  • 7fffdb5: 8310405: Linker.Option.firstVariadicArg should specify which index values are valid
  • b6c789f: 8309140: ResourceHashtable failed "assert(~(_allocation_t[0] | allocation_mask) == (uintptr_t)this) failed: lost resource object"
  • 9f98136: 6956385: URLConnection.getLastModified() leaks file handles for jar:file and file: URLs
  • 46e4ee1: 8310974: NMT: Arena diffs miss the scale
  • f17bfee: 8311034: Fix typo in javac man page
  • 2ccdd29: 8299825: Move StdoutLog and StderrLog to LogConfiguration
  • e3f18af: 8311007: jdk/jfr/tool/TestView.java can't find event
  • 08c51f2: 8310920: Fix -Wconversion warnings in command line flags
  • c2e9485: 8310921: Fix -Wconversion warnings from GenerateOopMap
  • ... and 450 more: https://git.openjdk.org/jdk/compare/a92363461dbe67d8736a6b0c3cbe1c3ad7aa28ae...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 28, 2023
@openjdk openjdk bot closed this Jun 28, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 28, 2023
@openjdk
Copy link

openjdk bot commented Jun 28, 2023

@alisenchung Pushed as commit f0c2f09.

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

if (peer.getState() != state) {
synchronized (lock) {
try {
lock.wait();
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that waiting forever is a good thing? Can this request be ignored by the OS for any reason?

Copy link
Member

Choose a reason for hiding this comment

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

It is very unlikely that we are waiting forever here unless there are some serious problems with corresponding platform API.
I understand your concern and agree that waiting for some reasonable time instead of forever is more suitable here.
@alisenchung Can you open a bug to replace forever waiting with timeout.

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