Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cocoa] Stop using CancelMenuTracking() #337

Open
SyntevoAlex opened this issue Aug 23, 2022 · 20 comments
Open

[cocoa] Stop using CancelMenuTracking() #337

SyntevoAlex opened this issue Aug 23, 2022 · 20 comments
Labels
macOS happens on macOS

Comments

@SyntevoAlex
Copy link
Member

SyntevoAlex commented Aug 23, 2022

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:

The suggested fix is to

@SyntevoAlex
Copy link
Member Author

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
Copy link
Member Author

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

@Phillipus
Copy link
Contributor

Phillipus commented Aug 23, 2022

Can we simply remove cancelRootMenuTracking and preventShellActivateJvmCrash to test?

Or replace with menubar.cancelTracking()?

@SyntevoAlex
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

Phillipus commented Aug 23, 2022

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
Copy link
Member Author

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

@Phillipus
Copy link
Contributor

@Phillipus
Copy link
Contributor

Phillipus commented Aug 24, 2022

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
Copy link
Contributor

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
Copy link
Contributor

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

@Phillipus
Copy link
Contributor

Phillipus commented Aug 24, 2022

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

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

@Phillipus
Copy link
Contributor

Phillipus commented Aug 25, 2022

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...
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
Copy link
Contributor

Phillipus commented Aug 25, 2022

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.

@lshanmug lshanmug added the macOS happens on macOS label Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macOS happens on macOS
Projects
None yet
Development

No branches or pull requests

3 participants