Backed out changeset 55bea2798c68 (bug 1131473) for OSX crashes.
authorRyan VanderMeulen <ryanvm@gmail.com>
Thu, 27 Aug 2015 15:04:30 -0400
changeset 259691 10a4c5fbcca7d515fa287f89892fdd4c5e9a243e
parent 259690 55322eeb14257d0de3dcbc69a7335a9382bd45ce
child 259692 cb7008d633835fa982492646de3cc191293fc28a
push id29287
push userryanvm@gmail.com
push dateFri, 28 Aug 2015 01:31:40 +0000
treeherdermozilla-central@87e23922be37 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1131473
milestone43.0a1
backs out55bea2798c68e18837b1c3a6bf3503f721f00c08
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out changeset 55bea2798c68 (bug 1131473) for OSX crashes. CLOSED TREE
widget/cocoa/nsMenuBarX.mm
widget/cocoa/nsMenuGroupOwnerX.h
widget/cocoa/nsMenuGroupOwnerX.mm
widget/cocoa/nsMenuX.mm
--- a/widget/cocoa/nsMenuBarX.mm
+++ b/widget/cocoa/nsMenuBarX.mm
@@ -927,22 +927,39 @@ static BOOL gMenuItemsExecuteCommands = 
 
 @implementation NativeMenuItemTarget
 
 // called when some menu item in this menu gets hit
 -(IBAction)menuItemHit:(id)sender
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
-  if (!gMenuItemsExecuteCommands) {
+  // 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) {
     return;
   }
 
   int tag = [sender tag];
 
+  if (!gMenuItemsExecuteCommands) {
+    return;
+  }
+
   nsMenuGroupOwnerX* menuGroupOwner = nullptr;
   nsMenuBarX* menuBar = nullptr;
   MenuItemInfo* info = [sender representedObject];
 
   if (info) {
     menuGroupOwner = [info menuGroupOwner];
     if (!menuGroupOwner) {
       return;
--- a/widget/cocoa/nsMenuGroupOwnerX.h
+++ b/widget/cocoa/nsMenuGroupOwnerX.h
@@ -29,17 +29,16 @@ public:
   nsresult Create(nsIContent * aContent);
 
   void RegisterForContentChanges(nsIContent* aContent,
                                  nsChangeObserver* aMenuObject);
   void UnregisterForContentChanges(nsIContent* aContent);
   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
 
 protected:
   virtual ~nsMenuGroupOwnerX();
 
   nsChangeObserver* LookupContentChangeObserver(nsIContent* aContent);
@@ -47,17 +46,11 @@ protected:
   uint32_t  mCurrentCommandID;  // unique command id (per menu-bar) to
                                 // give to next item that asks
 
   // stores observers for content change notification
   nsDataHashtable<nsPtrHashKey<nsIContent>, nsChangeObserver *> mContentToObserverTable;
 
   // 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_
--- a/widget/cocoa/nsMenuGroupOwnerX.mm
+++ b/widget/cocoa/nsMenuGroupOwnerX.mm
@@ -25,34 +25,24 @@
 #include "nsINode.h"
 
 using namespace mozilla;
 
 NS_IMPL_ISUPPORTS(nsMenuGroupOwnerX, nsIMutationObserver)
 
 
 nsMenuGroupOwnerX::nsMenuGroupOwnerX()
-: mCurrentCommandID(eCommand_ID_Last),
-  mInfoSet([NSMutableSet setWithCapacity:10])
+: mCurrentCommandID(eCommand_ID_Last)
 {
 }
 
 
 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];
 }
 
 
 nsresult nsMenuGroupOwnerX::Create(nsIContent* aContent)
 {
   if (!aContent)
     return NS_ERROR_INVALID_ARG;
 
@@ -244,13 +234,8 @@ void nsMenuGroupOwnerX::UnregisterComman
 nsMenuItemX* nsMenuGroupOwnerX::GetMenuItemForCommandID(uint32_t inCommandID)
 {
   nsMenuItemX * result;
   if (mCommandToMenuObjectTable.Get(inCommandID, &result))
     return result;
   else
     return nullptr;
 }
-
-void nsMenuGroupOwnerX::AddMenuItemInfoToSet(MenuItemInfo* info)
-{
-  [mInfoSet addObject:info];
-}
--- a/widget/cocoa/nsMenuX.mm
+++ b/widget/cocoa/nsMenuX.mm
@@ -57,16 +57,17 @@ int32_t nsMenuX::sIndexingMenuLevel = 0;
 // Objective-C class used for representedObject
 //
 
 @implementation MenuItemInfo
 
 - (id) initWithMenuGroupOwner:(nsMenuGroupOwnerX *)aMenuGroupOwner
 {
   if ((self = [super init]) != nil) {
+    mMenuGroupOwner = nullptr;
     [self setMenuGroupOwner:aMenuGroupOwner];
   }
   return self;
 }
 
 - (void) dealloc
 {
   [self setMenuGroupOwner:nullptr];
@@ -77,19 +78,16 @@ int32_t nsMenuX::sIndexingMenuLevel = 0;
 {
   return mMenuGroupOwner;
 }
 
 - (void) setMenuGroupOwner:(nsMenuGroupOwnerX *)aMenuGroupOwner
 {
   // weak reference as the nsMenuGroupOwnerX owns all of its sub-objects
   mMenuGroupOwner = aMenuGroupOwner;
-  if (aMenuGroupOwner) {
-    aMenuGroupOwner->AddMenuItemInfoToSet(self);
-  }
 }
 
 @end
 
 
 //
 // nsMenuX
 //