8334509: Cancelling PageDialog does not return the same PageFormat object #19786

prsadhuk · 2024-06-19T05:34:55Z

On cancelling PageDialog, same PageFormat object should be returned which stopped working after JDK-8307160.
Fix is made to reinstate "doIt" flag removed in JDK-8307160 so that correct value is returned from PageDialog.show action..
An automated printing testcase is created since the issue was caught by manual test and so having another manual test run the risk of not being executed during CI testing..

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-8334509: Cancelling PageDialog does not return the same PageFormat object (Bug - P2)

Reviewers

Alexey Ivanov (@aivanov-jdk - Reviewer) ⚠️ Review applies to 2b298f89
Phil Race (@prrace - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19786

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-06-19T05:35:36Z

👋 Welcome back psadhukhan! 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-06-19T05:35:42Z

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

8334509: Cancelling PageDialog does not return the same PageFormat object

Reviewed-by: aivanov, prr

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:

8e1d2b0: 8334441: Mark tests in jdk_security_infra group as manual
93d9802: 8334715: [riscv] Mixed use of tab and whitespace in riscv.ad
c41293a: 8334695: Fix build failure without zgc after JDK-8333300
75bea28: 8333867: SHA3 performance can be improved
9f8de22: 8327793: Deprecate jstatd for removal
dbf5a9a: 8334706: [JVMCI] APX registers incorrectly exposed on AMD64
08ace27: 8332314: Add window size configuration option to JavaShellToolBuilder interface
711e723: 6967482: TAB-key does not work in JTables after selecting details-view in JFileChooser
d2bebff: 8327370: (ch) sun.nio.ch.Poller.register throws AssertionError
ed14906: 8333361: ubsan,test : libHeapMonitorTest.cpp:518:9: runtime error: null pointer passed as argument 2, which is declared to never be null
... and 118 more: https://git.openjdk.org/jdk/compare/3a01b47ac97714608356ce3faf797c37dc63e9af...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 · 2024-06-19T05:36:06Z

@prsadhuk 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-06-19T05:39:34Z

Webrevs

kumarabhi006 · 2024-06-19T06:18:01Z

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

+ * @test
+ * @bug 8334366
+ * @key printer
+ * @summary Verifies oriignal pageobject is returned unmodified


Suggested change

* @summary Verifies oriignal pageobject is returned unmodified

* @summary Verifies original pageobject is returned unmodified

kumarabhi006 · 2024-06-19T06:20:29Z

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

+        PageFormat oldFormat = new PageFormat();
+        Robot robot = new Robot();
+        Thread t1 = new Thread(() -> {
+            robot.delay(2000);


usually we keep a delay of 1000 ms, any specific reason for longer delay?

kumarabhi006 · 2024-06-19T06:26:40Z

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

+        boolean result = newFormat.equals(oldFormat);
+        if (result) {
+            System.out.println("Test Passed");
+        } else {
+            throw new RuntimeException("Original PageFormat not returned on cancelling PageDialog");


result variable may not be required as newFormat.equals(oldFormat) can be evaluated in if condition. I guess there is no need to log "Test Passed", we can check for the failure condition and throw the run time exception.

kumarabhi006 · 2024-06-19T06:49:03Z

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

+                robot.keyPress(KeyEvent.VK_TAB);
+                robot.keyRelease(KeyEvent.VK_TAB);
+                robot.waitForIdle();
+                robot.delay(500);


I think shorter delay of 100ms should be ok here and below as well.

It might be ok for statndalone test but jtreg execution is failing sometimes with shorter delay so kept that way..

TejeshR13

Have you ever noticed focus going out of dialog/frame when application starts in CI system? In this test, since you are directly using TAB to navigate till Cancel button, any changes that focus might not be on the Dialog on startup? I roughly remember a test where the focus was not on the Frame and we then setFocus or something was used......

aivanov-jdk · 2024-06-19T10:32:28Z

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

+            for (int i = 0; i < 8; i++) {
+                robot.keyPress(KeyEvent.VK_TAB);
+                robot.keyRelease(KeyEvent.VK_TAB);
+                robot.waitForIdle();
+                robot.delay(500);
+            }
+            robot.keyPress(KeyEvent.VK_ENTER);
+            robot.keyRelease(KeyEvent.VK_ENTER);
+            robot.waitForIdle();
+            robot.delay(500);
+        });


These key presses are sent to whatever window has focus… before the Page Dialog is shown and after it's hidden. Is pressing VK_ESCAPE not enough? Pressing Esc is the same as clicking Cancel button.

Yes, VK_ESCAPE worked..Modified to use that...Thanks..

aivanov-jdk · 2024-06-19T10:38:26Z

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

+        PageFormat newFormat = pj.pageDialog(oldFormat);
+        if (!newFormat.equals(oldFormat)) {


You should interrupt the t1 thread after pj.pageDialog returns to stop robot from sending keyPress and keyRelease after the dialog is hidden. You can even use Thread.sleep instead of Robot.delay so that interrupting the thread throws InterruptedException and its handler just exits. (Robot.delay catches InterruptedException and then restores the interrupted flag.)

I wonder if any AWT event is sent when the Page Dialog is shown on the screen, an event is more reliable and key presses won't be sent to an arbitrary window in the system.

I am not sure I understand this..I guess this thread execution will be a one-time activity so I guess we dont need to do any thread cleanup specially when the dialog is modal so it will only be cancelled (ie pageDialog will return) by VK_ESCAPE in the thread

You've resolved the problem.

In previous iteration, the keys were sent in a loop, which meant that the thread could continue to run even after the page dialog was closed.

Now the t1 thread presses VK_ESCAPE once and exits.

aivanov-jdk · 2024-06-19T10:39:35Z

src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp

@@ -690,6 +691,7 @@ Java_sun_awt_windows_WPageDialogPeer__1show(JNIEnv *env, jobject peer)
            }
            ::GlobalUnlock(setup.hDevMode);
        }
+        doIt = JNI_TRUE;


Another option would be to return JNI_TRUE here and to return JNI_FALSE in the end of the function.

Yes, it could have been done that way and my intiial fix in JBS is that only but I wanted to reinstate the flag as it is before JDK-8307160 which was working till now..

The only problem with the flag is that the data flow is not as clear.

I'm not insisting, it worked before and it will work in the future.

The way it was "before" is that we always returned the value of "doIt". Why not restore that for consistency ?

I believe that's what this PR is doing, it returns value of "doIt" at end, isn't it?

The way it was "before" is that we always returned the value of "doIt". Why not restore that for consistency ?

I think it's clearer with explicit JNI_FALSE even though it's inconsistent. You don't have to track in your mind what value doIt has.

I believe that's what this PR is doing, it returns value of "doIt" at end, isn't it?

Yes, it does now. You changed it after Phil had left his comment.

aivanov-jdk · 2024-06-19T10:42:04Z

src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp

@@ -690,6 +691,7 @@ Java_sun_awt_windows_WPageDialogPeer__1show(JNIEnv *env, jobject peer)
            }
            ::GlobalUnlock(setup.hDevMode);
        }
+        doIt = JNI_TRUE;
    }

    AwtDialog::CheckUninstallModalHook();


I wonder why the block of code at lines 697–709 is not needed if JNI_FALSE is returned as a result of an error condition.

No, it is not directly connected to the bug you're fixing, yet it doesn't look right to me.

We can have a followup bug on this I guess since it was existing before JDK-8307160 also..

All I wanted is to bring up the inconsistency so that a few people would take a look at it while reviewing this change.

It does look odd. Focus would need transferring in both cases I'd expect.
It goes back to the very beginning of open source JDK so I can't see a changeset to point to in order to explain it.
And I also can't find any bug reports that might be related - either one about adding it, or one about things not working because it is not always executed.

I looked at the history before what's available in Git. I looks this has always been this way. Yet it doesn't look right.

AwtDialog::CheckInstallModalHook() is called right before the page dialog is displayed by using ::PageSetupDlg. I expect AwtDialog::CheckUninstallModalHook() needs to be called after it.

Likely, the early returns (inside if (ret)) are very rare, if any of these has ever occurred at all.

I'll submit a bug to include calling AwtDialog::CheckUninstallModalHook() in error cleanup. The done label which were before JDK-8307160 should've been before the line which calls CheckUninstallModalHook.

prsadhuk · 2024-06-20T05:09:09Z

Have you ever noticed focus going out of dialog/frame when application starts in CI system? In this test, since you are directly using TAB to navigate till Cancel button, any changes that focus might not be on the Dialog on startup? I roughly remember a test where the focus was not on the Frame and we then setFocus or something was used......

It works on CI...Job link in JBS..

aivanov-jdk · 2024-06-20T10:39:48Z

src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp

@@ -690,6 +691,7 @@ Java_sun_awt_windows_WPageDialogPeer__1show(JNIEnv *env, jobject peer)
            }
            ::GlobalUnlock(setup.hDevMode);
        }
+        doIt = JNI_TRUE;


The only problem with the flag is that the data flow is not as clear.

I'm not insisting, it worked before and it will work in the future.

aivanov-jdk · 2024-06-20T10:41:40Z

src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp

@@ -690,6 +691,7 @@ Java_sun_awt_windows_WPageDialogPeer__1show(JNIEnv *env, jobject peer)
            }
            ::GlobalUnlock(setup.hDevMode);
        }
+        doIt = JNI_TRUE;
    }

    AwtDialog::CheckUninstallModalHook();


All I wanted is to bring up the inconsistency so that a few people would take a look at it while reviewing this change.

aivanov-jdk · 2024-06-20T10:46:23Z

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

+        PageFormat newFormat = pj.pageDialog(oldFormat);
+        if (!newFormat.equals(oldFormat)) {


You've resolved the problem.

In previous iteration, the keys were sent in a loop, which meant that the thread could continue to run even after the page dialog was closed.

Now the t1 thread presses VK_ESCAPE once and exits.

aivanov-jdk · 2024-06-20T10:47:32Z

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

+            robot.delay(2000);
+            robot.keyPress(KeyEvent.VK_ESCAPE);
+            robot.keyRelease(KeyEvent.VK_ESCAPE);
+            robot.waitForIdle();


I think waitForIdle is redundant here: the thread doesn't do anything after pressing VK_ESCAPE.

prsadhuk · 2024-06-21T18:02:09Z

/integrate

openjdk · 2024-06-21T18:02:59Z

Going to push as commit 689cee3.
Since your change was applied there have been 128 commits pushed to the master branch:

8e1d2b0: 8334441: Mark tests in jdk_security_infra group as manual
93d9802: 8334715: [riscv] Mixed use of tab and whitespace in riscv.ad
c41293a: 8334695: Fix build failure without zgc after JDK-8333300
75bea28: 8333867: SHA3 performance can be improved
9f8de22: 8327793: Deprecate jstatd for removal
dbf5a9a: 8334706: [JVMCI] APX registers incorrectly exposed on AMD64
08ace27: 8332314: Add window size configuration option to JavaShellToolBuilder interface
711e723: 6967482: TAB-key does not work in JTables after selecting details-view in JFileChooser
d2bebff: 8327370: (ch) sun.nio.ch.Poller.register throws AssertionError
ed14906: 8333361: ubsan,test : libHeapMonitorTest.cpp:518:9: runtime error: null pointer passed as argument 2, which is declared to never be null
... and 118 more: https://git.openjdk.org/jdk/compare/3a01b47ac97714608356ce3faf797c37dc63e9af...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-21T18:03:05Z

@prsadhuk Pushed as commit 689cee3.

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

aivanov-jdk · 2024-06-21T16:34:02Z

src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp

-            return JNI_FALSE;
+            return doIt;


I'd rather keep JNI_FALSE here — it's more explicit and therefore clearer. You don't have to keep track of the value of doIt flag while reading the code.

In fact, I'd prefer no doIt flag at all… yet it makes handling the code below if (ret) slightly harder.

aivanov-jdk · 2024-06-21T16:46:16Z

src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp

@@ -690,6 +691,7 @@ Java_sun_awt_windows_WPageDialogPeer__1show(JNIEnv *env, jobject peer)
            }
            ::GlobalUnlock(setup.hDevMode);
        }
+        doIt = JNI_TRUE;


The way it was "before" is that we always returned the value of "doIt". Why not restore that for consistency ?

I think it's clearer with explicit JNI_FALSE even though it's inconsistent. You don't have to track in your mind what value doIt has.

I believe that's what this PR is doing, it returns value of "doIt" at end, isn't it?

Yes, it does now. You changed it after Phil had left his comment.

aivanov-jdk · 2024-06-21T18:10:59Z

src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp

@@ -690,6 +691,7 @@ Java_sun_awt_windows_WPageDialogPeer__1show(JNIEnv *env, jobject peer)
            }
            ::GlobalUnlock(setup.hDevMode);
        }
+        doIt = JNI_TRUE;
    }

    AwtDialog::CheckUninstallModalHook();


I looked at the history before what's available in Git. I looks this has always been this way. Yet it doesn't look right.

AwtDialog::CheckInstallModalHook() is called right before the page dialog is displayed by using ::PageSetupDlg. I expect AwtDialog::CheckUninstallModalHook() needs to be called after it.

Likely, the early returns (inside if (ret)) are very rare, if any of these has ever occurred at all.

I'll submit a bug to include calling AwtDialog::CheckUninstallModalHook() in error cleanup. The done label which were before JDK-8307160 should've been before the line which calls CheckUninstallModalHook.

prsadhuk · 2024-06-24T06:04:34Z

/backport jdk23-dev

openjdk · 2024-06-24T06:05:36Z

@prsadhuk The target repository jdk23-dev is not a valid target for backports.
List of valid target repositories: openjdk/jdk, openjdk/jdk11u, openjdk/jdk11u-dev, openjdk/jdk17u, openjdk/jdk17u-dev, openjdk/jdk21u, openjdk/jdk21u-dev, openjdk/jdk22u, openjdk/jdk23u, openjdk/jdk7u, openjdk/jdk8u, openjdk/jdk8u-dev, openjdk/jfx, openjdk/jfx17u, openjdk/jfx21u, openjdk/jfx22u, openjdk/lilliput-jdk17u, openjdk/lilliput-jdk21u, openjdk/shenandoah-jdk21u, openjdk/shenandoah-jdk8u.
Supplying the organization/group prefix is optional.

prsadhuk · 2024-06-24T06:06:53Z

/backport openjdk/jdk23u

openjdk · 2024-06-24T06:08:10Z

@prsadhuk the backport was successfully created on the branch backport-prsadhuk-689cee3d-master in my personal fork of openjdk/jdk23u. To create a pull request with this backport targeting openjdk/jdk23u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 689cee3d from the openjdk/jdk repository.

The commit being backported was authored by Prasanta Sadhukhan on 21 Jun 2024 and was reviewed by Alexey Ivanov and Phil Race.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk23u:

$ git fetch https://github.com/openjdk-bots/jdk23u.git backport-prsadhuk-689cee3d-master:backport-prsadhuk-689cee3d-master
$ git checkout backport-prsadhuk-689cee3d-master
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk23u.git backport-prsadhuk-689cee3d-master

⚠️ @prsadhuk You are not yet a collaborator in my fork openjdk-bots/jdk23u. An invite will be sent out and you need to accept it before you can proceed.

aivanov-jdk · 2024-06-24T14:18:15Z

/backport openjdk/jdk23u

@prsadhuk You should backport it to 23 which corresponds to jdk23 branch in jdk repo. The command should look like this: /backport :jdk23 or /backport jdk jdk23, see the /backport command.

prsadhuk · 2024-06-24T16:32:23Z

/backport jdk jdk23

openjdk · 2024-06-24T16:33:17Z

@prsadhuk the backport was successfully created on the branch backport-prsadhuk-689cee3d-jdk23 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk23, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 689cee3d from the openjdk/jdk repository.

The commit being backported was authored by Prasanta Sadhukhan on 21 Jun 2024 and was reviewed by Alexey Ivanov and Phil Race.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk:

$ git fetch https://github.com/openjdk-bots/jdk.git backport-prsadhuk-689cee3d-jdk23:backport-prsadhuk-689cee3d-jdk23
$ git checkout backport-prsadhuk-689cee3d-jdk23
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk.git backport-prsadhuk-689cee3d-jdk23

⚠️ @prsadhuk You are not yet a collaborator in my fork openjdk-bots/jdk. An invite will be sent out and you need to accept it before you can proceed.

Page dialog fix

921651b

openjdk bot added the rfr Pull request is ready for review label Jun 19, 2024

openjdk bot added the client client-libs-dev@openjdk.org label Jun 19, 2024

kumarabhi006 reviewed Jun 19, 2024

View reviewed changes

Test fix

7daa416

TejeshR13 reviewed Jun 19, 2024

View reviewed changes

aivanov-jdk suggested changes Jun 19, 2024

View reviewed changes

prsadhuk added 2 commits June 20, 2024 04:33

Use ESC

8675d54

Test fix headful

2b298f8

aivanov-jdk approved these changes Jun 20, 2024

View reviewed changes

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

ALways return doIt

8a7471c

prrace approved these changes Jun 21, 2024

View reviewed changes

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

openjdk bot closed this Jun 21, 2024

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

prsadhuk deleted the print-fix branch June 21, 2024 18:03

aivanov-jdk reviewed Jun 21, 2024

View reviewed changes

aivanov-jdk mentioned this pull request Jun 24, 2024

8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show #19867

Closed

3 tasks

	* @summary Verifies oriignal pageobject is returned unmodified
	* @summary Verifies original pageobject is returned unmodified

		PageFormat newFormat = pj.pageDialog(oldFormat);
		if (!newFormat.equals(oldFormat)) {

8334509: Cancelling PageDialog does not return the same PageFormat object #19786

8334509: Cancelling PageDialog does not return the same PageFormat object #19786

Uh oh!

Conversation

prsadhuk commented Jun 19, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jun 19, 2024

Uh oh!

openjdk bot commented Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 19, 2024

Uh oh!

mlbridge bot commented Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TejeshR13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsadhuk Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsadhuk commented Jun 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsadhuk commented Jun 19, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 19, 2024 •

edited

Loading

mlbridge bot commented Jun 19, 2024 •

edited

Loading

prsadhuk Jun 20, 2024 •

edited

Loading