8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show

[aivanov-jdk]

This is a re-worked version of my previous attempt to fix the issue that was found during code review #19786 (comment) for JDK-8334509.

To ensure AwtDialog::CheckInstallModalHook() and AwtDialog::ModalActivateNextWindow are always called, I moved them from the bottom of the function. Now these functions are called right after displaying the page dialog, that is after ::PageSetupDlg returns.

I reversed the condition of the if statement to if (!ret) and exit from the function right away if ::PageSetupDlg returned an error.

The diff looks large because the body of the if is unindented.

With this change, I modify the data flow and remove the doIt flag. The data flow with the flag is not as clear, and I already raised my concern. I'm sure this is what caused JDK-8334509.

There was only one place where JNI_TRUE is assigned to the doIt flag—in the end of the if (ret) block. Therefore, the last return statement in the WPageDialogPeer__1show function could have either JNI_FALSE or JNI_TRUE stored in the doIt flag. In all the other return statements, the value of doIt remains JNI_FALSE.

It was the mistake in #18584 that assumed JNI_TRUE was the only possibility in the end of the function.

By returning early if ::PageSetupDlg results in an error, I eliminate the above possibility—if the last return statement in WPageDialogPeer__1show is reached, it has to return JNI_TRUE.

All the other return statements explicitly return JNI_FALSE… because an error occurred.

To verify that this change does not introduce another regression,

• I use PageDialogCancelTest.java that was added in 8334509: Cancelling PageDialog does not return the same PageFormat object #19786 which covers the case where ::PageSetupDlg returns an error; and
• I add a new test, PageDialogFormatTest.java, that covers the successful case.

Additionally, I ensure ::GlobalUnlock(setup.hDevMode) is called on the error return from the block at lines 687–700.

---

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 2 Reviewers)

Issue

• JDK-8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show (Bug - P3)

Reviewers

• Dmitry Markov (@dmarkov20 - Reviewer)
• Phil Race (@prrace - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30207

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back aivanov! 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]

@aivanov-jdk 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:

8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show

Ensure AwtDialog::CheckInstallModalHook and
AwtDialog::ModalActivateNextWindow are always called
after ::PageSetupDlg returns.

Reverse the condition of the if statement and
bail out if ::PageSetupDlg returns an error.

Remove the doIt flag and use explicit returns:
* JNI_FALSE if an error detected;
* JNI_TRUE  if the function reached its end
without errors.

Reviewed-by: dmarkov, 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 393 new commits pushed to the master branch:

• e6bce14: 8381765: AArch32: Capstone hsdis does not build
• d68065e: 8365498: jdk/jfr/event/os/TestCPULoad.java fails with Expected at least one event
• 9a1f7d4: 8326205: Grouping frequently called C2 nmethods in CodeCache
• ... and 390 more: https://git.openjdk.org/jdk/compare/acbd1febdd8662f857a0ff140e85d59ce80f6d6a...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]

@aivanov-jdk 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]

Webrevs

• 00: Full (30f776b9)

[aivanov-jdk]

@prrace and @prsadhuk, please review.

[jaokim]

This looks much better compared to with the doIt flag and the long if (ret) {...}. Much less error prone, and easier to comprehend.
Thanks for fixing!

[aivanov-jdk]

/reviewers 2 reviewer

[openjdk]

@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 2 Reviewers).

[prrace]

LGTM.

[aivanov-jdk]

/summary
Ensure AwtDialog::CheckInstallModalHook and
AwtDialog::ModalActivateNextWindow are always called
after ::PageSetupDlg returns.

Reverse the condition of the if statement and
bail out if ::PageSetupDlg returns an error.

Remove the doIt flag and use explicit returns:

• JNI_FALSE if an error detected;
• JNI_TRUE if the function reached its end
  without errors.

[openjdk]

@aivanov-jdk Setting summary to:

Ensure AwtDialog::CheckInstallModalHook and
AwtDialog::ModalActivateNextWindow are always called
after ::PageSetupDlg returns.

Reverse the condition of the if statement and
bail out if ::PageSetupDlg returns an error.

Remove the doIt flag and use explicit returns:
* JNI_FALSE if an error detected;
* JNI_TRUE  if the function reached its end
without errors.

[aivanov-jdk]

/integrate

[openjdk]

Going to push as commit 8357de8.
Since your change was applied there have been 398 commits pushed to the master branch:

• a0af250: 8381006: G1: Wrong IHOP old gen allocation rate calculation in presence of periodic gcs
• d89bd4c: 8380542: ZipOutputStream.setComment and ZipEntry.setComment spec updates for rejecting unmappable characters and allowing null or empty comments
• 24f7945: 8377946: Use DWARF in mixed jstack on Linux AArch64
• ... and 395 more: https://git.openjdk.org/jdk/compare/acbd1febdd8662f857a0ff140e85d59ce80f6d6a...master

Your commit was automatically rebased without conflicts.

[openjdk]

@aivanov-jdk Pushed as commit 8357de8.

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