-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
8339561: The test/jdk/java/awt/Paint/ListRepaint.java may fail after JDK-8327401 #20861
base: master
Are you sure you want to change the base?
Changes from 5 commits
94b798e
a026bd4
4c88385
b2907dc
2a56f4b
4321e29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,34 +25,43 @@ | |
* @test | ||
* @key headful | ||
* @summary To check proper WINDOW_EVENTS are triggered when Frame gains or losses the focus | ||
* @author Jitender(jitender.singh@eng.sun.com) area=AWT | ||
* @author yan | ||
* @library /lib/client | ||
* @build ExtendedRobot | ||
* @run main ActiveAWTWindowTest | ||
*/ | ||
|
||
import java.awt.*; | ||
import java.awt.event.*; | ||
import java.awt.BorderLayout; | ||
import java.awt.Button; | ||
import java.awt.Color; | ||
import java.awt.EventQueue; | ||
import java.awt.FlowLayout; | ||
import java.awt.Frame; | ||
import java.awt.TextField; | ||
import java.awt.event.ActionEvent; | ||
import java.awt.event.ActionListener; | ||
import java.awt.event.InputEvent; | ||
import java.awt.event.WindowAdapter; | ||
import java.awt.event.WindowEvent; | ||
import java.awt.event.WindowFocusListener; | ||
|
||
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; | ||
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(() -> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test should wait until the frame will be disposed. |
||
if (test.frame != null) { | ||
test.frame.dispose(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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; | ||||||||||||
|
@@ -51,7 +50,7 @@ | |||||||||||
import jdk.test.lib.Platform; | ||||||||||||
|
||||||||||||
public class KeyEventsTest { | ||||||||||||
TestState currentState; | ||||||||||||
private volatile TestState currentState; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather not declare it a The value is assigned to At the same time, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||||||||
|
||||||||||||
|
@@ -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(); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 jdk/src/java.desktop/unix/classes/sun/awt/X11/XListPeer.java Lines 389 to 390 in 77468c2
Overall, it needs more investigation. It could be that the fix for JDK-6471693 needs revising. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
|
||||||||||||
r.waitForIdle(); | ||||||||||||
r.delay(500); | ||||||||||||
app.doTest(); | ||||||||||||
} finally { | ||||||||||||
EventQueue.invokeAndWait(() -> { | ||||||||||||
if (app.keyFrame != null) { | ||||||||||||
app.keyFrame.dispose(); | ||||||||||||
} | ||||||||||||
}); | ||||||||||||
if (app.keyFrame != null) { | ||||||||||||
app.keyFrame.dispose(); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
|
@@ -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(); | ||||||||||||
|
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 | ||
|
@@ -21,28 +21,35 @@ | |
* questions. | ||
*/ | ||
|
||
|
||
import java.awt.*; | ||
import java.awt.Button; | ||
import java.awt.EventQueue; | ||
import java.awt.Frame; | ||
import java.awt.Graphics; | ||
|
||
/** | ||
* @test | ||
* @key headful | ||
* @bug 7090424 | ||
* @author Sergey Bylokhov | ||
*/ | ||
public final class ButtonRepaint extends Button { | ||
|
||
public static void main(final String[] args) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you expand imports, remove |
||
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(); | ||
} | ||
} | ||
} | ||
} | ||
|
||
|
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 | ||
|
@@ -21,27 +21,35 @@ | |
* questions. | ||
*/ | ||
|
||
import java.awt.*; | ||
import java.awt.Checkbox; | ||
import java.awt.EventQueue; | ||
import java.awt.Frame; | ||
import java.awt.Graphics; | ||
|
||
/** | ||
* @test | ||
* @key headful | ||
* @bug 7090424 | ||
* @author Sergey Bylokhov | ||
*/ | ||
public final class CheckboxRepaint extends Checkbox { | ||
|
||
public static void main(final String[] args) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you expand imports, remove |
||
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(); | ||
} | ||
} | ||
} | ||
} | ||
|
||
|
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 | ||
|
@@ -30,21 +30,26 @@ | |
* @test | ||
* @key headful | ||
* @bug 7090424 | ||
* @author Sergey Bylokhov | ||
*/ | ||
public final class LabelRepaint extends Label { | ||
|
||
public static void main(final String[] args) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you remove |
||
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(); | ||
} | ||
} | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.