Bug 1131473 - crash in -[NativeMenuItemTarget menuItemHit:]. r=spohl

widget/cocoa/nsMenuBarX.mm

@@ -932,29 +932,12 @@ static BOOL gMenuItemsExecuteCommands = YES;
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
-  // menuGroupOwner below is an nsMenuBarX object, which we sometimes access
-  // after it's been deleted, causing crashes (see bug 704866 and bug 670914).
-  // To fix this "correctly", in nsMenuBarX::~nsMenuBarX() we'd need to
-  // iterate through every NSMenuItem in nsMenuBarX::mNativeMenu and its
-  // submenus, which might be quite time consuming.  (For every NSMenuItem
-  // that has a "representedObject" that's a MenuItemInfo object, we'd need
-  // need to null out its "menuGroupOwner" if it's the same as the nsMenuBarX
-  // object being destroyed.)  But if the nsMenuBarX object being destroyed
-  // corresponds to the currently focused window, it's likely that the
-  // nsMenuBarX destructor will null out sLastGeckoMenuBarPainted.  So we can
-  // probably eliminate most of these crashes if we use this variable being
-  // null as an indicator that we're likely to crash below when we dereference
-  // menuGroupOwner.
-  if (!nsMenuBarX::sLastGeckoMenuBarPainted) {
+  if (!gMenuItemsExecuteCommands) {
     return;
   }
 
   int tag = [sender tag];
 
-  if (!gMenuItemsExecuteCommands) {
-    return;
-  }
-
   nsMenuGroupOwnerX* menuGroupOwner = nullptr;
   nsMenuBarX* menuBar = nullptr;
   MenuItemInfo* info = [sender representedObject];

widget/cocoa/nsMenuGroupOwnerX.h

@@ -34,6 +34,7 @@ public:
   uint32_t RegisterForCommand(nsMenuItemX* aItem);
   void UnregisterCommand(uint32_t aCommandID);
   nsMenuItemX* GetMenuItemForCommandID(uint32_t inCommandID);
+  void AddMenuItemInfoToSet(MenuItemInfo* info);
 
   NS_DECL_ISUPPORTS
   NS_DECL_NSIMUTATIONOBSERVER
@@ -51,6 +52,12 @@ protected:
 
   // stores mapping of command IDs to menu objects
   nsDataHashtable<nsUint32HashKey, nsMenuItemX *> mCommandToMenuObjectTable;
+
+  // Stores references to all the MenuItemInfo objects created with weak
+  // references to us.  They may live longer than we do, so when we're
+  // destroyed we need to clear all their weak references.  This avoids
+  // crashes in -[NativeMenuItemTarget menuItemHit:].  See bug 1131473.
+  NSMutableSet* mInfoSet;
 };
 
 #endif // nsMenuGroupOwner_h_

widget/cocoa/nsMenuGroupOwnerX.mm

@@ -30,7 +30,8 @@ NS_IMPL_ISUPPORTS(nsMenuGroupOwnerX, nsIMutationObserver)
 
 
 nsMenuGroupOwnerX::nsMenuGroupOwnerX()
-: mCurrentCommandID(eCommand_ID_Last)
+: mCurrentCommandID(eCommand_ID_Last),
+  mInfoSet([NSMutableSet setWithCapacity:10])
 {
 }
 
@@ -38,6 +39,15 @@ nsMenuGroupOwnerX::nsMenuGroupOwnerX()
 nsMenuGroupOwnerX::~nsMenuGroupOwnerX()
 {
   MOZ_ASSERT(mContentToObserverTable.Count() == 0, "have outstanding mutation observers!\n");
+
+  // The MenuItemInfo objects in mInfoSet may live longer than we do.  So when
+  // we get destroyed we need to invalidate all their mMenuGroupOwner pointers.
+  NSEnumerator* counter = [mInfoSet objectEnumerator];
+  MenuItemInfo* info;
+  while ((info = (MenuItemInfo*) [counter nextObject])) {
+    [info setMenuGroupOwner:nil];
+  }
+  [mInfoSet release];
 }
 
 
@@ -239,3 +249,8 @@ nsMenuItemX* nsMenuGroupOwnerX::GetMenuItemForCommandID(uint32_t inCommandID)
   else
     return nullptr;
 }
+
+void nsMenuGroupOwnerX::AddMenuItemInfoToSet(MenuItemInfo* info)
+{
+  [mInfoSet addObject:info];
+}

widget/cocoa/nsMenuX.mm

@@ -62,7 +62,6 @@ int32_t nsMenuX::sIndexingMenuLevel = 0;
 - (id) initWithMenuGroupOwner:(nsMenuGroupOwnerX *)aMenuGroupOwner
 {
   if ((self = [super init]) != nil) {
-    mMenuGroupOwner = nullptr;
     [self setMenuGroupOwner:aMenuGroupOwner];
   }
   return self;
@@ -83,6 +82,9 @@ int32_t nsMenuX::sIndexingMenuLevel = 0;
 {
   // weak reference as the nsMenuGroupOwnerX owns all of its sub-objects
   mMenuGroupOwner = aMenuGroupOwner;
+  if (aMenuGroupOwner) {
+    aMenuGroupOwner->AddMenuItemInfoToSet(self);
+  }
 }
 
 @end