[cocoa] Stop using `CancelMenuTracking()`

[SyntevoAlex]

In Bug 578171, a crash was discovered. Back then I didn't realize that it's triggered by CancelMenuTracking(), and thought that it was triggered by showing a Shell when browsing menu bar.

Today I spent time to make a native snippet to report bug to Apple, but when doing so, I found that

• Crash is triggered by CancelMenuTracking()
• menubar.cancelTracking() doesn't trigger it

I also found that CancelMenuTracking() is not supposed to be called from 64-bit code, see Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/Headers/Menus.h :

#if !__LP64__
/*
 *  CancelMenuTracking()
 *  
 *  Summary:
 *    Cancels a menu tracking session.
 *  
 *  Mac OS X threading:
 *    Not thread safe
 *  
 *  Parameters:
 *    
 *    inRootMenu:
 *      The root menu of the menu tracking session that should be
 *      dismissed. For menubar tracking, use the result of AcquireRoot
 *      menu; for popup menu tracking, use the menu that was passed to
 *      PopUpMenuSelect.
 *    
 *    inImmediate:
 *      Whether the open menus should disappear immediately or fade out.
 *    
 *    inDismissalReason:
 *      Why the menu is being dismissed; this value will be added to
 *      the kEventMenuEndTracking event. On Mac OS X 10.5 and later,
 *      you may pass zero to indicate that
 *      kHIMenuDismissedByCancelMenuTracking should be passed to the
 *      EndTracking event.
 *  
 *  Availability:
 *    Mac OS X:         in version 10.3 and later in Carbon.framework [32-bit only]
 *    CarbonLib:        not available in CarbonLib 1.x, is available on Mac OS X version 10.3 and later
 *    Non-Carbon CFM:   not available
 */

Here, note [32-bit only] and #if !__LP64__.

In order to circumvent that, SWT uses dynamic linking to access the API:

/** @method flags=dynamic */
public static final native long AcquireRootMenu ();
/** @method flags=dynamic */
public static final native int CancelMenuTracking (long inRootMenu, boolean inImmediate, int inDismissalReason);

To summarize:

• SWT uses API it shouldn't be using
• This causes a JVM crash in macOS 12+ (currently band-aided in #277)

The suggested fix is to

• Figure if there's still a reason to call CancelMenuTracking()
• Get rid of it
• Revert patch #277 as no longer needed.

[SyntevoAlex]

Here's my native snippet I used for testing (compile with clang++ -std=c++11 -Wno-deprecated-declarations --debug -framework Cocoa -framework Carbon SWT_578171_macOS_crash_ShowWindowWhenBrowsingMenu.mm -o SWT_578171_macOS_crash_ShowWindowWhenBrowsingMenu) SWT_578171_macOS_crash_ShowWindowWhenBrowsingMenu.mm.txt

SWT snippet is already in the repo, see Bug578171_macOS_JvmCrash_ShowMenuWindow

[SyntevoAlex]

I will not be able to handle this soon, so everyone is welcome to fix that.

[Phillipus]

Can we simply remove cancelRootMenuTracking and preventShellActivateJvmCrash to test?

Or replace with menubar.cancelTracking()?

[SyntevoAlex]

Removing preventShellActivateJvmCrash() will restore the crash that is currently suppressed in #277. Removing cancelRootMenuTracking() will remove the cause of the crash. I have no idea if it's still needed at all.

[SyntevoAlex]

If it's needed, then replacing with menubar.cancelTracking() is the most reasonable fix, but I haven't investigated why it was changed to CancelMenuTracking() years ago and if it's still important.

[Phillipus]

Comment in that part of the code:

* For some reason, NSMenu.cancelTracking() does not dismisses
* the menu right away when the menu bar is set in a stacked
* event loop. The fix is to use CancelMenuTracking() instead.

[SyntevoAlex]

Yes, I seen this comment, of course. It doesn't quite explain what does it really fix, and why cancelling tracking is needed in the first place.

[Phillipus]

Can you make a PR with these changes (and use menubar.cancelTracking()) for testing?

(I put the javadoc comment here for other people to see)

[SyntevoAlex]

Here, I removed both: #338 With very quick testing, it doesn't seem that any form of cancelling tracking is needed.

[Phillipus]

It was added in 080eb4cfdf4de7f642c2aaa74935d2369b548d27

https://bugs.eclipse.org/bugs/show_bug.cgi?id=270379

[Phillipus]

I removed CancelMenuTracking and tried to recreate the steps in https://bugs.eclipse.org/bugs/show_bug.cgi?id=270379 ~but could not because I couldn't do "Then click the Help menu again"~

Edit: see below

[Phillipus]

I removed CancelMenuTracking and tried to recreate the steps in https://bugs.eclipse.org/bugs/show_bug.cgi?id=270379 but could not because I couldn't do "Then click the Help menu again"

Actually I am wrong. I did manage to follow those steps and see the problem.

[Phillipus]

And I tried the same thing using menubar.cancelTracking() and I saw the same problem.

[Phillipus]

The steps in 270379 can be tricky to follow. So in our RCP app we have a Help menu to which I added an Action with this run method:

    public void run() {
        Display.getDefault().timerExec(2000, () -> {
            MessageDialog.openInformation(null, "Test", "Test");
        });
    }

1. Click in the Help menu to perform above action
2. Click back in the Help menu so that Help search field has the focus and wait for dialog to appear
3. Dismiss dialog
4. Main menu bar is not properly restored if CancelMenuTracking is not called

[Phillipus]

In fact, while testing above with not using CancelMenuTracking I got lots of NPEs:

java.lang.NullPointerException
    at org.eclipse.ui.internal.handlers.LegacyHandlerService.registerLegacyHandler(LegacyHandlerService.java:164)
    at org.eclipse.ui.internal.handlers.LegacyHandlerService.registerLegacyHandler(LegacyHandlerService.java:158)
    at org.eclipse.ui.internal.handlers.LegacyHandlerService.activateHandler(LegacyHandlerService.java:306)
    at org.eclipse.ui.internal.handlers.LegacyHandlerService.activateHandler(LegacyHandlerService.java:299)
    at org.eclipse.ui.SubActionBars.setGlobalActionHandler(SubActionBars.java:549)
    at org.eclipse.gef.ui.actions.ActionBarContributor.setActiveEditor(ActionBarContributor.java:161)
    at org.eclipse.ui.internal.EditorActionBars.partChanged(EditorActionBars.java:335)
    at org.eclipse.ui.internal.WorkbenchPage.updateActivations(WorkbenchPage.java:323)
    at org.eclipse.ui.internal.WorkbenchPage$E4PartListener.partActivated(WorkbenchPage.java:212)
    at org.eclipse.e4.ui.internal.workbench.PartServiceImpl$2.run(PartServiceImpl.java:250)

[Phillipus]

Note that my tests were done with preventShellActivateJvmCrash removed. If that is present then CancelMenuTracking is not needed in Display#setMenuBar in the above test. Of course it may be needed in other cases.

[Phillipus]

My conclusion is not to remove CancelMenuTracking unless an alternative can be found to stop above situation.

CancelMenuTracking() is not supposed to be called from 64-bit code SWT uses API it shouldn't be using

Maybe, but it does still prevent the above situation, so it is doing something.

[SyntevoAlex]

Thanks for testing! Now we "just" need to find a better solution for an initial problem without making things even uglier :)

[Phillipus]

To make testing even easier I created a simple snippet. Instructions are in the snippet and on the app's label when run.

This is to test the effect of removing or commenting out this line in Display#cancelRootMenuTracking (line 4930 in current version):

OS.CancelMenuTracking (rootMenu, true, 0);

Click here to reveal the Snippet...

```java import org.eclipse.jface.dialogs.MessageDialog; import org.eclipse.swt.SWT; import org.eclipse.swt.events.SelectionListener; import org.eclipse.swt.layout.GridLayout; import org.eclipse.swt.widgets.Display; import org.eclipse.swt.widgets.Label; import org.eclipse.swt.widgets.Menu; import org.eclipse.swt.widgets.MenuItem; import org.eclipse.swt.widgets.Shell; public class CancelMenuTrackingTest { public static void main(String[] args) { final Display display = new Display(); final Shell shell = new Shell(display); shell.setLayout(new GridLayout(1, true)); shell.setSize(500, 300); new Label(shell, 0).setText("1. Run this on Mac\n" + "2. Go to the Help menu and select the \"Open Dialog\" menu item\n" + "3. Click back into the Help menu so that the Search field has the focus\n" + "4. Close the dialog that appears\n" + "5. Notice behaviour of menu bar if OS.CancelMenuTracking() is not called"); // Create Root Menu Menu rootMenu = new Menu(shell, SWT.BAR); shell.setMenuBar(rootMenu); // Add some menu items for(int iMenu = 1; iMenu < 3; iMenu++) { MenuItem rootItem = new MenuItem(rootMenu, SWT.CASCADE); rootItem.setText("Menu " + iMenu); Menu menu = new Menu(shell, SWT.DROP_DOWN); rootItem.setMenu(menu); for(int iItem = 0; iItem < 6; iItem++) { MenuItem item = new MenuItem(menu, SWT.CASCADE); item.setText("MenuItem " + iMenu + " : " + iItem); } } // Add the Mac Help Menu Item which will have a search text field MenuItem helpMenuItem = new MenuItem(rootMenu, SWT.CASCADE); helpMenuItem.setText("Help"); Menu helpMenu = new Menu(shell, SWT.DROP_DOWN); helpMenuItem.setMenu(helpMenu); // Add a menu item which will open a modal dialog after 2 seconds MenuItem runItem = new MenuItem(helpMenu, SWT.CASCADE); runItem.setText("Open Dialog"); runItem.addSelectionListener(SelectionListener.widgetSelectedAdapter(e -> { Display.getDefault().timerExec(2000, () -> { MessageDialog.openInformation(shell, "Test", "Test"); }); })); shell.open(); while(!shell.isDisposed()) { if(!display.readAndDispatch()) { display.sleep(); } } display.dispose(); } } ```

[Phillipus]

~I think I've found a clue.~

~If we want to use NSMenu#cancelTracking instead of OS.CancelMenuTracking the problem lies in when we call it.~

As POC, replace Display#cancelRootMenuTracking() with this:

static void cancelRootMenuTracking () {
    NSMenu menubar = getCurrent().application.mainMenu();
    menubar.cancelTracking();
}

~This pretty much solve my test case, above,~ and also the crash in Bug578171_macOS_JvmCrash_ShowMenuWindow

Edit - nope. Menu items are left hanging in the air in my test snippet. I should have tested this more before commenting here. Ignore above.