RFR: JDK-8344907 : NullPointerException in Win32ShellFolder2.getSystemIcon when "icon" is null [v3]

[Alexey Ivanov]

On Tue, 7 Jan 2025 20:33:50 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:

>> Win32ShellFolder2.getSystemIcon() can result in NPE if icon is null. Sanity null checks have been added.
>> 
>> 
>> long hIcon = getSystemIcon(iconType.getIconID()); //Can return null
>> Image icon = makeIcon(hIcon); // returns null if the condition (hIcon != 0L && hIcon != -1L) is false
>> if (LARGE_ICON_SIZE != icon.getWidth(null)) { // NullPointerException -- icon is null
>> 
>> 
>> Occurs when Windows is unable to provide a system icon and the code assumes an icon will **always** be returned, but the Windows API makes no such guarantee.
>> 
>> getSystemIcon calls LoadIcon (User32.dll). This can return null.
>> https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-loadicona
>
> Harshitha Onkar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   inverted hIcon condition

Looks good to me except for minor comments.

src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1207:

> 1205:             return null;
> 1206:         }
> 1207:         Image icon = makeIcon(hIcon);

Suggestion:

        }

        Image icon = makeIcon(hIcon);

I'd add a blank line here to visually separate the two blocks: deal with `null`, then handle the “happy” case.

src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1222:

> 1220:         if (hIcon == 0) {
> 1221:             return null;
> 1222:         }

Suggestion:

        if (hIcon == 0) {
            return null;
        }


And I'd add a blank line here, too.

-------------

Marked as reviewed by aivanov (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22776#pullrequestreview-2537024999
PR Review Comment: https://git.openjdk.org/jdk/pull/22776#discussion_r1907112424
PR Review Comment: https://git.openjdk.org/jdk/pull/22776#discussion_r1907113744