On Tue, 7 Jan 2025 19:37:24 GMT, Rajat Mahajan <rmaha...@openjdk.org> wrote:
>> **Issue:**
>> AwtWindow::SetIconData leaks the old icon handles in hOldIcon and hOldIconSm
>> if CreateIconFromRaster raises an exception. Additionally, an exception is
>> checked only after the first call to CreateIconFromRaster.
>>
>> **Solution:**
>> I have added the exception handling code to take care that the handles are
>> properly destroyed and not leaked.
>>
>> **Testing:**
>> I have tested the code to make sure there are no regressions caused by this.
>
> Rajat Mahajan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update copyright year
Changes requested by aivanov (Reviewer).
src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 2127:
> 2125: try {
> 2126: m_hIcon = CreateIconFromRaster(env, iconRaster, w, h);
> 2127: JNU_CHECK_EXCEPTION(env); // Might throw here
The comment is misleading, `JNU_CHECK_EXCEPTION` does not throw an exception.
https://github.com/openjdk/jdk/blob/27646e551686ec02740600fc73694fc2fbd00a88/src/java.base/share/native/libjava/jni_util.h#L278-L283
It checks if an exception is raised in Java code and returns immediately if
there's a pending Java exception.
If a Java exception is raised at this line, after `m_hIcon` is assigned and its
value is not `NULL`, it has to be deleted.
Perhaps, you have to handle it explicitly here:
if (env->ExceptionCheck()) {
if (m_hIcon) {
DestroyIcon(m_hIcon);
}
return;
}
It looks that the original values stored in `hOldIcon` and `hOldIconSm` should
be restored if an error occurs.
src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 2129:
> 2127: JNU_CHECK_EXCEPTION(env); // Might throw here
> 2128: m_hIconSm = CreateIconFromRaster(env, smallIconRaster, smw,
> smh);
> 2129: JNU_CHECK_EXCEPTION(env); // Or here
The clean-up code here should delete both `m_hIcon` and `m_hIconSm`.
And restore the previous values?
src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 2130:
> 2128: m_hIconSm = CreateIconFromRaster(env, smallIconRaster, smw,
> smh);
> 2129: JNU_CHECK_EXCEPTION(env); // Or here
> 2130: } catch (...) {
At first I thought using C++ exception handling was a bad idea, but it's
actually needed. The code in `CreateIconFromRaster` can throw a C++ exception,
specifically `BitmapUtil::CreateTransparencyMaskFromARGB` and
`BitmapUtil::CreateV4BitmapFromARGB`.
https://github.com/openjdk/jdk/blob/e413fc643c4a58e3c46d81025c3ac9fbf89db4b9/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp#L2075-L2087
Since the exception is re-thrown in the handler there, it has to be caught in
`AwtWindow::SetIconData`. Yet you shouldn't re-throw the C++ exception here, it
must not escape JNI code, otherwise, the unhandled exception will likely crash
the Java process.
src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 2138:
> 2136: DestroyIcon(m_hIconSm);
> 2137: }
> 2138: throw; // Re-throw the exception
The C++ exception shouldn't be *re-thrown* here.
Whether we should raise a Java exception to indicate an error is to be
discussed. If we restore the state of the `AwtWindow` object as if no failure
occurred, it can continue. And I don't see any meaningful way to return an
error back to Java here.
`CreateIconFromRaster` throws `NullPointerException` if it fails to get the
image raster.
https://github.com/openjdk/jdk/blob/e413fc643c4a58e3c46d81025c3ac9fbf89db4b9/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp#L2076-L2078
where `JNI_CHECK_NULL_GOTO` is
https://github.com/openjdk/jdk/blob/27646e551686ec02740600fc73694fc2fbd00a88/src/java.desktop/windows/native/libawt/windows/awt.h#L48-L54
-------------
PR Review: https://git.openjdk.org/jdk/pull/22932#pullrequestreview-2535323825
PR Review Comment: https://git.openjdk.org/jdk/pull/22932#discussion_r1906029348
PR Review Comment: https://git.openjdk.org/jdk/pull/22932#discussion_r1906031377
PR Review Comment: https://git.openjdk.org/jdk/pull/22932#discussion_r1906013829
PR Review Comment: https://git.openjdk.org/jdk/pull/22932#discussion_r1906050261
