From e6368a182e2aec6ca613ed5e9d7677136880167e Mon Sep 17 00:00:00 2001
From: Steven Michaud <smichaud@pobox.com>
Date: Thu, 27 Aug 2015 15:54:15 -0500
Subject: [PATCH] Bug 1131473 - crash in -[NativeMenuItemTarget menuItemHit:].
 r=spohl

---
 widget/cocoa/nsMenuBarX.mm        | 19 +------------------
 widget/cocoa/nsMenuGroupOwnerX.h  |  7 +++++++
 widget/cocoa/nsMenuGroupOwnerX.mm | 15 +++++++++++++++
 widget/cocoa/nsMenuX.mm           |  4 +++-
 4 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/widget/cocoa/nsMenuBarX.mm b/widget/cocoa/nsMenuBarX.mm
index e8187cb5d0fe..e9e59d1f8d93 100644
--- a/widget/cocoa/nsMenuBarX.mm
+++ b/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];
diff --git a/widget/cocoa/nsMenuGroupOwnerX.h b/widget/cocoa/nsMenuGroupOwnerX.h
index 5972fa9ddc08..04b039f4042f 100644
--- a/widget/cocoa/nsMenuGroupOwnerX.h
+++ b/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_
diff --git a/widget/cocoa/nsMenuGroupOwnerX.mm b/widget/cocoa/nsMenuGroupOwnerX.mm
index c8a70ce56cfb..e071b146646d 100644
--- a/widget/cocoa/nsMenuGroupOwnerX.mm
+++ b/widget/cocoa/nsMenuGroupOwnerX.mm
@@ -32,12 +32,22 @@ NS_IMPL_ISUPPORTS(nsMenuGroupOwnerX, nsIMutationObserver)
 nsMenuGroupOwnerX::nsMenuGroupOwnerX()
 : mCurrentCommandID(eCommand_ID_Last)
 {
+  mInfoSet = [[NSMutableSet setWithCapacity:10] retain];
 }
 
 
 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];
+}
diff --git a/widget/cocoa/nsMenuX.mm b/widget/cocoa/nsMenuX.mm
index aacb13580f1b..b5d136a0109d 100644
--- a/widget/cocoa/nsMenuX.mm
+++ b/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