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

8261689: javax/swing/JComponent/7154030/bug7154030.java still fails with "Exception: Failed to hide opaque button" #2790

Closed
wants to merge 3 commits into from

Conversation

@azuev-java
Copy link
Member

@azuev-java azuev-java commented Mar 2, 2021


Progress

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

Issue

  • JDK-8261689: javax/swing/JComponent/7154030/bug7154030.java still fails with "Exception: Failed to hide opaque button"

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2790/head:pull/2790
$ git checkout pull/2790

…ith "Exception: Failed to hide opaque button"

Slightly adjusted the sequence of the waiting plus set specific min/max
sizes on the component and frame. That should get rid of the incomplete
painting at the start. Also added the additional screen capture that
should show the state of the application after initialization and the
area that will be analyzed.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 2, 2021

👋 Welcome back kizune! 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
Copy link

@openjdk openjdk bot commented Mar 2, 2021

@azuev-java The following label will be automatically applied to this pull request:

  • swing

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.

@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Mar 2, 2021

I finally was able to get the failure on the test system and it seems like that was a result of the incomplete drawing of the frame at the time of screenshot taking. Here's the screenshot in full resolution - note the black part at the right part of the frame.
fullScreenInit
Setting min/max size of the frame and the content pane inside it fixes the issue so after couple of hundred of the runs i see no failures. Plus in the future should failure occur we will have better information in order to analyze the failure.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 2, 2021

Webrevs

Rectangle bounds = frame.getBounds();
Insets insets = frame.getInsets();
locx = bounds.x + insets.left;
locy = bounds.y + insets.top;
frw = bounds.width - insets.left - insets.right;
frh = bounds.height - insets.top - insets.bottom;

imageInit = robot.createScreenCapture(new Rectangle(locx, locy, frw, frh));
Copy link
Member

@azvegint azvegint Mar 2, 2021

Choose a reason for hiding this comment

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

Looks like the result of this screen capture is unused.

Copy link
Member Author

@azuev-java azuev-java Mar 2, 2021

Choose a reason for hiding this comment

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

Yes, accidentally copied line instead of moving it.

Copy link
Member

@aivanov-jdk aivanov-jdk left a comment

Should frame be declared as volatile? It's accessed on main thread in finally block.
In addition to it, frame.getBounds() and frame.getInsets() are called on main thread instead of EDT.

BufferedImage fullScreen = robot.createScreenCapture(screen);
Graphics g = fullScreen.getGraphics();
g.setColor(Color.RED);
g.drawRect(locx-1, locy-1, frw+1, frh+1);
Copy link
Member

@aivanov-jdk aivanov-jdk Mar 2, 2021

Choose a reason for hiding this comment

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

Suggested change
g.drawRect(locx-1, locy-1, frw+1, frh+1);
g.drawRect(locx - 1, locy - 1, frw + 1, frh + 1);

Copy link
Member Author

@azuev-java azuev-java Mar 2, 2021

Choose a reason for hiding this comment

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

Fixed.

desktop.setMinimumSize(new Dimension(300, 300));
desktop.setMaximumSize(new Dimension(300, 300));

frame.setContentPane(desktop);
frame.setSize(300, 300);
frame.setMinimumSize(new Dimension(350, 350));
frame.setMaximumSize(new Dimension(350, 350));
frame.pack();
Copy link
Member

@aivanov-jdk aivanov-jdk Mar 2, 2021

Choose a reason for hiding this comment

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

Wouldn't it make {{desktop}} larger than 300,300?

Copy link
Member Author

@azuev-java azuev-java Mar 2, 2021

Choose a reason for hiding this comment

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

It will but it forces the invalidation and repainting of the content so test does not fail.

Copy link
Member

@aivanov-jdk aivanov-jdk Mar 2, 2021

Choose a reason for hiding this comment

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

It will but it forces the invalidation and repainting of the content so test does not fail.

Got it! It's the intention to cause additional repaint.

@@ -115,6 +129,7 @@ public void run() {
if (Util.compareBufferedImages(imageInit, imageShow)) {
ImageIO.write(imageInit, "png", new File("imageInit.png"));
ImageIO.write(imageShow, "png", new File("imageShow.png"));
ImageIO.write(fullScreen, "png", new File("fullScreenInit.png"));
throw new Exception("Failed to show opaque button");
Copy link
Member

@aivanov-jdk aivanov-jdk Mar 2, 2021

Choose a reason for hiding this comment

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

I suggest moving saving the images into a new method.

I see the set of images is different each time. Probably it makes sense to save all the images which are non-null, what do you think?

Copy link
Member Author

@azuev-java azuev-java Mar 2, 2021

Choose a reason for hiding this comment

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

I don't think it is necessary. And since this is an existing test i'm trying not to change its overall behavior and having screenshots at other stages of the testing is not beneficial in the test debugging.

if (Util.compareBufferedImages(imageInit, imageShow)) {
ImageIO.write(imageInit, "png", new File("imageInit.png"));
ImageIO.write(imageShow, "png", new File("imageShow.png"));
ImageIO.write(fullScreen, "png", new File("fullScreenInit.png"));
throw new Exception("Failed to show non-opaque button");
}
Copy link
Member

@aivanov-jdk aivanov-jdk Mar 2, 2021

Choose a reason for hiding this comment

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

Does it make sense to move this block above before invokeAndWait to make the sequence of actions consistent?

Copy link
Member Author

@azuev-java azuev-java Mar 2, 2021

Choose a reason for hiding this comment

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

Well, functionally it will not make any difference, we are analyzing the screenshots and the only thing that matters is at which time screenshot was taken. The only reason we don't take all screenshots in the beginning and analyze them later is to save test run time if test runs early. At the end it is not critical anymore. And as i said this is an existing test so i'm trying not to change it unless it affects its functionality.

Copy link
Member

@aivanov-jdk aivanov-jdk Mar 2, 2021

Choose a reason for hiding this comment

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

Yes, I agree it makes no difference. Yet when you read the test code, the pattern — show or hide the button, take screenshot, analyze it — gets broken. This place is the only exception to the pattern; here the button is shown, screenshot is taken, then the button is hidden and only after hiding the button the screenshot is analyzed, which could give the wrong impression as if the screenshot after hiding the button is being analyzed here.

Copy link
Member Author

@azuev-java azuev-java Mar 3, 2021

Choose a reason for hiding this comment

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

Ok, i give up, you won - i'll fix it.

robot.waitForIdle(1000);
robot.delay(1000);
Copy link
Member

@mrserb mrserb Mar 2, 2021

Choose a reason for hiding this comment

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

I doubt that this is a test issue, 2 seconds was not enough to properly show the frame?

Copy link
Member Author

@azuev-java azuev-java Mar 2, 2021

Choose a reason for hiding this comment

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

The problem is that i tried to reproduce this behavior locally on Ubuntu running this test (and the simplified version of it) literally millions of times. The only time it fails is when it is run on mach5 and even then it fails very rarely - once per hundreds of runs. The changes here - especially forcing the resize - seems to eliminate the problem with mach5.

Copy link
Member

@mrserb mrserb Mar 3, 2021

Choose a reason for hiding this comment

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

Then it is a product bug, If the problem is unrelated to the performance and the root cause is in the wrong/broken final layout. Do we want to suggest the same thing if the user will get the same issue in their app?

Copy link
Member Author

@azuev-java azuev-java Mar 3, 2021

Choose a reason for hiding this comment

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

It might be a product bug. Or it might be something specific to the machines with mach5 system. Or to some specific machine having hardware issue (i noticed that in my case all failures happened on one specific Linux test system). That's the problem. I have a simplified test case that should check for that specific problem and i'm running a dedicated Linux host in my home office with the same OS version than problematic machine that does nothing but runs this test in interactive mode for days. Today only i ran this test over 700000 times - 0 failures. If i would have a way to at least semi-reliable reproduce the problem i would create a new bug and would debug and fix it. But i don't. And the changes i made is simply a way to let this test to run even on the problematic host because the problem that happens have nothing to do with the functionality related to the test itself.

@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Mar 2, 2021

Should frame be declared as volatile? It's accessed on main thread in finally block.
In addition to it, frame.getBounds() and frame.getInsets() are called on main thread instead of EDT.

I couldn't link this comment to the code because GitHub does not allow adding comments to unmodified lines. Do I understand correctly that you're for leaving it as is?

@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Mar 3, 2021

Should frame be declared as volatile? It's accessed on main thread in finally block.
In addition to it, frame.getBounds() and frame.getInsets() are called on main thread instead of EDT.

I couldn't link this comment to the code because GitHub does not allow adding comments to unmodified lines. Do I understand correctly that you're for leaving it as is?

Pretty much. Making frame volatile will not change the behavior, the assignment to it made in one place and it's on EDT inside invokeAndWait block, making it volatile will change nothing, it will be fully assigned by the time we leave EDT. And calling non-disruptive getters such getInsets or getBounds from main thread is not a problem either. By that time frame should be visible and in position - otherwise we will have much more interesting problems.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 3, 2021

@azuev-java 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:

8261689: javax/swing/JComponent/7154030/bug7154030.java still fails with "Exception: Failed to hide opaque button"

Reviewed-by: aivanov, azvegint

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

  • 0265ab6: 8262466: linux libsaproc/DwarfParser.cpp delete DwarfParser object in early return
  • c15801e: 8261142: AArch64: Incorrect instruction encoding when right-shifting vectors with shift amount equals to the element width
  • 044e2a2: 8183569: Assert the same limits are used in parse_xss and globals.hpp
  • 5de0f4b: 8260869: Test java/foreign/TestHandshake.java fails intermittently
  • c9097a6: 8262893: Enable more doclint checks in javadoc build
  • 40bdf52: 8262096: Vector API fails to work due to VectorShape initialization exception
  • 93ffe6a: 8262892: minor typo in implSpec comment
  • 4f4d0f5: 8261969: SNIHostName should check if the encoded hostname conform to RFC 3490
  • c92f3bc: 8262876: Shenandoah: Fix comments regarding VM_ShenandoahOperation inheritances
  • 20b9ba5: 8262875: doccheck: empty paragraphs, etc in java.base module
  • ... and 118 more: https://git.openjdk.java.net/jdk/compare/18188c2aa7d68f057d4e08d70944d5694dbe73a2...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 label Mar 3, 2021
BufferedImage fullScreen = robot.createScreenCapture(screen);
Graphics g = fullScreen.getGraphics();
g.setColor(Color.RED);
g.drawRect(locx - 1, locy - 1, frw + 1, frh + 1);
Copy link
Contributor

@prsadhuk prsadhuk Mar 4, 2021

Choose a reason for hiding this comment

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

I believe we need to call g.dispose to dispose this object which we normallydo after getGraphics() call.

Copy link
Member Author

@azuev-java azuev-java Mar 4, 2021

Choose a reason for hiding this comment

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

It is not necessary in such a short test, JVM will exit after test ends which will clear all the resources captured by this Graphics instance.

Copy link
Member

@mrserb mrserb Mar 4, 2021

Choose a reason for hiding this comment

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

BTW it will be closed/released only if the tests are run in the othervm mode, but jtreg tests might share the single VM for a group of tests.

@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Mar 5, 2021

/integrate

@openjdk openjdk bot closed this Mar 6, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 6, 2021

@azuev-java Since your change was applied there have been 175 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 5eb2091.

💡 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
5 participants