Bug 1707598 - When opening a new menu just after closing an old menu, make sure we exit the nested event loop for the old NSMenu before we ask the new NSMenu to open. r=harry

On CI, where we open and close menu items in quick succession, we sometimes got
into a state where the new menu was opened while we were still in the old menu's
nested event loop. So we had the following sequence of events:

```
 - old menu +[NSMenu popUpContextMenu:withEvent:forView:]
   - nested event loop for old menu
     - old menu -[NSMenu cancelTrackingWithoutAnimation]
     - new menu +[NSMenu popUpContextMenu:withEvent:forView:]
       - nested event loop for new menu
         - new menu -[NSMenu cancelTrackingWithoutAnimation]
     - new menu's event loop is exited, but old menu's event loop remains on the
       stack
     - shutdown hang here
```

MOZMenuOpeningCoordinator makes sure that +[NSMenu popUpContextMenu:withEvent:forView:]
is always called in sequence, never in a nested fashion.

Differential Revision: https://phabricator.services.mozilla.com/D113373

widget/cocoa/MOZMenuOpeningCoordinator.h

@@ -0,0 +1,37 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef MOZMenuOpeningCoordinator_h
+#define MOZMenuOpeningCoordinator_h
+
+#import <Cocoa/Cocoa.h>
+
+/*
+ * MOZMenuOpeningCoordinator is a workaround for the fact that opening an NSMenu creates a nested
+ * event loop. This event loop is only exited after the menu is closed. The caller of
+ * NativeMenuMac::ShowAsContextMenu does not expect ShowAsContextMenu to create a nested event loop,
+ * so we need to make sure to open the NSMenu asynchronously.
+ */
+
+@interface MOZMenuOpeningCoordinator : NSObject
+
++ (instancetype)sharedInstance;
+
+// Queue aMenu for opening.
+// The menu will open from a new event loop tick so that its nested event loop does not block the
+// caller. If another menu's nested event loop is currently on the stack, we wait for that nested
+// event loop to unwind before opening aMenu. Returns a handle that can be passed to
+// cancelAsynchronousOpening:. Can only be called on the main thread.
+- (NSInteger)asynchronouslyOpenMenu:(NSMenu*)aMenu
+                   atScreenPosition:(NSPoint)aPosition
+                            forView:(NSView*)aView;
+
+// If the menu opening request for aHandle hasn't been processed yet, cancel it.
+// Can only be called on the main thread.
+- (void)cancelAsynchronousOpening:(NSInteger)aHandle;
+
+@end
+
+#endif  // MOZMenuOpeningCoordinator_h

widget/cocoa/MOZMenuOpeningCoordinator.mm

@@ -0,0 +1,161 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/*
+ * Makes sure that the nested event loop for NSMenu tracking is situated as low
+ * on the stack as possible, and that two NSMenu event loops are never nested.
+ */
+
+#include "MOZMenuOpeningCoordinator.h"
+
+#include "nsCocoaFeatures.h"
+#include "nsCocoaUtils.h"
+#include "nsObjCExceptions.h"
+#include "SDKDeclarations.h"
+
+@interface MOZMenuOpeningInfo : NSObject
+@property NSInteger handle;
+@property(retain) NSMenu* menu;
+@property NSPoint position;
+@property(retain) NSView* view;
+@end
+
+@implementation MOZMenuOpeningInfo
+@end
+
+@implementation MOZMenuOpeningCoordinator {
+  // non-nil between asynchronouslyOpenMenu:atScreenPosition:forView: and the
+  // time at at which it is unqueued in _runMenu.
+  MOZMenuOpeningInfo* mPendingOpening;  // strong
+
+  // An incrementing counter
+  NSInteger mLastHandle;
+
+  // YES while _runMenu is on the stack
+  BOOL mRunMenuIsOnTheStack;
+}
+
++ (instancetype)sharedInstance {
+  static MOZMenuOpeningCoordinator* sInstance = nil;
+  if (!sInstance) {
+    sInstance = [[MOZMenuOpeningCoordinator alloc] init];
+  }
+  return sInstance;
+}
+
+- (NSInteger)asynchronouslyOpenMenu:(NSMenu*)aMenu
+                   atScreenPosition:(NSPoint)aPosition
+                            forView:(NSView*)aView {
+  MOZ_RELEASE_ASSERT(!mPendingOpening,
+                     "A menu is already waiting to open. Before opening the next one, either wait "
+                     "for this one to open or cancel the request.");
+
+  NSInteger handle = ++mLastHandle;
+
+  MOZMenuOpeningInfo* info = [[MOZMenuOpeningInfo alloc] init];
+  info.handle = handle;
+  info.menu = aMenu;
+  info.position = aPosition;
+  info.view = aView;
+  mPendingOpening = [info retain];
+  [info release];
+
+  if (!mRunMenuIsOnTheStack) {
+    // Call _runMenu from the event loop, so that it doesn't block this call.
+    [self performSelector:@selector(_runMenu) withObject:nil afterDelay:0.0];
+  }
+
+  return handle;
+}
+
+- (void)_runMenu {
+  MOZ_RELEASE_ASSERT(!mRunMenuIsOnTheStack);
+
+  mRunMenuIsOnTheStack = YES;
+
+  while (mPendingOpening) {
+    MOZMenuOpeningInfo* info = [mPendingOpening retain];
+    [mPendingOpening release];
+    mPendingOpening = nil;
+
+    @try {
+      [self _openMenu:info.menu atScreenPosition:info.position forView:info.view];
+    } @catch (NSException* exception) {
+      nsObjCExceptionLog(exception);
+    }
+
+    [info release];
+  }
+
+  mRunMenuIsOnTheStack = NO;
+}
+
+- (void)cancelAsynchronousOpening:(NSInteger)aHandle {
+  if (mPendingOpening && mPendingOpening.handle == aHandle) {
+    [mPendingOpening release];
+    mPendingOpening = nil;
+  }
+}
+
+- (void)_openMenu:(NSMenu*)aMenu atScreenPosition:(NSPoint)aPosition forView:(NSView*)aView {
+  // There are multiple ways to display an NSMenu as a context menu.
+  //
+  //  1. We can return the NSMenu from -[ChildView menuForEvent:] and the NSView will open it for
+  //     us.
+  //  2. We can call +[NSMenu popUpContextMenu:withEvent:forView:] inside a mouseDown handler with a
+  //     real mouse down event.
+  //  3. We can call +[NSMenu popUpContextMenu:withEvent:forView:] at a later time, with a real
+  //     mouse event that we stored earlier.
+  //  4. We can call +[NSMenu popUpContextMenu:withEvent:forView:] at any time, with a synthetic
+  //     mouse event that we create just for that purpose.
+  //  5. We can call -[NSMenu popUpMenuPositioningItem:atLocation:inView:] and it just takes a
+  //     position, not an event.
+  //
+  // 1-4 look the same, 5 looks different: 5 is made for use with NSPopUpButton, where the selected
+  // item needs to be shown at a specific position. If a tall menu is opened with a position close
+  // to the bottom edge of the screen, 5 results in a cropped menu with scroll arrows, even if the
+  // entire menu would fit on the screen, due to the positioning constraint.
+  // 1-2 only work if the menu contents are known synchronously during the call to menuForEvent or
+  // during the mouseDown event handler.
+  // NativeMenuMac::ShowAsContextMenu can be called at any time. It could be called during a
+  // menuForEvent call (during a "contextmenu" event handler), or during a mouseDown handler, or at
+  // a later time.
+  // The code below uses option 4 as the preferred option because it's the simplest: It works in all
+  // scenarios and it doesn't have the positioning drawbacks of option 5.
+
+  if (@available(macOS 10.14, *)) {
+#if !defined(MAC_OS_VERSION_11_0) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_VERSION_11_0
+    if (nsCocoaFeatures::OnBigSurOrLater()) {
+#else
+    if (@available(macOS 11.0, *)) {
+#endif
+      // Make native context menus respect the NSApp appearance rather than the NSWindow appearance.
+      [aMenu setAppearance:NSApp.effectiveAppearance];
+    }
+  }
+
+  if (aView) {
+    // Create a synthetic event at the right location and open the menu [option 4].
+    NSPoint locationInWindow = nsCocoaUtils::ConvertPointFromScreen(aView.window, aPosition);
+    NSEvent* event = [NSEvent mouseEventWithType:NSEventTypeRightMouseDown
+                                        location:locationInWindow
+                                   modifierFlags:0
+                                       timestamp:NSProcessInfo.processInfo.systemUptime
+                                    windowNumber:aView.window.windowNumber
+                                         context:nil
+                                     eventNumber:0
+                                      clickCount:1
+                                        pressure:0.0f];
+    [NSMenu popUpContextMenu:aMenu withEvent:event forView:aView];
+  } else {
+    // Open the menu using popUpMenuPositioningItem:atLocation:inView: [option 5].
+    // This is not preferred, because it positions the menu differently from how a native context
+    // menu would be positioned; it enforces aPosition for the top left corner even if this
+    // means that the menu will be displayed in a clipped fashion with scroll arrows.
+    [aMenu popUpMenuPositioningItem:nil atLocation:aPosition inView:nil];
+  }
+}
+
+@end

widget/cocoa/NativeMenuMac.h

@@ -31,8 +31,8 @@ class NativeMenuMac : public NativeMenu,
   // NativeMenu
   void ShowAsContextMenu(const mozilla::DesktopPoint& aPosition) override;
   bool Close() override;
-  void ActivateItem(dom::Element* aItemElement, Modifiers aModifiers,
-                    int16_t aButton, ErrorResult& aRv) override;
+  void ActivateItem(dom::Element* aItemElement, Modifiers aModifiers, int16_t aButton,
+                    ErrorResult& aRv) override;
   void OpenSubmenu(dom::Element* aMenuElement) override;
   void CloseSubmenu(dom::Element* aMenuElement) override;
   RefPtr<dom::Element> Element() override;
@@ -68,11 +68,6 @@ class NativeMenuMac : public NativeMenu,
  protected:
   virtual ~NativeMenuMac();
 
-  // Proceed with showing the menu after a call to ShowAsContextMenu.
-  // This is done from a runnable so that the nested event loop doesn't run
-  // during ShowAsContextMenu.
-  void OpenMenu(const mozilla::DesktopPoint& aPosition);
-
   // Find the deepest nsMenuX which contains aElement, only descending into open
   // menus.
   // Returns nullptr if the element was not found or if the menus on the path
@@ -80,11 +75,14 @@ class NativeMenuMac : public NativeMenu,
   RefPtr<nsMenuX> GetOpenMenuContainingElement(dom::Element* aElement);
 
   RefPtr<dom::Element> mElement;
-  RefPtr<CancelableRunnable> mOpenRunnable;
   RefPtr<nsMenuGroupOwnerX> mMenuGroupOwner;
   RefPtr<nsMenuX> mMenu;
   nsTArray<NativeMenu::Observer*> mObservers;
   NSStatusItem* mContainerStatusBarItem;
+
+  // Non-zero after a call to ShowAsContextMenu. Stores the handle from the
+  // MOZMenuOpeningCoordinator.
+  NSInteger mOpeningHandle = 0;
 };
 
 }  // namespace widget

widget/cocoa/NativeMenuMac.mm

@@ -4,14 +4,15 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #import <Cocoa/Cocoa.h>
-#include "mozilla/BasicEvents.h"
-#include "nsThreadUtils.h"
-#include "mozilla/dom/Document.h"
 
 #include "NativeMenuMac.h"
 
 #include "mozilla/Assertions.h"
+#include "mozilla/BasicEvents.h"
+#include "mozilla/dom/Document.h"
 #include "mozilla/dom/Element.h"
+
+#include "MOZMenuOpeningCoordinator.h"
 #include "nsISupports.h"
 #include "nsGkAtoms.h"
 #include "nsGkAtoms.h"
@@ -19,27 +20,12 @@
 #include "nsMenuItemX.h"
 #include "nsMenuUtilsX.h"
 #include "nsObjCExceptions.h"
-#include "mozilla/dom/Document.h"
+#include "nsThreadUtils.h"
 #include "PresShell.h"
 #include "nsCocoaUtils.h"
 #include "nsIFrame.h"
 #include "nsCocoaFeatures.h"
 
-#if !defined(MAC_OS_X_VERSION_10_14) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_14
-@interface NSApplication (NSApplicationAppearance)
-@property(readonly, strong) NSAppearance* effectiveAppearance NS_AVAILABLE_MAC(10_14);
-@end
-#endif
-
-#if !defined(MAC_OS_VERSION_11_0) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_VERSION_11_0
-@interface NSMenu (NSMenuAppearance)
-// In reality, NSMenu implements the NSAppearanceCustomization protocol, and picks up the appearance
-// property from that protocol. But we can't tack on protocol implementations, so we just declare
-// the property setter here.
-- (void)setAppearance:(NSAppearance*)appearance;
-@end
-#endif
-
 namespace mozilla {
 
 using dom::Element;
@@ -243,85 +229,21 @@ static NSView* NativeViewForContent(nsIContent* aContent) {
 void NativeMenuMac::ShowAsContextMenu(const mozilla::DesktopPoint& aPosition) {
   mMenu->PopupShowingEventWasSentAndApprovedExternally();
 
-  // Do the actual opening off of a runnable, so that this ShowAsContextMenu call does not spawn a
-  // nested event loop, which would be surprising to our callers.
-  mozilla::DesktopPoint position = aPosition;
-  RefPtr<NativeMenuMac> self = this;
-  mOpenRunnable = NS_NewCancelableRunnableFunction("NativeMenuMac::ShowAsContextMenu",
-                                                   [=]() { self->OpenMenu(position); });
-  NS_DispatchToCurrentThread(mOpenRunnable);
-}
-
-void NativeMenuMac::OpenMenu(const mozilla::DesktopPoint& aPosition) {
-  mOpenRunnable = nullptr;
-
-  // There are multiple ways to display an NSMenu as a context menu.
-  //
-  //  1. We can return the NSMenu from -[ChildView menuForEvent:] and the NSView will open it for
-  //     us.
-  //  2. We can call +[NSMenu popUpContextMenu:withEvent:forView:] inside a mouseDown handler with a
-  //     real mouse down event.
-  //  3. We can call +[NSMenu popUpContextMenu:withEvent:forView:] at a later time, with a real
-  //     mouse event that we stored earlier.
-  //  4. We can call +[NSMenu popUpContextMenu:withEvent:forView:] at any time, with a synthetic
-  //     mouse event that we create just for that purpose.
-  //  5. We can call -[NSMenu popUpMenuPositioningItem:atLocation:inView:] and it just takes a
-  //     position, not an event.
-  //
-  // 1-4 look the same, 5 looks different: 5 is made for use with NSPopUpButton, where the selected
-  // item needs to be shown at a specific position. If a tall menu is opened with a position close
-  // to the bottom edge of the screen, 5 results in a cropped menu with scroll arrows, even if the
-  // entire menu would fit on the screen, due to the positioning constraint.
-  // 1-2 only work if the menu contents are known synchronously during the call to menuForEvent or
-  // during the mouseDown event handler.
-  // NativeMenuMac::ShowAsContextMenu can be called at any time. It could be called during a
-  // menuForEvent call (during a "contextmenu" event handler), or during a mouseDown handler, or at
-  // a later time.
-  // The code below uses option 4 as the preferred option because it's the simplest: It works in all
-  // scenarios and it doesn't have the positioning drawbacks of option 5.
-
+  NSMenu* menu = mMenu->NativeNSMenu();
   NSView* view = NativeViewForContent(mMenu->Content());
-  NSMenu* nativeMenu = mMenu->NativeNSMenu();
-
-  if (@available(macOS 10.14, *)) {
-#if !defined(MAC_OS_VERSION_11_0) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_VERSION_11_0
-    if (nsCocoaFeatures::OnBigSurOrLater()) {
-#else
-    if (@available(macOS 11.0, *)) {
-#endif
-      // Make native context menus respect the NSApp appearance rather than the NSWindow appearance.
-      [nativeMenu setAppearance:NSApp.effectiveAppearance];
-    }
-  }
-
   NSPoint locationOnScreen = nsCocoaUtils::GeckoPointToCocoaPoint(aPosition);
-  if (view) {
-    // Create a synthetic event at the right location and open the menu [option 4].
-    NSPoint locationInWindow = nsCocoaUtils::ConvertPointFromScreen(view.window, locationOnScreen);
-    NSEvent* event = [NSEvent mouseEventWithType:NSEventTypeRightMouseDown
-                                        location:locationInWindow
-                                   modifierFlags:0
-                                       timestamp:[[NSProcessInfo processInfo] systemUptime]
-                                    windowNumber:view.window.windowNumber
-                                         context:nil
-                                     eventNumber:0
-                                      clickCount:1
-                                        pressure:0.0f];
-    [NSMenu popUpContextMenu:nativeMenu withEvent:event forView:view];
-  } else {
-    // Open the menu using popUpMenuPositioningItem:atLocation:inView: [option 5].
-    // This is not preferred, because it positions the menu differently from how a native context
-    // menu would be positioned; it enforces locationOnScreen for the top left corner even if this
-    // means that the menu will be displayed in a clipped fashion with scroll arrows.
-    [nativeMenu popUpMenuPositioningItem:nil atLocation:locationOnScreen inView:nil];
-  }
+
+  // Let the MOZMenuOpeningCoordinator do the actual opening, so that this ShowAsContextMenu call
+  // does not spawn a nested event loop, which would be surprising to our callers.
+  mOpeningHandle = [MOZMenuOpeningCoordinator.sharedInstance asynchronouslyOpenMenu:menu
+                                                                   atScreenPosition:locationOnScreen
+                                                                            forView:view];
 }
 
 bool NativeMenuMac::Close() {
-  if (mOpenRunnable) {
-    // The menu was trying to open, but this Close() call interrupted it.
-    mOpenRunnable->Cancel();
-    mOpenRunnable = nullptr;
+  if (mOpeningHandle) {
+    // In case the menu was trying to open, but this Close() call interrupted it, cancel opening.
+    [MOZMenuOpeningCoordinator.sharedInstance cancelAsynchronousOpening:mOpeningHandle];
   }
   return mMenu->Close();
 }

widget/cocoa/SDKDeclarations.h

@@ -76,6 +76,13 @@ typedef NS_ENUM(NSInteger, NSTitlebarSeparatorStyle) {
 @property NSTitlebarSeparatorStyle titlebarSeparatorStyle;
 @end
 
+@interface NSMenu (NSMenu11_0)
+// In reality, NSMenu implements the NSAppearanceCustomization protocol, and picks up the appearance
+// property from that protocol. But we can't tack on protocol implementations, so we just declare
+// the property setter here.
+- (void)setAppearance:(NSAppearance*)appearance;
+@end
+
 #endif
 
 #endif  // SDKDefines_h

widget/cocoa/moz.build

@@ -37,6 +37,7 @@ UNIFIED_SOURCES += [
     "AppearanceOverride.mm",
     "GfxInfo.mm",
     "MOZIconHelper.mm",
+    "MOZMenuOpeningCoordinator.mm",
     "NativeKeyBindings.mm",
     "NativeMenuMac.mm",
     "NativeMenuSupport.mm",