RFR: JDK-8311113: Remove invalid pointer cast and clean up setLabel() in awt_MenuItem.cpp [v3]

[Phil Race]

On Fri, 25 Aug 2023 19:18:45 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:

>> In awt_MenuItem.cpp (712,22): ` mii.dwTypeData = (LPTSTR)(*sb)`  produces invalid pointer cast warning when complied on clang and moreover this is a no-op.  
>> 
>> `mii.dwTypeData` is used only when **MIIM_STRING** flag is set in the fMask (as per [Docs](https://learn.microsoft.com/en-us/windows/win32/api/winuser/ns-winuser-menuiteminfoa)), which is not the case in JDK [Ln#705](https://github.com/openjdk/jdk/blob/e56d3bc2dab3d32453b6eda66e8434953c436084/src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp#L706). Hence the assignment ` mii.dwTypeData = (LPTSTR)(*sb)`  is not required and so is the label parameter. Additionally necessary cleanup is done at the following places -
>> 
>> - WMenuItemPeer.java - to the native function call
>> - awt_MenuItem.cpp -  `WMenuItemPeer__1setLabel() ,_SetLabel(), SetLabel()`
>> - awt_MenuItem.h
>> 
>> Added a test which checks setLabel() functionality on Menu, MenuItem and PopupMenu.
>
> Harshitha Onkar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   removed robot.waitForIdle() calls

> I would also add that the pointer saved to `mii.dwTypeData` becomes invalid as soon as `m->SetLabel(labelPtr)` returns because the code in `_SetLabel` releases the pointer `labelPtr`.
> 
> Essentially, this was the code flow in `_SetLabel`:
> 
> ```c++
> LPCTSTR labelPtr = JNU_GetStringPlatformChars(env, label, 0);
> m->SetLabel(labelPtr);
> JNU_ReleaseStringPlatformChars(env, label, labelPtr);
> ```
> 
> If any code had dereferenced the pointer stored for a menu item in `dwTypeData`, the process would've crashed with access violation, or it could've led to a memory corruption.

I don't think that's relevant. "mii" is stack allocated  and the code does
    ::InsertMenuItem(hMenu, idx, TRUE, &mii);

and this pattern  occurs in other places too.

So I conclude that - although it isn't documented SFAICS - that GDI deep copies what it needs out of the struct.

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

PR Comment: https://git.openjdk.org/jdk/pull/15276#issuecomment-1699664817