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 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..7c440437aca2 100644 --- a/widget/cocoa/nsMenuGroupOwnerX.mm +++ b/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]; +} 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