RFR: 8282862: AwtWindow::SetIconData leaks old icon handles if an exception is detected [v3]

[Alexey Ivanov]

On Tue, 7 Jan 2025 19:37:24 GMT, Rajat Mahajan <rmahajan at 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