On Wed, 11 Mar 2026 19:19:37 GMT, Alexey Ivanov <[email protected]> wrote:
> This is a re-worked version of [my previous > attempt](https://git.openjdk.org/jdk/pull/19867) to fix the issue that was > found during code review > https://github.com/openjdk/jdk/pull/19786#discussion_r1645900131 for > [JDK-8334509](https://bugs.openjdk.org/browse/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](https://github.com/openjdk/jdk/pull/19786#discussion_r1647370601). > I'm sure this is what caused > [JDK-8334509](https://bugs.openjdk.org/browse/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 https://github.com/openjdk/jdk/pull/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 #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](https://github.com/aivanov-jdk/jdk/blob/30f776b95a1eda9fcacf77ff74a8eb41591fd6e9/src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp#L687-L700). 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! ------------- PR Comment: https://git.openjdk.org/jdk/pull/30207#issuecomment-4045129561
