On Tue, 7 Jan 2025 19:37:24 GMT, Rajat Mahajan <[email protected]> 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
The suggested fix does not address the reported issue yet: both `hOldIcon` and
`hOldIconSm` are still leaked if the function exists prematurely.
Based on my above comments, I suggest restructuring the code.
1. Create new icons using `CreateIconFromRaster`. Do not assign the handles to
member fields `m_hIcon` and `m_hIconSm` just yet.
2. Wrap the calls to `CreateIconFromRaster` in step 1 into a try-catch block to
prevent a C++ exception escaping from JNI code as well as ensure a Java
exception isn't raised.
1. If any exception occurs, destroy the newly created icon handles
(`(new_)hIcon` and `(new_)hIconSm`) and exit.
In this case both `m_hIcon` and `m_hIconSm` remain unmodified—the
current icons remain unchanged.
2. If no exception occurs,
1. Assign the old values of `m_hIcon` and `m_hIconSm` to temporary
variables (`hOldIcon` and `hOldIconSm`),
~~or call `DestroyIcon` right on the member fields `m_*`~~;
2. Assign the newly created icons to the member fields `m_hIcon` and
`m_hIconSm` and execute the code below to update the inherited icons;
3. Call `DestroyIcon` on `hOldIcon` and `hOldIconSm`.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/22932#issuecomment-2584089564
