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

8273617: UninitializedDisplayModeChangeTest.java times out on macOS 12 #5647

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

Conversation

@prsadhuk
Copy link
Contributor

@prsadhuk prsadhuk commented Sep 23, 2021

java/awt/FullScreen/UninitializedDisplayModeChangeTest/UninitializedDisplayModeChangeTest.java is timing out every time on macOS 12 even though the test passed.
Looks like the the main process waits for child process created in the test forever, making it timeout after jtreg timeout of 2min.
Modified to wait for childprocess for 30secs.


Progress

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

Issue

  • JDK-8273617: UninitializedDisplayModeChangeTest.java times out on macOS 12

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5647

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 23, 2021

👋 Welcome back psadhukhan! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 23, 2021

@prsadhuk this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8273617
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 23, 2021

@prsadhuk 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.

Loading

@openjdk openjdk bot added client and removed merge-conflict labels Sep 23, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 23, 2021

Webrevs

Loading

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Sep 23, 2021

Mach5 Test job running for several iterations for all platforms (including macos 12 both x64 and aarch64) is green

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 23, 2021

It intentionally waits forever since if the child process is alive some bug has occurred. So you need to find why the child hangs.

Loading

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Sep 24, 2021

It intentionally waits forever since if the child process is alive some bug has occurred. So you need to find why the child hangs.

Can you point out which bugs you were referring to if we don't wait for ever? AFAICT, it was done that way in jdk6(JDK-6358034) because there was no provision for waitFor(timeout), it only has waitFor() API...If test cannot finish/change display in 30secs, then it should fail..

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 24, 2021

Are you sure that if it cannot change the display in two minutes it should pass? It looks like an important bug.

Loading

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Sep 25, 2021

The child process executing DisplayModeChanger class hangs when it is supposed to go into windowed mode in gd.setFullScreenWindow(null) call.
It seems in macos12, it hangs while executing CGCompleteDisplayConfiguration() in Java_sun_awt_CGraphicsDevice_nativeSetDisplayMode method in CGraphicsDevice.m

Basically, setFullScreenWindow() calls CGGraphicsDevice.java#setFullScreenWindow which calls JNI nativeSetDisplayMode which does CGBeginDisplayConfiguration, CGConfigureDisplayWithDisplayMode and CGCompleteDisplayConfiguration.

Now, DisplayModeChanger class calls setFullScreenWindow to go to fullscreen mode and then to windowed mode.
When going to fullscreen mode, it calls Java_sun_awt_CGraphicsDevice_nativeSetDisplayMode() with
displayId:724860118, w:1280, h:720, bpp:32, refrate 60 and CGCompleteDisplayConfiguration works ok

But when it is called for windowed mode, Java_sun_awt_CGraphicsDevice_nativeSetDisplayMode() is called with displayId:724860118, w:1920, h:1080, bpp:32, refrate 60 and CGCompleteDisplayConfiguration hangs.

For macos11, also the same code flow is followed with same values but CGCompleteDisplayConfiguration executes and the testcase works.
For macos10.15.7 also, same codeflow is followed where fullscreen mode is called with
displayId:731394562, w:640, h:480, bpp:32, refrate 60 and windowed mode with
displayId:731394562, w:1920, h:1080, bpp:32, refrate 60 and it works.
So, it seems to be some macos12 quirk.

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 25, 2021

So this is not a test issue, I guess you should report this bug to Apple using some standalone native example.

Loading

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Sep 28, 2021

Since CGCompleteDisplayConfiguration hangs if we ask the thread to wait, remove thread blocking allows the test to run on all CI macos version.

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 28, 2021

I am not sure that it is a good thing to add a workaround to the code just to pass the test. The appkit still will be locked and any call to it will hang as well.

You can achieve the same test result by commenting out the "gd.setFullScreenWindow(null)", no code no problems, but I do not think it is the right thing to do.

Loading

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Sep 29, 2021

Why it is considered a workaround? There are many instances where we use [ThreadUtilities performOnMainThreadWaiting:NO block:^()
and it seems CGCompleteDisplayConfiguration might have a tendancy to take time
https://github.com/jakehilborn/displayplacer/pull/72/files
https://github.com/jakehilborn/displayplacer/issues/28
so it might be good to place the call not in blocking thread

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 29, 2021

Why it is considered a workaround? There are many instances where we use [ThreadUtilities performOnMainThreadWaiting:NO block:^()

It is a workaround because it does not fix the hang, but just skips part of the functionality. Since appkit hangs any UI operations will be blocked, and the app become broken

Loading

@prrace
Copy link
Contributor

@prrace prrace commented Oct 13, 2021

If changing waiting to NO doesn't fix the hang, then test would still hang and time out wouldn't it ?
It would be nice to know what actually happens on the system with that change.
And if it does fix it .. why ? Could it be a deadlock ? But if so who's and where ?
If the problem is anything like that then a simple native test case may not show it.
But the child process that hangs and does all the display mode changing doesn't create a lot of threads.
And why does it hang and the (several) other tests we have that change display modes etc not hang ?
If you just directly (no jtreg, no parent process) run the child from the command line does it hang ?
Without your updates I mean ..

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 10, 2021

@prsadhuk 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!

Loading

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