Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Mon, 24 Jun 2024 20:16:30 GMT, Alexey Ivanov wrote: > This is somewhat a continuation for > [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) and > [JDK-8334509](https://bugs.openjdk.org/browse/JDK-8334509). > > The former removed the `doIt` flag in #18584, but it introduced a regression. > > The regression is resolved by the latter in #19786, and it added the 'doIt' > flag again. > > I think using a flag makes the code less clear. When reading the code, one > has to keep track of the current value of the `doIt` flag. > > I raised my concern in [comments in PR > 19786](https://github.com/openjdk/jdk/pull/19786#discussion_r1649191308): > >> 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. > > I came up with a solution which simplifies handling the clean-up. The > solution relies on C++ destructors which are called when the declared objects > go out of scope. > > The C++ object wraps a lambda function to clean up and invokes this function > in its destructor. Such C++ class has to be a templated class because there's > no defined type to represent a lambda. The class has to be declared at the > top of the file because it needs C++ linkage. > > There are two `Cleaner` objects declared in the code of > `Java_sun_awt_windows_WPageDialogPeer__1show`: > > **`refCleaner`** is declared right after all the references to Java objects > are initialised. The corresponding `refCleanup` lambda deletes all the > references and replaces the `CLEANUP_SHOW` macro. Before JDK-8307160, the > code jumped to a go-to label to perform the clean-up. > > **`postCleaner`** is declared after `AwtDialog::CheckInstallModalHook` is > called. Its lambda `postCleanup` uninstalls the modal hook and handles focus > transfer as well as saves the updated printer parameters. > > It is `postCleaner` that resolves the bug JDK-8334868 by ensuring > `CheckUninstallModalHook` and `ModalActivateNextWindow` are called if > `::PageSetupDlg` is called. > > As the result of using `refCleaner`, all the `return` statements in the > function use an explicit value, which makes the code cleaner. There's no need > to use a `go to` statement or insert a macro to delete references to Java > objects, about which one had to remember — the destructor of the `refCleaner` > handles deleting references when `refCleaner` goes out of scope, that is when > the function returns. Sorry, but I don't see this as better or needed. I was the one who requested the current code and I don't see the need for all this complexity. Just do the minimum change to make sure we do CheckUninstallModalHook is called. - Changes requested by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19867#pullrequestreview-2169910991
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Mon, 24 Jun 2024 20:16:30 GMT, Alexey Ivanov wrote: > This is somewhat a continuation for > [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) and > [JDK-8334509](https://bugs.openjdk.org/browse/JDK-8334509). > > The former removed the `doIt` flag in #18584, but it introduced a regression. > > The regression is resolved by the latter in #19786, and it added the 'doIt' > flag again. > > I think using a flag makes the code less clear. When reading the code, one > has to keep track of the current value of the `doIt` flag. > > I raised my concern in [comments in PR > 19786](https://github.com/openjdk/jdk/pull/19786#discussion_r1649191308): > >> 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. > > I came up with a solution which simplifies handling the clean-up. The > solution relies on C++ destructors which are called when the declared objects > go out of scope. > > The C++ object wraps a lambda function to clean up and invokes this function > in its destructor. Such C++ class has to be a templated class because there's > no defined type to represent a lambda. The class has to be declared at the > top of the file because it needs C++ linkage. > > There are two `Cleaner` objects declared in the code of > `Java_sun_awt_windows_WPageDialogPeer__1show`: > > **`refCleaner`** is declared right after all the references to Java objects > are initialised. The corresponding `refCleanup` lambda deletes all the > references and replaces the `CLEANUP_SHOW` macro. Before JDK-8307160, the > code jumped to a go-to label to perform the clean-up. > > **`postCleaner`** is declared after `AwtDialog::CheckInstallModalHook` is > called. Its lambda `postCleanup` uninstalls the modal hook and handles focus > transfer as well as saves the updated printer parameters. > > It is `postCleaner` that resolves the bug JDK-8334868 by ensuring > `CheckUninstallModalHook` and `ModalActivateNextWindow` are called if > `::PageSetupDlg` is called. > > As the result of using `refCleaner`, all the `return` statements in the > function use an explicit value, which makes the code cleaner. There's no need > to use a `go to` statement or insert a macro to delete references to Java > objects, about which one had to remember — the destructor of the `refCleaner` > handles deleting references when `refCleaner` goes out of scope, that is when > the function returns. Marked as reviewed by achung (Committer). - PR Review: https://git.openjdk.org/jdk/pull/19867#pullrequestreview-2147167889
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Thu, 27 Jun 2024 13:15:19 GMT, Julian Waters wrote: > It would be helpful if JNI had a jobject unique_ptr type for C++ But `std::unique_ptr` wasn't available when JNI had been conceived… It could be added… The declaration in your sample still looks cumbersome… or _unwieldy_ as you said. Writing a custom deleter for each `jobject` doesn't look good either. Most local references don't require explicit `DeleteLocalRef`, which also helps, I guess. > Oops, you sent your correction about the custom deleter just as I sent my post C++ has got many new features… I'm learning these feature on the fly since I don't use C++ as often. - PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194699149
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Mon, 24 Jun 2024 20:16:30 GMT, Alexey Ivanov wrote:
> This is somewhat a continuation for
> [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) and
> [JDK-8334509](https://bugs.openjdk.org/browse/JDK-8334509).
>
> The former removed the `doIt` flag in #18584, but it introduced a regression.
>
> The regression is resolved by the latter in #19786, and it added the 'doIt'
> flag again.
>
> I think using a flag makes the code less clear. When reading the code, one
> has to keep track of the current value of the `doIt` flag.
>
> I raised my concern in [comments in PR
> 19786](https://github.com/openjdk/jdk/pull/19786#discussion_r1649191308):
>
>> 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.
>
> I came up with a solution which simplifies handling the clean-up. The
> solution relies on C++ destructors which are called when the declared objects
> go out of scope.
>
> The C++ object wraps a lambda function to clean up and invokes this function
> in its destructor. Such C++ class has to be a templated class because there's
> no defined type to represent a lambda. The class has to be declared at the
> top of the file because it needs C++ linkage.
>
> There are two `Cleaner` objects declared in the code of
> `Java_sun_awt_windows_WPageDialogPeer__1show`:
>
> **`refCleaner`** is declared right after all the references to Java objects
> are initialised. The corresponding `refCleanup` lambda deletes all the
> references and replaces the `CLEANUP_SHOW` macro. Before JDK-8307160, the
> code jumped to a go-to label to perform the clean-up.
>
> **`postCleaner`** is declared after `AwtDialog::CheckInstallModalHook` is
> called. Its lambda `postCleanup` uninstalls the modal hook and handles focus
> transfer as well as saves the updated printer parameters.
>
> It is `postCleaner` that resolves the bug JDK-8334868 by ensuring
> `CheckUninstallModalHook` and `ModalActivateNextWindow` are called if
> `::PageSetupDlg` is called.
>
> As the result of using `refCleaner`, all the `return` statements in the
> function use an explicit value, which makes the code cleaner. There's no need
> to use a `go to` statement or insert a macro to delete references to Java
> objects, about which one had to remember — the destructor of the `refCleaner`
> handles deleting references when `refCleaner` goes out of scope, that is when
> the function returns.
std::unique_ptr has the option for a custom deleter, meaning it can be used to
clean up Java objects and other cases where a simple delete call does not
suffice, for instance
```c++
std::unique_ptr self(env->GetObjectField(target,
AwtPrintDialog::controlID), [] (jobject* object) -> void {
env->DeleteLocalRef(*object); });
Admittedly for jobjects this is a lot more cumbersome, since instead of
replacing the pointer with a smart pointer, you wrap the pointer inside a smart
pointer... Which is unwieldy. It would be helpful if JNI had a jobject
unique_ptr type for C++
Oops, you sent your correction about the custom deleter just as I sent my post
-
PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194667659
PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194669210
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Thu, 27 Jun 2024 12:54:55 GMT, Alexey Ivanov wrote: > > …I believe I was referring to the use of C++'s std::unique_ptr, which has > > the functionality for cleanup that we need. > > Yes, `std::unique_ptr` could be useful for handling automatic deallocation of > objects created with the `new` operator. > > ~~It won't help with releasing references to Java objects though.~~ Since `std::unique_ptr` accepts a custom deleter, it may be used for deleting any object. - PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194662542
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Thu, 27 Jun 2024 11:26:24 GMT, Julian Waters wrote: > …I believe I was referring to the use of C++'s std::unique_ptr, which has the > functionality for cleanup that we need. Yes, `std::unique_ptr` could be useful for handling automatic deallocation of objects created with the `new` operator. It won't help with releasing references to Java objects though. > That is currently blocked by awt.dll not being allowed to use the C++ > Standard Library, but if one looks at the current awt.dll with dumpbin, > awt.dll is actually already is linked to msvcp.dll, meaning it already uses > C++ Standard Library somehow (I don't know what exactly makes it dependent on > msvcp.dll) I remember STL has been banned from AWT code, yet I don't know the reasons for it. It could be to keep the size smaller. I can see that `awt.dll` depends on `msvcp140.dll` and imports one function from it: `?_Xlength_error@std@@YAXPEBD@Z`. It doesn't look like the C++ Standard Library is used. I can't find where it comes from though… It could be worth getting rid of this dependency. I built a small sample app which uses `std::unique_ptr` and `std::make_unique` and it does not depend on `msvcp140.dll` at all. This makes sense, template libraries are header-only and will become part of the executable (or the dynamic library). The only references to `std` namespace that I found in client are in `ShellFolder.cpp` which uses `std::bad_alloc`, it uses this type in `catch` blocks and it may throw an object of this class. So, it looks `awt.dll` doesn't use the C++ Standard Library. However, using C++ objects with [RAII](https://en.wikipedia.org/wiki/Resource_acquisition_is_initialization) makes the code shorter and frees the programmer from repeating clean-up blocks¹. Indeed, it's already used by C++ classes in AWT: [`JNILocalFrame`](https://github.com/openjdk/jdk/blob/79a23017fc7154738c375fbb12a997525c3bf9e7/src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h#L64-L77) and [`CriticalSection`](https://github.com/openjdk/jdk/blob/79a23017fc7154738c375fbb12a997525c3bf9e7/src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h#L85-L119) along with its `Lock` class inside. ¹ Repeated clean-up blocks can be seen in #18584 in nearly all the files. - PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194603793
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Mon, 24 Jun 2024 20:16:30 GMT, Alexey Ivanov wrote: > This is somewhat a continuation for > [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) and > [JDK-8334509](https://bugs.openjdk.org/browse/JDK-8334509). > > The former removed the `doIt` flag in #18584, but it introduced a regression. > > The regression is resolved by the latter in #19786, and it added the 'doIt' > flag again. > > I think using a flag makes the code less clear. When reading the code, one > has to keep track of the current value of the `doIt` flag. > > I raised my concern in [comments in PR > 19786](https://github.com/openjdk/jdk/pull/19786#discussion_r1649191308): > >> 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. > > I came up with a solution which simplifies handling the clean-up. The > solution relies on C++ destructors which are called when the declared objects > go out of scope. > > The C++ object wraps a lambda function to clean up and invokes this function > in its destructor. Such C++ class has to be a templated class because there's > no defined type to represent a lambda. The class has to be declared at the > top of the file because it needs C++ linkage. > > There are two `Cleaner` objects declared in the code of > `Java_sun_awt_windows_WPageDialogPeer__1show`: > > **`refCleaner`** is declared right after all the references to Java objects > are initialised. The corresponding `refCleanup` lambda deletes all the > references and replaces the `CLEANUP_SHOW` macro. Before JDK-8307160, the > code jumped to a go-to label to perform the clean-up. > > **`postCleaner`** is declared after `AwtDialog::CheckInstallModalHook` is > called. Its lambda `postCleanup` uninstalls the modal hook and handles focus > transfer as well as saves the updated printer parameters. > > It is `postCleaner` that resolves the bug JDK-8334868 by ensuring > `CheckUninstallModalHook` and `ModalActivateNextWindow` are called if > `::PageSetupDlg` is called. > > As the result of using `refCleaner`, all the `return` statements in the > function use an explicit value, which makes the code cleaner. There's no need > to use a `go to` statement or insert a macro to delete references to Java > objects, about which one had to remember — the destructor of the `refCleaner` > handles deleting references when `refCleaner` goes out of scope, that is when > the function returns. It's been quite a while since I said that, so I don't fully remember what I was talking about, but I believe I was referring to the use of C++'s std::unique_ptr, which has the functionality for cleanup that we need. That is currently blocked by awt.dll not being allowed to use the C++ Standard Library, but if one looks at the current awt.dll with dumpbin, awt.dll is actually already is linked to msvcp.dll, meaning it already uses C++ Standard Library somehow (I don't know what exactly makes it dependent on msvcp.dll) - PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194433117
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Mon, 24 Jun 2024 20:16:30 GMT, Alexey Ivanov wrote: > This is somewhat a continuation for > [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) and > [JDK-8334509](https://bugs.openjdk.org/browse/JDK-8334509). > > The former removed the `doIt` flag in #18584, but it introduced a regression. > > The regression is resolved by the latter in #19786, and it added the 'doIt' > flag again. > > I think using a flag makes the code less clear. When reading the code, one > has to keep track of the current value of the `doIt` flag. > > I raised my concern in [comments in PR > 19786](https://github.com/openjdk/jdk/pull/19786#discussion_r1649191308): > >> 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. > > I came up with a solution which simplifies handling the clean-up. The > solution relies on C++ destructors which are called when the declared objects > go out of scope. > > The C++ object wraps a lambda function to clean up and invokes this function > in its destructor. Such C++ class has to be a templated class because there's > no defined type to represent a lambda. The class has to be declared at the > top of the file because it needs C++ linkage. > > There are two `Cleaner` objects declared in the code of > `Java_sun_awt_windows_WPageDialogPeer__1show`: > > **`refCleaner`** is declared right after all the references to Java objects > are initialised. The corresponding `refCleanup` lambda deletes all the > references and replaces the `CLEANUP_SHOW` macro. Before JDK-8307160, the > code jumped to a go-to label to perform the clean-up. > > **`postCleaner`** is declared after `AwtDialog::CheckInstallModalHook` is > called. Its lambda `postCleanup` uninstalls the modal hook and handles focus > transfer as well as saves the updated printer parameters. > > It is `postCleaner` that resolves the bug JDK-8334868 by ensuring > `CheckUninstallModalHook` and `ModalActivateNextWindow` are called if > `::PageSetupDlg` is called. > > As the result of using `refCleaner`, all the `return` statements in the > function use an explicit value, which makes the code cleaner. There's no need > to use a `go to` statement or insert a macro to delete references to Java > objects, about which one had to remember — the destructor of the `refCleaner` > handles deleting references when `refCleaner` goes out of scope, that is when > the function returns. src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 537: > 535: env->DeleteLocalRef(page); > 536: env->DeleteLocalRef(self); > 537: }; In the long term, it is better to create a special class that deletes global and local references. It could be these constructs to which @TheShermanTanker referred in [this comment](https://github.com/openjdk/jdk/pull/18584#pullrequestreview-1975384014): https://github.com/openjdk/jdk/pull/18584#pullrequestreview-1975384014";>…I have a change in mind relying on _RAII_ that would look much cleaner and neater. - PR Review Comment: https://git.openjdk.org/jdk/pull/19867#discussion_r1656909846
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Wed, 26 Jun 2024 23:42:16 GMT, Alisen Chung wrote:
>> src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 640:
>>
>>> 638: HGLOBAL oldG = AwtPrintControl::getPrintHDMode(env, self);
>>> 639: if (setup.hDevMode != oldG) {
>>> 640:AwtPrintControl::setPrintHDMode(env, self, setup.hDevMode);
>>
>> what does setPrintHDMode and setPrintHDName do? do they need to be called as
>> part of cleanup even if there is an exception and we return JNI_FALSE?
>
> i think this code preserves the printer settings after the dialog closes (and
> only on success before this fix), so is it possible in the case the printer
> is removed and causes an exception, we shouldn't preserve these settings?
It's a good question…
Yes, the code saves (or updates) the printer name and mode.
These lines of code were called in cases where OK or Cancel is clicked in the
print dialog. However, it was skipped in case of an error, just like
`CheckUninstallModalHook`.
I also feel that these lines belong in the successful case, right before
`return JNI_TRUE`, but I could quickly find any confirmation whether
`setup.hDevMode` or `setup.hDevNames` can be modified. For this reason, I
preserved the previous code flow.
I presume that neither `setup.hDevMode` or `setup.hDevNames` changes if the
user clicks Cancel in `::PageSetupDlg`. It still leaves the question open what
happens when the user clicks OK but we catch an error inside `if (ret)`. The
old handles stored inside `self` could have become invalid, so it's safer to
update the handles in `self` even if an error occurs.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19867#discussion_r1656892141
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Wed, 26 Jun 2024 23:37:48 GMT, Alisen Chung wrote:
>> This is somewhat a continuation for
>> [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) and
>> [JDK-8334509](https://bugs.openjdk.org/browse/JDK-8334509).
>>
>> The former removed the `doIt` flag in #18584, but it introduced a
>> regression.
>> The regression is resolved by the latter in #19786, and it added the 'doIt'
>> flag again.
>>
>> I think using a flag makes the code less clear. When reading the code, one
>> has to keep track of the current value of the `doIt` flag.
>>
>> I raised my concern in [comments in PR
>> 19786](https://github.com/openjdk/jdk/pull/19786#discussion_r1649191308):
>>
>>> 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.
>>
>> I came up with a solution which simplifies handling the clean-up. The
>> solution relies on C++ destructors which are called when the declared
>> objects go out of scope.
>>
>> The C++ object wraps a lambda function to clean up and invokes this function
>> in its destructor. Such C++ class has to be a templated class because
>> there's no defined type to represent a lambda. The class has to be declared
>> at the top of the file because it needs C++ linkage.
>>
>> There are two `Cleaner` objects declared in the code of
>> `Java_sun_awt_windows_WPageDialogPeer__1show`:
>>
>> **`refCleaner`** is declared right after all the references to Java objects
>> are initialised. The corresponding `refCleanup` lambda deletes all the
>> references and replaces the `CLEANUP_SHOW` macro. Before JDK-8307160, the
>> code jumped to a go-to label to perform the clean-up.
>>
>> **`postCleaner`** is declared after `AwtDialog::CheckInstallModalHook` is
>> called. Its lambda `postCleanup` uninstalls the modal hook and handles focus
>> transfer as well as saves the updated printer parameters.
>>
>> It is `postCleaner` that resolves the bug JDK-8334868 by ensuring
>> `CheckUninstallModalHook` and `ModalActivateNextWindow` are called if
>> `::PageSetupDlg` is called.
>>
>> As the result of using `refCleaner`, all the `return` statements in the
>> function use an explicit value, which makes the code cleaner. There's no
>> need to use a `go to` statement or insert a macro to delete references to
>> Java objects, about which one had to remember — the destructor of the
>> `refCleaner` handles deleting references when `refCleaner` goes out of
>> scope, that is when the function returns.
>
> src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 640:
>
>> 638: HGLOBAL oldG = AwtPrintControl::getPrintHDMode(env, self);
>> 639: if (setup.hDevMode != oldG) {
>> 640:AwtPrintControl::setPrintHDMode(env, self, setup.hDevMode);
>
> what does setPrintHDMode and setPrintHDName do? do they need to be called as
> part of cleanup even if there is an exception and we return JNI_FALSE?
i think this code preserves the printer settings after the dialog closes (and
only on success before this fix), so is it possible in the case the printer is
removed and causes an exception, we shouldn't preserve these settings?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19867#discussion_r1655642352
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Mon, 24 Jun 2024 20:16:30 GMT, Alexey Ivanov wrote:
> This is somewhat a continuation for
> [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) and
> [JDK-8334509](https://bugs.openjdk.org/browse/JDK-8334509).
>
> The former removed the `doIt` flag in #18584, but it introduced a regression.
>
> The regression is resolved by the latter in #19786, and it added the 'doIt'
> flag again.
>
> I think using a flag makes the code less clear. When reading the code, one
> has to keep track of the current value of the `doIt` flag.
>
> I raised my concern in [comments in PR
> 19786](https://github.com/openjdk/jdk/pull/19786#discussion_r1649191308):
>
>> 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.
>
> I came up with a solution which simplifies handling the clean-up. The
> solution relies on C++ destructors which are called when the declared objects
> go out of scope.
>
> The C++ object wraps a lambda function to clean up and invokes this function
> in its destructor. Such C++ class has to be a templated class because there's
> no defined type to represent a lambda. The class has to be declared at the
> top of the file because it needs C++ linkage.
>
> There are two `Cleaner` objects declared in the code of
> `Java_sun_awt_windows_WPageDialogPeer__1show`:
>
> **`refCleaner`** is declared right after all the references to Java objects
> are initialised. The corresponding `refCleanup` lambda deletes all the
> references and replaces the `CLEANUP_SHOW` macro. Before JDK-8307160, the
> code jumped to a go-to label to perform the clean-up.
>
> **`postCleaner`** is declared after `AwtDialog::CheckInstallModalHook` is
> called. Its lambda `postCleanup` uninstalls the modal hook and handles focus
> transfer as well as saves the updated printer parameters.
>
> It is `postCleaner` that resolves the bug JDK-8334868 by ensuring
> `CheckUninstallModalHook` and `ModalActivateNextWindow` are called if
> `::PageSetupDlg` is called.
>
> As the result of using `refCleaner`, all the `return` statements in the
> function use an explicit value, which makes the code cleaner. There's no need
> to use a `go to` statement or insert a macro to delete references to Java
> objects, about which one had to remember — the destructor of the `refCleaner`
> handles deleting references when `refCleaner` goes out of scope, that is when
> the function returns.
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 640:
> 638: HGLOBAL oldG = AwtPrintControl::getPrintHDMode(env, self);
> 639: if (setup.hDevMode != oldG) {
> 640:AwtPrintControl::setPrintHDMode(env, self, setup.hDevMode);
what does setPrintHDMode and setPrintHDName do? do they need to be called as
part of cleanup even if there is an exception and we return JNI_FALSE?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19867#discussion_r1655640044
RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
This is somewhat a continuation for [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) and [JDK-8334509](https://bugs.openjdk.org/browse/JDK-8334509). The former removed the `doIt` flag in #18584, but it introduced a regression. The regression is resolved by the latter in #19786, and it added the 'doIt' flag again. I think using a flag makes the code less clear. When reading the code, one has to keep track of the current value of the `doIt` flag. I raised my concern in [comments in PR 19786](https://github.com/openjdk/jdk/pull/19786#discussion_r1649191308): > 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. I came up with a solution which simplifies handling the clean-up. The solution relies on C++ destructors which are called when the declared objects go out of scope. The C++ object wraps a lambda function to clean up and invokes this function in its destructor. Such C++ class has to be a templated class because there's no defined type to represent a lambda. The class has to be declared at the top of the file because it needs C++ linkage. There are two `Cleaner` objects declared in the code of `Java_sun_awt_windows_WPageDialogPeer__1show`: **`refCleaner`** is declared right after all the references to Java objects are initialised. The corresponding `refCleanup` lambda deletes all the references and replaces the `CLEANUP_SHOW` macro. Before JDK-8307160, the code jumped to a go-to label to perform the clean-up. **`postCleaner`** is declared after `AwtDialog::CheckInstallModalHook` is called. Its lambda `postCleanup` uninstalls the modal hook and handles focus transfer as well as saves the updated printer parameters. It is `postCleaner` that resolves the bug JDK-8334868 by ensuring `CheckUninstallModalHook` and `ModalActivateNextWindow` are called if `::PageSetupDlg` is called. As the result of using `refCleaner`, all the `return` statements in the function use an explicit value, which makes the code cleaner. There's no need to use a `go to` statement or insert a macro to delete references to Java objects, about which one had to remember — the destructor of the `refCleaner` handles deleting references when `refCleaner` goes out of scope, that is when the function returns. - Commit messages: - 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show Changes: https://git.openjdk.org/jdk/pull/19867/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19867&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8334868 Stats: 92 lines in 1 file changed: 48 ins; 36 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/19867.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19867/head:pull/19867 PR: https://git.openjdk.org/jdk/pull/19867
