8339561: The test/jdk/java/awt/Paint/ListRepaint.java may fail after JDK-8327401 #20861

mrserb · 2024-09-04T20:50:38Z

Several tests modified by #19339 have been tweaked, see inline comments.

Notes:

We have a few XXXRepaint.java tests and in this patch, I updated all of them to follow the change added to the ListRepaint.java

@azvegint @aivanov-jdk please take a look.

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-8339561: The test/jdk/java/awt/Paint/ListRepaint.java may fail after JDK-8327401 (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20861

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

Using diff file

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

Webrev

Link to Webrev Comment

…JDK-8327401

bridgekeeper · 2024-09-04T20:58:52Z

👋 Welcome back serb! 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 · 2024-09-04T20:59:24Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2024-09-04T20:59:58Z

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

mrserb · 2024-09-09T06:25:13Z

test/jdk/java/awt/Frame/MiscUndecorated/ActiveAWTWindowTest.java

        ActiveAWTWindowTest test = new ActiveAWTWindowTest();
        try {
            test.doTest();
        } finally {
-            EventQueue.invokeLater(() -> {
+            EventQueue.invokeAndWait(() -> {


The test should wait until the frame will be disposed.
"invokeLater" was added here,

mrserb · 2024-09-09T06:27:26Z

test/jdk/java/awt/Frame/MiscUndecorated/ActiveAWTWindowTest.java

-    private TextField textField, textField2;
+    private volatile Frame frame, frame2;
+    private volatile Button button, button2;
+    private volatile TextField textField, textField2;


probably unnecessary but some of these fields might be used on different threads, so just in case.

Likely, it's unnecessary. I don't mind though.

I'd rather refactor the test to simplify it. Using CountDownLatch or other synchronisation primitives instead of wait/notify. I submitted JDK-8339791. Feel free to re-assign if you like.

mrserb · 2024-09-09T06:30:43Z

test/jdk/java/awt/List/KeyEventsTest/KeyEventsTest.java

@@ -66,16 +65,14 @@ public static void main(final String[] args) throws Exception {
        r = new Robot();
        KeyEventsTest app = new KeyEventsTest();
        try {
-            EventQueue.invokeAndWait(app::initAndShowGui);
+            app.initAndShowGui();


The test was changed here, the root cause of the failure discussed in https://github.com/openjdk/jdk/pull/19339/files#r1609189736 is a product bug: https://bugs.openjdk.org/browse/JDK-8201307.

Not sure if it's a product bug…

AWT components do not explicitly state whether they're thread-safe or not. It's more or less assumed they should be. At the same time, I don't see code which ensures all the cases are thread-safe.

As far as I know, clientlibs group has been treating AWT components just like Swing ones: for safety create, access and destroy AWT components on EDT only.

I haven't looked at the test code thoroughly. It looks it still needs more investigation.

At the same time, I find it weird that repaint paints directly in its code and then posts paint event.

jdk/src/java.desktop/unix/classes/sun/awt/X11/XListPeer.java

Lines 389 to 390 in 77468c2

painter.paint(g, firstItem, lastItem, options, source, distance);

postPaintEvent(target, 0, 0, getWidth(), getHeight());

Overall, it needs more investigation. It could be that the fix for JDK-6471693 needs revising.

At the same time, I find it weird that repaint paints directly in its code and then posts paint event.

This is one of the implementation details of the AWT, the "native" component should be painted before the paint event will proceed on EDT, and even if EDT is blocked the "native" part should be painted. Since the XAWT is implemented via Swing(or some custom "java peers") which are not thread-safe we should use some additional synchronization. On macOS we have a special lock for that

jdk/src/java.desktop/macosx/classes/sun/lwawt/LWListPeer.java

Line 87 in 559fc71

synchronized (getDelegateLock()) {

and

jdk/src/java.desktop/macosx/classes/sun/lwawt/LWComponentPeer.java

Line 361 in 559fc71

final Object getDelegateLock() {

but on XAWT it is quite "cumbersome" since it was implemented a long time ago.

And the NPE is triggered for List.select only because it is one method in the test that uses peer w/o synchronization.

jdk/src/java.desktop/share/classes/java/awt/List.java

Line 588 in 77468c2

// Bug #4059614: select can't be synchronized while calling the peer,

mrserb · 2024-09-09T06:36:44Z

test/jdk/java/awt/Paint/ListRepaint.java

            try {
-                EventQueue.invokeLater(ListRepaint::createAndShowGUI);


The purpose of the test is to run some methods of the AWT component on the main thread and check if it will be refreshed on the EDT. It was changed here. It is a product bug: https://bugs.openjdk.org/browse/JDK-8201307.

Yep, the previous update has changed the logic of the test so that it no longer reproduces the original problem for which it was written, namely JDK-7090424.

mrserb · 2024-09-09T06:38:44Z

test/jdk/javax/swing/JButton/bug4490179.java

@@ -81,10 +82,11 @@ public static void main(String[] args) throws Exception {
            robot.mousePress(InputEvent.BUTTON1_DOWN_MASK);
            robot.mousePress(InputEvent.BUTTON3_DOWN_MASK);
            robot.mouseRelease(InputEvent.BUTTON3_DOWN_MASK);
+            robot.delay(3000);


The test was discussed here. It seems that the purpose of the test is to check that no events will be generated while the left button is pressed, so we should check that "ActionListener" is not triggered and then release the mouse button.
Thoughts?

mlbridge · 2024-09-09T07:28:12Z

Webrevs

aivanov-jdk · 2024-09-09T18:56:07Z

test/jdk/java/awt/Frame/MiscUndecorated/ActiveAWTWindowTest.java

-    private TextField textField, textField2;
+    private volatile Frame frame, frame2;
+    private volatile Button button, button2;
+    private volatile TextField textField, textField2;


Likely, it's unnecessary. I don't mind though.

I'd rather refactor the test to simplify it. Using CountDownLatch or other synchronisation primitives instead of wait/notify. I submitted JDK-8339791. Feel free to re-assign if you like.

aivanov-jdk · 2024-09-09T19:30:35Z

test/jdk/java/awt/List/KeyEventsTest/KeyEventsTest.java

@@ -51,7 +50,7 @@
 import jdk.test.lib.Platform;

 public class KeyEventsTest {
-    TestState currentState;
+    private volatile TestState currentState;


I'd rather not declare it a volatile. It could give a false impression of being thread-safe but it is not. The volatile modifier has a meaning only when it's written and read subsequently. If the reference doesn't change, it has no effect on the visibility of the internal object state.

The value is assigned to currentState while holding a lock LOCK.

At the same time, currentState.setAction(true) is called without any synchronisation and adding volatile won't make the change of the state thread-safe.

Submitted JDK-8340196: j.a/List/KeyEventsTest/KeyEventsTest: TestState currentState is not thread-safe.

aivanov-jdk · 2024-09-09T19:32:04Z

test/jdk/java/awt/List/KeyEventsTest/KeyEventsTest.java

@@ -66,16 +65,14 @@ public static void main(final String[] args) throws Exception {
        r = new Robot();
        KeyEventsTest app = new KeyEventsTest();
        try {
-            EventQueue.invokeAndWait(app::initAndShowGui);
+            app.initAndShowGui();


Not sure if it's a product bug…

AWT components do not explicitly state whether they're thread-safe or not. It's more or less assumed they should be. At the same time, I don't see code which ensures all the cases are thread-safe.

As far as I know, clientlibs group has been treating AWT components just like Swing ones: for safety create, access and destroy AWT components on EDT only.

I haven't looked at the test code thoroughly. It looks it still needs more investigation.

At the same time, I find it weird that repaint paints directly in its code and then posts paint event.

jdk/src/java.desktop/unix/classes/sun/awt/X11/XListPeer.java

Lines 389 to 390 in 77468c2

painter.paint(g, firstItem, lastItem, options, source, distance);

postPaintEvent(target, 0, 0, getWidth(), getHeight());

Overall, it needs more investigation. It could be that the fix for JDK-6471693 needs revising.

aivanov-jdk · 2024-09-09T19:38:37Z

test/jdk/java/awt/Paint/ButtonRepaint.java

@@ -34,15 +34,21 @@ public final class ButtonRepaint extends Button {

    public static void main(final String[] args) {


Could expand imports, remove @author tag and the second * from /**?

aivanov-jdk · 2024-09-09T19:39:39Z

test/jdk/java/awt/Paint/CheckboxRepaint.java

@@ -33,15 +33,21 @@ public final class CheckboxRepaint extends Checkbox {

    public static void main(final String[] args) {


Could expand imports, remove @author tag and the second * from /**?

aivanov-jdk · 2024-09-09T20:02:56Z

test/jdk/javax/swing/JButton/bug4490179.java


-            if (!passed) {
+            if (!result) {


Perhaps, we should ensure that an ActionEvent gets delivered at this point. Is it expected or not?

aivanov-jdk · 2024-09-09T20:04:25Z

test/jdk/javax/swing/JButton/bug4490179.java

-                button.addActionListener(e -> {
-                    if ((e.getModifiers() & InputEvent.BUTTON1_MASK)
-                            != InputEvent.BUTTON1_MASK) {
-                        System.out.println("Status: Failed");
+                button.addActionListener(new ActionListener() {
+                    public void actionPerformed(ActionEvent e) {
                        passed = false;
                    }


The updated code here is correct; Alexander has restored the condition that was present in the original test which was closed-source and manual.

However, it may be not necessary. Instead, the test should fail immediately if ActionEvent contains anything but BUTTON1_DOWN_MASK. (Well, cleaning up robot state to release mouse buttons is a good thing to do.)

aivanov-jdk · 2024-09-09T20:12:41Z

test/jdk/javax/swing/JButton/bug4490179.java

-                            != InputEvent.BUTTON1_MASK) {
-                        System.out.println("Status: Failed");
+                button.addActionListener(new ActionListener() {
+                    public void actionPerformed(ActionEvent e) {
                        passed = false;


Maybe use two CountDownLatches:

if ((e.getModifiers() & InputEvent.BUTTON1_MASK) != InputEvent.BUTTON1_MASK) { wrongMouseButton.countDown(); } else { mouseButton1.countDown(); }

Then in main:

robot.mouseRelease(InputEvent.BUTTON3_DOWN_MASK); // robot.delay(3000); if (wrongMouseButton.await(2, TimeUnit.SECONDS)) { // Restore robot state robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK); throw new RuntimeException("ActionEvent delivered from mouse button 3"); } robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK); if (mouseButton1.await(2, TimeUnit.SECONDS)) { throw new RuntimeException("ActionEvent not delivered from mouse button 1"); }

aivanov-jdk · 2024-09-09T20:16:55Z

test/jdk/javax/swing/JButton/bug4490179.java

@@ -53,10 +56,8 @@ public static void main(String[] args) throws Exception {
                frame = new JFrame("bug4490179");


Instead of three volatile fields — pt, buttonW, buttonH — there could be only one:

SwingUtilities.invokeAndWait(() -> { Point loc = button.getLocationOnScreen(); Dimension size = button.getSize(); pt = new Point(loc.x + size.width / 2, loc.y + size.height / 2); });

I think this better conveys the idea, less variables / fields to track in your mind.

aivanov-jdk · 2024-09-09T20:18:48Z

test/jdk/javax/swing/plaf/basic/BasicMenuUI/4983388/bug4983388.java

+            if (frame != null) {
+                frame.dispose();
+            }
+        });

        if (!bMenuSelected) {


Could you also expand imports and remove @author tag?

aivanov-jdk · 2024-09-10T14:56:37Z

test/jdk/javax/swing/JButton/bug4490179.java

-            robot.delay(3000);
-            boolean result = passed;
-            robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
+            if (anyButton.await(3, TimeUnit.SECONDS)) {


Should 2 seconds be enough?

azvegint · 2024-09-10T16:47:34Z

test/jdk/javax/swing/JButton/bug4490179.java

            robot.waitForIdle();
            robot.mousePress(InputEvent.BUTTON3_DOWN_MASK);
            robot.mouseRelease(InputEvent.BUTTON3_DOWN_MASK);

            robot.mousePress(InputEvent.BUTTON1_DOWN_MASK);
            robot.mousePress(InputEvent.BUTTON3_DOWN_MASK);
            robot.mouseRelease(InputEvent.BUTTON3_DOWN_MASK);
-            robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
-            robot.delay(500);
+            if (anyButton.await(3, TimeUnit.SECONDS)) {


await is also throws the InterruptedException, so theoretically we can leave the mouse button pressed in this case.

await throws InterruptedException when the thread is interrupted; the main thread is never interrupted in the test, therefore InterruptedException is never thrown.

bridgekeeper · 2024-10-14T12:37:24Z

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

8339561: The test/jdk/java/awt/Paint/ListRepaint.java may fail after …

94b798e

…JDK-8327401

openjdk bot added the client client-libs-dev@openjdk.org label Sep 4, 2024

mrserb added 2 commits September 4, 2024 20:08

add try/finally

a026bd4

Update ProblemList.txt

4c88385

mrserb commented Sep 9, 2024

View reviewed changes

mrserb marked this pull request as ready for review September 9, 2024 07:23

openjdk bot added the rfr Pull request is ready for review label Sep 9, 2024

aivanov-jdk suggested changes Sep 9, 2024

View reviewed changes

mrserb added 2 commits September 9, 2024 23:08

PR review part 1

b2907dc

Update bug4490179.java

2a56f4b

aivanov-jdk reviewed Sep 10, 2024

View reviewed changes

azvegint reviewed Sep 10, 2024

View reviewed changes

aivanov-jdk mentioned this pull request Oct 11, 2024

8341978: Improve JButton/bug4490179.java #21474

Closed

3 tasks

Merge branch 'openjdk:master' into JDK-8339561

4321e29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8339561: The test/jdk/java/awt/Paint/ListRepaint.java may fail after JDK-8327401 #20861

8339561: The test/jdk/java/awt/Paint/ListRepaint.java may fail after JDK-8327401 #20861

mrserb commented Sep 4, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 4, 2024

openjdk bot commented Sep 4, 2024

openjdk bot commented Sep 4, 2024

mrserb Sep 9, 2024

mrserb Sep 9, 2024

aivanov-jdk Sep 9, 2024

mrserb Sep 9, 2024

aivanov-jdk Sep 9, 2024

mrserb Sep 9, 2024 •

edited

Loading

mrserb Sep 9, 2024

mrserb Sep 9, 2024

aivanov-jdk Sep 9, 2024

mrserb Sep 9, 2024

mlbridge bot commented Sep 9, 2024 •

edited

Loading

aivanov-jdk Sep 9, 2024

aivanov-jdk Sep 9, 2024

aivanov-jdk Sep 16, 2024

aivanov-jdk Sep 9, 2024

aivanov-jdk Sep 9, 2024

aivanov-jdk Sep 9, 2024

aivanov-jdk Sep 9, 2024

aivanov-jdk Sep 9, 2024 •

edited

Loading

aivanov-jdk Sep 9, 2024

aivanov-jdk Sep 9, 2024

aivanov-jdk Sep 9, 2024

aivanov-jdk Sep 10, 2024

azvegint Sep 10, 2024

aivanov-jdk Sep 11, 2024

bridgekeeper bot commented Oct 14, 2024

	painter.paint(g, firstItem, lastItem, options, source, distance);
	postPaintEvent(target, 0, 0, getWidth(), getHeight());

		@@ -34,15 +34,21 @@ public final class ButtonRepaint extends Button {

		public static void main(final String[] args) {

		@@ -33,15 +33,21 @@ public final class CheckboxRepaint extends Checkbox {

		public static void main(final String[] args) {

		@@ -53,10 +56,8 @@ public static void main(String[] args) throws Exception {
		frame = new JFrame("bug4490179");

8339561: The test/jdk/java/awt/Paint/ListRepaint.java may fail after JDK-8327401 #20861

Are you sure you want to change the base?

8339561: The test/jdk/java/awt/Paint/ListRepaint.java may fail after JDK-8327401 #20861

Conversation

mrserb commented Sep 4, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewing

Webrev

bridgekeeper bot commented Sep 4, 2024

openjdk bot commented Sep 4, 2024

openjdk bot commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrserb Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Sep 9, 2024 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aivanov-jdk Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridgekeeper bot commented Oct 14, 2024

mrserb commented Sep 4, 2024 •

edited by openjdk bot

Loading

mrserb Sep 9, 2024 •

edited

Loading

mlbridge bot commented Sep 9, 2024 •

edited

Loading

aivanov-jdk Sep 9, 2024 •

edited

Loading