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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+177 −147
Open

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
94b798e
8339561: The test/jdk/java/awt/Paint/ListRepaint.java may fail after …
mrserb Sep 4, 2024
a026bd4
add try/finally
mrserb Sep 5, 2024
4c88385
Update ProblemList.txt
mrserb Sep 9, 2024
b2907dc
PR review part 1
mrserb Sep 10, 2024
2a56f4b
Update bug4490179.java
mrserb Sep 10, 2024
4321e29
Merge branch 'openjdk:master' into JDK-8339561
mrserb Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions test/jdk/ProblemList.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ java/awt/Focus/WrongKeyTypedConsumedTest/WrongKeyTypedConsumedTest.java 8169096
java/awt/EventQueue/6980209/bug6980209.java 8198615 macosx-all
java/awt/grab/EmbeddedFrameTest1/EmbeddedFrameTest1.java 7080150 macosx-all
java/awt/event/InputEvent/EventWhenTest/EventWhenTest.java 8168646 generic-all
java/awt/List/KeyEventsTest/KeyEventsTest.java 8201307 linux-all
java/awt/Paint/ListRepaint.java 8201307 linux-all
java/awt/Mixing/AWT_Mixing/HierarchyBoundsListenerMixingTest.java 8049405 macosx-all
java/awt/Mixing/AWT_Mixing/OpaqueOverlapping.java 8294264 windows-x64
java/awt/Mixing/AWT_Mixing/OpaqueOverlappingChoice.java 8048171 generic-all
Expand Down
10 changes: 5 additions & 5 deletions test/jdk/java/awt/Frame/MiscUndecorated/ActiveAWTWindowTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,22 +37,22 @@

public class ActiveAWTWindowTest {

private Frame frame, frame2;
private Button button, button2;
private TextField textField, textField2;
private volatile Frame frame, frame2;
private volatile Button button, button2;
private volatile TextField textField, textField2;
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

private volatile int eventType;
private final Object lock1 = new Object();
private final Object lock2 = new Object();
private final Object lock3 = new Object();
private boolean passed = true;
private final int delay = 150;

public static void main(String[] args) {
public static void main(String[] args) throws Exception {
ActiveAWTWindowTest test = new ActiveAWTWindowTest();
try {
test.doTest();
} finally {
EventQueue.invokeLater(() -> {
EventQueue.invokeAndWait(() -> {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

if (test.frame != null) {
test.frame.dispose();
}
Expand Down
98 changes: 47 additions & 51 deletions test/jdk/java/awt/List/KeyEventsTest/KeyEventsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,8 @@
*/

import java.awt.BorderLayout;
import java.awt.EventQueue;
import java.awt.KeyboardFocusManager;
import java.awt.Frame;
import java.awt.KeyboardFocusManager;
import java.awt.List;
import java.awt.Panel;
import java.awt.Point;
Expand All @@ -51,7 +50,7 @@
import jdk.test.lib.Platform;

public class KeyEventsTest {
TestState currentState;
private volatile TestState currentState;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

final Object LOCK = new Object();
final int ACTION_TIMEOUT = 500;

Expand All @@ -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();
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@mrserb mrserb Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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,

r.waitForIdle();
r.delay(500);
app.doTest();
} finally {
EventQueue.invokeAndWait(() -> {
if (app.keyFrame != null) {
app.keyFrame.dispose();
}
});
if (app.keyFrame != null) {
app.keyFrame.dispose();
}
}
}

Expand Down Expand Up @@ -184,52 +181,51 @@ private void test(TestState currentState) throws Exception {
throw new RuntimeException("Test failed - list isn't focus owner.");
}

EventQueue.invokeAndWait(() -> {
list.deselect(0);
list.deselect(1);
list.deselect(2);
list.deselect(3);
list.deselect(4);
list.deselect(5);
list.deselect(6);
list.deselect(7);
list.deselect(8);

int selectIndex = 0;
int visibleIndex = 0;

if (currentState.getScrollMoved()) {
if (currentState.getKeyID() == KeyEvent.VK_PAGE_UP ||
currentState.getKeyID() == KeyEvent.VK_HOME) {
selectIndex = 8;
visibleIndex = 8;
} else if (currentState.getKeyID() == KeyEvent.VK_PAGE_DOWN ||
currentState.getKeyID() == KeyEvent.VK_END) {
list.deselect(0);
list.deselect(1);
list.deselect(2);
list.deselect(3);
list.deselect(4);
list.deselect(5);
list.deselect(6);
list.deselect(7);
list.deselect(8);

int selectIndex = 0;
int visibleIndex = 0;

if (currentState.getScrollMoved()) {
if (currentState.getKeyID() == KeyEvent.VK_PAGE_UP ||
currentState.getKeyID() == KeyEvent.VK_HOME) {
selectIndex = 8;
visibleIndex = 8;
} else if (currentState.getKeyID() == KeyEvent.VK_PAGE_DOWN ||
currentState.getKeyID() == KeyEvent.VK_END) {
selectIndex = 0;
visibleIndex = 0;
}
} else {
if (currentState.getKeyID() == KeyEvent.VK_PAGE_UP ||
currentState.getKeyID() == KeyEvent.VK_HOME) {
if (currentState.getSelectedMoved()) {
selectIndex = 1;
} else {
selectIndex = 0;
visibleIndex = 0;
}
} else {
if (currentState.getKeyID() == KeyEvent.VK_PAGE_UP ||
currentState.getKeyID() == KeyEvent.VK_HOME) {
if (currentState.getSelectedMoved()) {
selectIndex = 1;
} else {
selectIndex = 0;
}
visibleIndex = 0;
} else if (currentState.getKeyID() == KeyEvent.VK_PAGE_DOWN ||
currentState.getKeyID() == KeyEvent.VK_END) {
if (currentState.getSelectedMoved()) {
selectIndex = 7;
} else {
selectIndex = 8;
}
visibleIndex = 8;
visibleIndex = 0;
} else if (currentState.getKeyID() == KeyEvent.VK_PAGE_DOWN ||
currentState.getKeyID() == KeyEvent.VK_END) {
if (currentState.getSelectedMoved()) {
selectIndex = 7;
} else {
selectIndex = 8;
}
visibleIndex = 8;
}
list.select(selectIndex);
list.makeVisible(visibleIndex);
});
}
list.select(selectIndex);
list.makeVisible(visibleIndex);


r.delay(10);
r.waitForIdle();
Expand Down
26 changes: 16 additions & 10 deletions test/jdk/java/awt/Paint/ButtonRepaint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2013, 2016, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2013, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -34,15 +34,21 @@ public final class ButtonRepaint extends Button {

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

@aivanov-jdk aivanov-jdk Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

for (int i = 0; i < 10; ++i) {
final Frame frame = new Frame();
frame.setSize(300, 300);
frame.setLocationRelativeTo(null);
ButtonRepaint button = new ButtonRepaint();
frame.add(button);
frame.setVisible(true);
sleep();
button.test();
frame.dispose();
Frame frame = null;
try {
frame = new Frame();
frame.setSize(300, 300);
frame.setLocationRelativeTo(null);
ButtonRepaint button = new ButtonRepaint();
frame.add(button);
frame.setVisible(true);
sleep();
button.test();
} finally {
if (frame != null) {
frame.dispose();
}
}
}
}

Expand Down
26 changes: 16 additions & 10 deletions test/jdk/java/awt/Paint/CheckboxRepaint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2013, 2016, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2013, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -33,15 +33,21 @@ public final class CheckboxRepaint extends Checkbox {

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

@aivanov-jdk aivanov-jdk Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

for (int i = 0; i < 10; ++i) {
final Frame frame = new Frame();
frame.setSize(300, 300);
frame.setLocationRelativeTo(null);
CheckboxRepaint checkbox = new CheckboxRepaint();
frame.add(checkbox);
frame.setVisible(true);
sleep();
checkbox.test();
frame.dispose();
Frame frame = null;
try {
frame = new Frame();
frame.setSize(300, 300);
frame.setLocationRelativeTo(null);
CheckboxRepaint checkbox = new CheckboxRepaint();
frame.add(checkbox);
frame.setVisible(true);
sleep();
checkbox.test();
} finally {
if (frame != null) {
frame.dispose();
}
}
}
}

Expand Down
24 changes: 15 additions & 9 deletions test/jdk/java/awt/Paint/LabelRepaint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,21 @@ public final class LabelRepaint extends Label {

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

Choose a reason for hiding this comment

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

Could you remove @author tag, update copyright year?

for (int i = 0; i < 10; ++i) {
final Frame frame = new Frame();
frame.setSize(300, 300);
frame.setLocationRelativeTo(null);
LabelRepaint label = new LabelRepaint();
frame.add(label);
frame.setVisible(true);
sleep();
label.test();
frame.dispose();
Frame frame = null;
try {
frame = new Frame();
frame.setSize(300, 300);
frame.setLocationRelativeTo(null);
LabelRepaint label = new LabelRepaint();
frame.add(label);
frame.setVisible(true);
sleep();
label.test();
} finally {
if (frame != null) {
frame.dispose();
}
}
}
}

Expand Down
45 changes: 17 additions & 28 deletions test/jdk/java/awt/Paint/ListRepaint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,22 +34,27 @@
*/
public final class ListRepaint extends List {
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove @author tag and the second * from /**?


static ListRepaint listRepaint;
static Frame frame;

public static void main(final String[] args) throws Exception {
public static void main(final String[] args) {
for (int i = 0; i < 10; ++i) {
Frame frame = null;
try {
EventQueue.invokeLater(ListRepaint::createAndShowGUI);
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

frame = new Frame();
frame.setSize(300, 300);
frame.setLocationRelativeTo(null);
ListRepaint list = new ListRepaint();
list.add("1");
list.add("2");
list.add("3");
list.add("4");
list.select(0);
frame.add(list);
frame.setVisible(true);
sleep();
EventQueue.invokeAndWait(listRepaint::test);
list.test();
} finally {
EventQueue.invokeAndWait(() -> {
if (frame != null) {
frame.dispose();
frame = null;
}
});
if (frame != null) {
frame.dispose();
}
}
}
}
Expand All @@ -61,22 +66,6 @@ private static void sleep() {
}
}

static void createAndShowGUI() {
frame = new Frame();
frame.setSize(300, 300);
frame.setLocationRelativeTo(null);

listRepaint = new ListRepaint();
listRepaint.add("1");
listRepaint.add("2");
listRepaint.add("3");
listRepaint.add("4");
listRepaint.select(0);

frame.add(listRepaint);
frame.setVisible(true);
}

@Override
public void paint(final Graphics g) {
super.paint(g);
Expand Down
14 changes: 8 additions & 6 deletions test/jdk/javax/swing/JButton/bug4490179.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@

import java.awt.Point;
import java.awt.Robot;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.InputEvent;

import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.SwingUtilities;
Expand All @@ -53,10 +56,8 @@ public static void main(String[] args) throws Exception {
frame = new JFrame("bug4490179");
Copy link
Member

Choose a reason for hiding this comment

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

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.

button = new JButton("Button");
frame.getContentPane().add(button);
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;
Copy link
Member

Choose a reason for hiding this comment

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

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");
           }

}
Copy link
Member

@aivanov-jdk aivanov-jdk Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.)

});
Expand All @@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

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

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?

boolean result = passed;
robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
robot.delay(500);

if (!passed) {
if (!result) {
Copy link
Member

Choose a reason for hiding this comment

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

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

throw new RuntimeException("Test Failed");
}
} finally {
Expand Down
6 changes: 5 additions & 1 deletion test/jdk/javax/swing/plaf/basic/BasicMenuUI/4983388/bug4983388.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,11 @@ public void run() {
robot.waitForIdle();
robot.delay(500);

SwingUtilities.invokeAndWait(() -> frame.dispose());
SwingUtilities.invokeAndWait(() -> {
if (frame != null) {
frame.dispose();
}
});

if (!bMenuSelected) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you also expand imports and remove @author tag?

throw new RuntimeException("shortcuts on menus do not work");
Expand Down