JDK-8320675 : PrinterJob/SecurityDialogTest.java hangs #18299

Renjithkannath · 2024-03-14T11:52:09Z

Hi Reviewers,

I have updated the test with PassFailJFrame with information panel, earlier this was informed through terminal.
Please review and let me know your suggestions if any.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8320675: PrinterJob/SecurityDialogTest.java hangs (Bug - P4)

Reviewers

Alexey Ivanov (@aivanov-jdk - Reviewer)
Tejesh R (@TejeshR13 - Committer) ⚠️ Review applies to 6e80eaa0

Contributors

Alexey Ivanov <aivanov@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18299

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

Using diff file

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

Webrev

Link to Webrev Comment

Rearrange code; Really show native Page Dialog; Get rid of vertical glue, inline the title; Use JLabel to display current dialog tested; Create the label in a thread-safe way; Rename msg to dialogType; Use thread-safe UI updates. Remove unused imports Remove trailing whitespace

bridgekeeper · 2024-03-14T11:52:48Z

👋 Welcome back rkannathpari! 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-03-14T11:52:54Z

@Renjithkannath 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:

8320675: PrinterJob/SecurityDialogTest.java hangs

Co-authored-by: Alexey Ivanov <aivanov@openjdk.org>
Reviewed-by: aivanov, tr

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

dea94f4: 8328300: Convert PrintDialogsTest.java from Applet to main program
d32746e: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs
000f4d8: 8319251: [REDO] Change LockingMode default from LM_LEGACY to LM_LIGHTWEIGHT
fbeac98: 8328379: Convert URLDragTest.html applet test to main
96530bc: 8326521: JFR: CompilerPhase event test fails on windows 32 bit
a112fc8: 8328225: Convert ImageDecoratedDnD.html applet test to main
4e83f4c: 8328185: Convert java/awt/image/MemoryLeakTest/MemoryLeakTest.java applet test to main
d3f3011: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream
e5e7cd2: 8328386: Convert java/awt/FileDialog/FileNameOverrideTest test to main
1b68c73: 8328377: Convert java/awt/Cursor/MultiResolutionCursorTest test to main
... and 124 more: https://git.openjdk.org/jdk/compare/49d8008947534898e2ba36e2d81d87d6ae4b4c02...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@aivanov-jdk, @TejeshR13) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-03-14T11:53:13Z

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

mlbridge · 2024-03-14T11:55:48Z

Webrevs

Renjithkannath · 2024-03-14T12:39:43Z

/contributor add aivanov-jdk

openjdk · 2024-03-14T12:39:59Z

@Renjithkannath aivanov-jdk was not found in the census.

Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

Renjithkannath · 2024-03-14T12:41:04Z

/contributor add @aivanov-jdk

openjdk · 2024-03-14T12:41:17Z

@Renjithkannath
Contributor Alexey Ivanov <aivanov@openjdk.org> successfully added.

aivanov-jdk · 2024-03-14T13:34:06Z

/reviewers 2

openjdk · 2024-03-14T13:34:24Z

@aivanov-jdk
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

TejeshR13 · 2024-03-15T03:31:32Z

test/jdk/java/awt/print/PrinterJob/SecurityDialogTest.java

 */
+public class SecurityDialogTest extends Frame {
+    private static final String INSTRUCTIONS =


INSTRUCTIONS can be moved inside main.

Most of the test with PassFailJFrame instruction stored as string inside class, what additional benefit will get keeping it inside Main ?

When it can become local variable then why to keep it in class? Not like significant benefit maybe, but yeah just to keep its scope locally when it can be.

INSTRUCTIONS can be moved inside main.

No. Well, it can but what's the point?

I find a value in having the instructions as a dedicated constant at the top of the file. When you open the test, the instructions will be the first thing you see, and it's what you need after reading the summary in @summary tag above the class declaration.

TejeshR13 · 2024-03-15T03:33:24Z

test/jdk/java/awt/print/PrinterJob/SecurityDialogTest.java

 * @test
 * @bug 4937672 5100706 6252456
 * @key printer
- * @run main/othervm/manual -Djava.security.manager=allow SecurityDialogTest
+ * @library /java/awt/regtesthelpers
+ * @build PassFailJFrame


Any reason why test summary is not there?

It was not part of original test case, I have updated.

TejeshR13 · 2024-03-15T03:33:50Z

test/jdk/java/awt/print/PrinterJob/SecurityDialogTest.java

- * @run main/othervm/manual -Djava.security.manager=allow SecurityDialogTest
+ * @library /java/awt/regtesthelpers
+ * @build PassFailJFrame
+ * @run main/manual/othervm -Djava.security.manager=allow SecurityDialogTest
 */


Line spacing here would be nice.

@TejeshR13 What do you mean? What kind of line spacing would be nice here?

Line spacing between JTREG tag and class.

It's not needed… but most people add it. And we cannot agree on a common style, unfortunately.

These jtreg tags are akin to javadoc comment (but they're not javadoc comment, and javadoc syntax /** with double-asterisk should not be used), and in most cases the javadoc comment is not separated with a blank line from its class.

The last line of the jtreg tags comment is nearly empty by itself.

TejeshR13 · 2024-03-15T03:43:13Z

test/jdk/java/awt/print/PrinterJob/SecurityDialogTest.java

-            "to be disabled if there is no read/write file permission.",
-            "You should test this by trying different policy files."
-          };
+        PassFailJFrame passFailJFrame = new PassFailJFrame.Builder()


I am getting an error for using new PassFailJFrame passFailJFrame = new PassFailJFrame.Builder(), stating "'Builder()' has private access in 'PassFailJFrame.Builder'" Probably because I have fetched latest PassFailJFrame.
And also you can use PassFailJFrame.builder() method.

For me its building and running properly

Ok, might be an issue from my side. Will look into it.

I am getting an error for using new PassFailJFrame passFailJFrame = new PassFailJFrame.Builder(), stating "'Builder()' has private access in 'PassFailJFrame.Builder'". Probably because I have fetched latest PassFailJFrame.

@TejeshR13 You're absolutely right. Thanks for catching it.

And also you can use PassFailJFrame.builder() method.

Not can but must.

The test won't work after it's integrated, it won't compile.

aivanov-jdk · 2024-03-15T10:37:16Z

test/jdk/java/awt/print/PrinterJob/SecurityDialogTest.java

 */
+public class SecurityDialogTest extends Frame {


Suggested change

public class SecurityDialogTest extends Frame {

public class SecurityDialogTest {

It does not need to extend Frame.

aivanov-jdk · 2024-03-15T10:39:39Z

test/jdk/java/awt/print/PrinterJob/SecurityDialogTest.java

 */
+public class SecurityDialogTest extends Frame {
+    private static final String INSTRUCTIONS =


INSTRUCTIONS can be moved inside main.

No. Well, it can but what's the point?

I find a value in having the instructions as a dedicated constant at the top of the file. When you open the test, the instructions will be the first thing you see, and it's what you need after reading the summary in @summary tag above the class declaration.

aivanov-jdk · 2024-03-15T10:42:07Z

test/jdk/java/awt/print/PrinterJob/SecurityDialogTest.java

-            "to be disabled if there is no read/write file permission.",
-            "You should test this by trying different policy files."
-          };
+        PassFailJFrame passFailJFrame = new PassFailJFrame.Builder()


I am getting an error for using new PassFailJFrame passFailJFrame = new PassFailJFrame.Builder(), stating "'Builder()' has private access in 'PassFailJFrame.Builder'". Probably because I have fetched latest PassFailJFrame.

@TejeshR13 You're absolutely right. Thanks for catching it.

And also you can use PassFailJFrame.builder() method.

Not can but must.

The test won't work after it's integrated, it won't compile.

aivanov-jdk · 2024-03-15T10:47:42Z

INSTRUCTIONS can be moved inside main.

No. Well, it can but what's the point?

IDE never suggests converting constants to local variables. It is because constants can be inlined by Java compiler.

Static or instance fields as well as local variables are usually kept. Converting a field to local variable reduces the size of the object.

aivanov-jdk · 2024-03-15T15:19:56Z

test/jdk/java/awt/print/PrinterJob/SecurityDialogTest.java

+ * @summary tests native and cross-platform page and print dialog
+ *          when security manager is installed


Verifies "Print to file" option is disable if reading/writing files is not allowed by Security Manager.

I believe this type of summary explains better the purpose of the test, and the purpose is to ensure "Print to file" is disabled in writing to files is not allowed, according to the instructions of in the test.

aivanov-jdk · 2024-03-15T15:21:19Z

test/jdk/java/awt/print/PrinterJob/SecurityDialogTest.java

@@ -64,7 +66,7 @@ public static void main(String[] args) throws Exception {
            throw new RuntimeException("Printer not configured or available.");
        }

-        PassFailJFrame passFailJFrame = new PassFailJFrame.Builder()
+        PassFailJFrame passFailJFrame =  PassFailJFrame.builder()


Suggested change

PassFailJFrame passFailJFrame = PassFailJFrame.builder()

PassFailJFrame passFailJFrame = PassFailJFrame.builder()

Removed redundant double space.

Renjithkannath · 2024-03-21T03:41:24Z

/integrate

openjdk · 2024-03-21T03:42:54Z

@Renjithkannath
Your change (at version cc6d560) is now ready to be sponsored by a Committer.

TejeshR13 · 2024-03-21T10:27:22Z

/sponsor

openjdk · 2024-03-21T10:29:20Z

Going to push as commit 9f5ad43.
Since your change was applied there have been 139 commits pushed to the master branch:

684678f: 8328633: s390x: Improve vectorization of Match.sqrt() on floats
93d1700: 8328589: unify os::breakpoint among posix platforms
a5a2eca: 8327683: Crash with primitive type pattern and generic expression in instanceof
65d9f11: 8328328: Convert javax/swing/JTabbedPane/4666224/bug4666224.java applet test to main
481473e: 8328035: Convert javax/swing/text/html/TableView/7030332/bug7030332.java applet test to main
dea94f4: 8328300: Convert PrintDialogsTest.java from Applet to main program
d32746e: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs
000f4d8: 8319251: [REDO] Change LockingMode default from LM_LEGACY to LM_LIGHTWEIGHT
fbeac98: 8328379: Convert URLDragTest.html applet test to main
96530bc: 8326521: JFR: CompilerPhase event test fails on windows 32 bit
... and 129 more: https://git.openjdk.org/jdk/compare/49d8008947534898e2ba36e2d81d87d6ae4b4c02...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-21T10:29:28Z

@TejeshR13 @Renjithkannath Pushed as commit 9f5ad43.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Renjithkannath and others added 7 commits March 13, 2024 16:27

JDK-8320675 : PrinterJob/SecurityDialogTest.java hangs

fb1b47b

Update instructions

da70b5d

Move printing services after Security Manager is set

c56e8a7

Use new PageFormat() for pageDialog

261d507

Updated with message

a391764

Removed extra space before instruction lines

ede65c0

openjdk bot added the rfr Pull request is ready for review label Mar 14, 2024

openjdk bot added the client client-libs-dev@openjdk.org label Mar 14, 2024

aivanov-jdk approved these changes Mar 14, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 14, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Mar 14, 2024

TejeshR13 reviewed Mar 15, 2024

View reviewed changes

Added summary

6e80eaa

TejeshR13 approved these changes Mar 15, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 15, 2024

aivanov-jdk suggested changes Mar 15, 2024

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Mar 15, 2024

Updated PassFailJFrame object access

777a237

Removed frame

b95f902

aivanov-jdk suggested changes Mar 15, 2024

View reviewed changes

Removed extra space

cc6d560

aivanov-jdk approved these changes Mar 20, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 20, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 21, 2024

openjdk bot added the integrated Pull request has been integrated label Mar 21, 2024

openjdk bot closed this Mar 21, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 21, 2024

	public class SecurityDialogTest extends Frame {
	public class SecurityDialogTest {

		* @summary tests native and cross-platform page and print dialog
		* when security manager is installed

	PassFailJFrame passFailJFrame = PassFailJFrame.builder()
	PassFailJFrame passFailJFrame = PassFailJFrame.builder()

JDK-8320675 : PrinterJob/SecurityDialogTest.java hangs #18299

JDK-8320675 : PrinterJob/SecurityDialogTest.java hangs #18299

Conversation

Renjithkannath commented Mar 14, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

Webrev

bridgekeeper bot commented Mar 14, 2024

openjdk bot commented Mar 14, 2024 • edited Loading

openjdk bot commented Mar 14, 2024

mlbridge bot commented Mar 14, 2024 • edited Loading

Webrevs

Renjithkannath commented Mar 14, 2024

openjdk bot commented Mar 14, 2024

Renjithkannath commented Mar 14, 2024

openjdk bot commented Mar 14, 2024

aivanov-jdk commented Mar 14, 2024

openjdk bot commented Mar 14, 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

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

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 commented Mar 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Renjithkannath commented Mar 21, 2024

openjdk bot commented Mar 21, 2024

TejeshR13 commented Mar 21, 2024

openjdk bot commented Mar 21, 2024

openjdk bot commented Mar 21, 2024

Renjithkannath commented Mar 14, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 14, 2024 •

edited

Loading

mlbridge bot commented Mar 14, 2024 •

edited

Loading