JDK-8311113: Remove invalid pointer cast and clean up setLabel() in awt_MenuItem.cpp #15276
Conversation
|
👋 Welcome back honkar! A progress list of the required criteria for merging this PR into |
|
@honkar-jdk The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
honkar-jdk
changed the title
|
Please double-check how the external tools like jaws/narrator will work after this change, will they be able to read the correct content of the menu item? |
|
I would also add that the pointer saved to Essentially, this was the code flow in 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 |
src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp
Outdated
Show resolved
Hide resolved
src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp
Outdated
Show resolved
Hide resolved
| @@ -709,7 +708,6 @@ void AwtMenuItem::SetLabel(LPCTSTR sb) | |||
| ::GetMenuItemInfo(hMenu, GetID(), FALSE, &mii); | |||
There was a problem hiding this comment.
Should we update the fMask to use MIIM_FTYPE instead of MIIM_TYPE? The latter requests both fType and dwTypeData whereas the former requests only fType, as we don't use dwTypeData.
(I understand that this code was written before Windows introduced new values for fMask field, which probably happened in Windows 98. Now we can update the code to document our intentions clearer.)
There was a problem hiding this comment.
Updated fMask to use MIIM_FTYPE
| private static boolean checkLabels() { | ||
| for (int i = 0; i < 2; i++) { | ||
| Menu m1 = mb.getMenu(i); | ||
| String menuLabel = m1.getLabel(); | ||
| String menuItemLabel = m1.getItem(0).getLabel(); | ||
| if (!(menuLabel.equals(newLabels[i][0]) | ||
| && menuItemLabel.equals(newLabels[i][1]))) { | ||
| passed = false; | ||
| break; | ||
| } | ||
| } | ||
| return passed; | ||
| } |
There was a problem hiding this comment.
My only concern here is that the test doesn't ensure that the menu has updated on the screen.
The data inside the Java object are always updated?
The test should probably make a screenshot before a label is changed, then make a screenshot after the label is changed — there should be differences on the screenshots.
There was a problem hiding this comment.
Test updated to compare after and before screenshots.
The updating of the label works correctly since the application handles the drawing/updating of the string as specified by the |
|
@aivanov-jdk @mrserb Please review updated changes. |
I don't think that's relevant. "mii" is stack allocated and the code does 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. |
Hmm, so where does JAWS get that info if we no longer set it ? It isn't scraping pixels and doing OCR. Seems like a lot of changes here to silence a warning we don't even get. |
Yes, I was wrong. I realised it later after I looked at the docs more. The description of the MENUITEMINFOW says, Yet this applies to menus of type |
Isn't JAWS using Accessible interfaces? @prrace The string has never been set because AWT menus aren't of When a new menu item is added to menu, we use jdk/src/java.desktop/windows/native/libawt/windows/awt_Menu.cpp Lines 214 to 221 in 3eac890 The last parameter passed to the So, the string value of a menu item is never set if The code above confusingly “sets” the jdk/src/java.desktop/windows/native/libawt/windows/awt_Menu.cpp Lines 212 to 213 in 3eac890 which is overridden by the Because the value of The above holds true for the flags in
Even though we don't get this warning, the value set to I strongly think that we should remove the code which copies the string label because it's never used. |
What about Narrator? I think it will use the actuall data from the windows, same as for other "native" awt components. Unlike JAWS which know how to use our internal a11y interface. |
Checked with Narrator and JAWS. It works the same in both cases - with and without the changes. In case of JAWS, it reads out label names as in "Item-1" and the changed labels "New Item-1". |
I'll have to check this further. |
It will get it via javaaccessbridge we provided(kind of native interface) and this native lib uses our a11y java code for each component. |
|
@mrserb @prrace If the string label was stored in the menu structure by Windows, we could retrieve it, right? Let's do it. I applied the following patch to the original diff --git a/src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp b/src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp
index 69a7f4606e2..ccdc82c63f5 100644
--- a/src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp
+++ b/src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp
@@ -34,6 +34,7 @@
#include <tchar.h>
#include <imm.h>
#include <ime.h>
+#include <stdio.h>
// End -- Win32 SDK include files
//add for multifont menuitem
@@ -685,6 +686,51 @@ void AwtMenuItem::DoCommand()
}
}
+static void PrintMenuItemInfo(HMENU hMenu, UINT id)
+{
+ MENUITEMINFO mii;
+
+ memset(&mii, 0, sizeof(MENUITEMINFO));
+ mii.cbSize = sizeof(MENUITEMINFO);
+ mii.fMask = MIIM_CHECKMARKS | MIIM_DATA | MIIM_ID
+ | MIIM_STATE | MIIM_SUBMENU | MIIM_TYPE;
+
+ if (!::GetMenuItemInfo(hMenu, id, FALSE, &mii)) {
+ wprintf(L"GetMenuItemInfo returned an error for %u, %p\n", id, hMenu);
+ return;
+ }
+
+ wprintf(L"Menu info for %u, %p\n", id, hMenu);
+ wprintf(L" dwItemData = %p\n", (void *) mii.dwItemData);
+ wprintf(L" cch = %u\n", mii.cch);
+ wprintf(L" dwTypeData = %p\n", mii.dwTypeData);
+ if (mii.cch > 0) {
+ mii.dwTypeData = new TCHAR[mii.cch + 1];
+ if (!::GetMenuItemInfo(hMenu, id, FALSE, &mii)) {
+ wprintf(L" failed to get label\n");
+ } else {
+ wprintf(L" label = %s\n", mii.dwTypeData);
+ }
+ delete[] mii.dwTypeData;
+ }
+
+ // fType
+ wprintf(L" fType = %x = ", mii.fType);
+ if (mii.fType & MFT_OWNERDRAW) {
+ wprintf(L"MFT_OWNERDRAW ");
+ }
+ if (mii.fType & MFT_BITMAP) {
+ wprintf(L"MFT_BITMAP ");
+ }
+ if ((mii.fType & (MFT_OWNERDRAW | MFT_BITMAP)) == 0) {
+ wprintf(L"MFT_STRING ");
+ }
+ if (mii.fType & MFT_SEPARATOR) {
+ wprintf(L"MFT_SEPARATOR ");
+ }
+ wprintf(L"\n");
+}
+
void AwtMenuItem::SetLabel(LPCTSTR sb)
{
AwtMenu* menu = GetMenuContainer();
@@ -700,6 +746,13 @@ void AwtMenuItem::SetLabel(LPCTSTR sb)
HMENU hMenu = menu->GetHMenu();
MENUITEMINFO mii, mii1;
+ wprintf(L"AwtMenuItem::SetLabel(this = %p, id = %u, hMenu = %p\n",
+ this, GetID(), hMenu);
+ wprintf(L"> Before\n");
+ PrintMenuItemInfo(hMenu, GetID());
+ wprintf(L"< Before\n");
+
+
// get full information about menu item
memset(&mii, 0, sizeof(MENUITEMINFO));
mii.cbSize = sizeof(MENUITEMINFO);
@@ -725,6 +778,10 @@ void AwtMenuItem::SetLabel(LPCTSTR sb)
::RemoveMenu(hMenu, idx, MF_BYPOSITION);
::InsertMenuItem(hMenu, idx, TRUE, &mii);
+ wprintf(L"> After\n");
+ PrintMenuItemInfo(hMenu, GetID());
+ wprintf(L"< After\n");
+
RedrawMenuBar();
}
By running Harshitha's test, I get the following output: As you can see, in all the cases, the This proves what's done in |
|
As for Windows 98 added new members to the At the same time, This is why we should switch to using |
|
@honkar-jdk This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 398 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@prrace The drawing of the label is being handled by the application here AwtMenuItem::DrawItem & AwtMenuItem::DrawSelf. |
Hmm, I didn't know the AWT heavyweights participated in that. Only the Swing components. |
OK. So it doesn't pull it because we don't tell it it is there to be read. |
I see the two a11y native libraries - |
It's a limitation of Windows Menu API: the system doesn't store any additional data for owner drawn menu items. We tell the system, here's the label, but it ignores it. It seems there's nothing we can do about it. I tried setting
There's no other way…
It is also my understanding, so I was surprised JAWS can read AWT menus. It is not a surprise Narrator can't read menus (or anything else in a Java app) since Java doesn't implement native Windows accessibility APIs. |
jdk/src/java.desktop/share/classes/java/awt/Menu.java Lines 618 to 628 in 2f7c65e jdk/src/java.desktop/share/classes/java/awt/MenuItem.java Lines 815 to 825 in 2f7c65e Thus AWT components implement the same accessibility interfaces that Swing components do. JAWS uses these interfaces. Mystery solved. |
I will be creating a separate issue to handle cleanup at other places. |
|
/integrate |
|
Going to push as commit 0c97246.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@honkar-jdk Pushed as commit 0c97246. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
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.dwTypeDatais used only when MIIM_STRING flag is set in the fMask (as per Docs), which is not the case in JDK Ln#705. Hence the assignmentmii.dwTypeData = (LPTSTR)(*sb)is not required and so is the label parameter. Additionally necessary cleanup is done at the following places -WMenuItemPeer__1setLabel() ,_SetLabel(), SetLabel()Added a test which checks setLabel() functionality on Menu, MenuItem and PopupMenu.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15276/head:pull/15276$ git checkout pull/15276Update a local copy of the PR:
$ git checkout pull/15276$ git pull https://git.openjdk.org/jdk.git pull/15276/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15276View PR using the GUI difftool:
$ git pr show -t 15276Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15276.diff
Webrev
Link to Webrev Comment