Fix crash on quit in nsMenuX::RemoveAll() on 10.5.X (work around Apple bug) -- 2nd attempt to check in. b=422827 r=josh sr=vlad
authorsmichaud@pobox.com
Thu, 27 Mar 2008 09:30:13 -0700
changeset 13630 980048d16491a7d5e3e0da27158a0ac6fce6a656
parent 13629 730713dd898f562e9c6c4f2ab2112ddfe526e71b
child 13631 d7bacd294fa1f2fa99744643672c26438ca0e95d
push idunknown
push userunknown
push dateunknown
reviewersjosh, vlad
bugs422827
milestone1.9pre
Fix crash on quit in nsMenuX::RemoveAll() on 10.5.X (work around Apple bug) -- 2nd attempt to check in. b=422827 r=josh sr=vlad
widget/src/cocoa/nsCocoaUtils.h
widget/src/cocoa/nsMenuX.mm
widget/src/cocoa/nsToolkit.h
widget/src/cocoa/nsToolkit.mm
--- a/widget/src/cocoa/nsCocoaUtils.h
+++ b/widget/src/cocoa/nsCocoaUtils.h
@@ -41,16 +41,37 @@
 #ifndef nsCocoaUtils_h_
 #define nsCocoaUtils_h_
 
 #import <Cocoa/Cocoa.h>
 
 #include "nsRect.h"
 #include "nsIWidget.h"
 
+// "Borrowed" in part from the QTKit framework's QTKitDefines.h.  This is
+// needed when building on OS X Tiger (10.4.X) or with a 10.4 SDK.  It won't
+// be used when building on Leopard (10.5.X) or higher (or with a 10.5 or
+// higher SDK).
+//
+// These definitions for NSInteger and NSUInteger are the 32-bit ones -- since
+// we assume we'll always be building 32-bit binaries when building on Tiger
+// (or with a 10.4 SDK).
+#ifndef NSINTEGER_DEFINED
+
+typedef int NSInteger;
+typedef unsigned int NSUInteger;
+
+#define NSIntegerMax    LONG_MAX
+#define NSIntegerMin    LONG_MIN
+#define NSUIntegerMax   ULONG_MAX
+
+#define NSINTEGER_DEFINED 1
+
+#endif  /* NSINTEGER_DEFINED */
+
 @interface NSApplication (Undocumented)
 
 // Present in all versions of OS X from (at least) 10.2.8 through 10.5.
 - (BOOL)_isRunningModal;
 - (BOOL)_isRunningAppModal;
 
 // It's sometimes necessary to explicitly remove a window from the "window
 // cache" in order to deactivate it.  The "window cache" is an undocumented
--- a/widget/src/cocoa/nsMenuX.mm
+++ b/widget/src/cocoa/nsMenuX.mm
@@ -48,16 +48,17 @@
 #include "prinrval.h"
 #include "nsIRollupListener.h"
 
 #include "nsMenuItemIconX.h"
 #include "nsIMenu.h"
 #include "nsIMenuBar.h"
 #include "nsIMenuItem.h"
 #include "nsToolkit.h"
+#include "nsCocoaUtils.h"
 
 #include "nsString.h"
 #include "nsReadableUtils.h"
 #include "nsUnicharUtils.h"
 #include "plstr.h"
 
 #include "nsINameSpaceManager.h"
 #include "nsWidgetAtoms.h"
@@ -74,32 +75,42 @@
 #include "nsIXPConnect.h"
 
 // externs defined in nsChildView.mm
 extern nsIRollupListener * gRollupListener;
 extern nsIWidget         * gRollupWidget;
 
 static PRBool gConstructingMenu = PR_FALSE;
 
+static PRBool gMenuMethodsSwizzled = PR_FALSE;
+
 // CIDs
 #include "nsWidgetsCID.h"
 static NS_DEFINE_CID(kMenuCID,     NS_MENU_CID);
 static NS_DEFINE_CID(kMenuItemCID, NS_MENUITEM_CID);
 
 NS_IMPL_ISUPPORTS1(nsMenuX, nsIMenu)
 
 
 nsMenuX::nsMenuX()
 : mVisibleItemsCount(0), mParent(nsnull), mMenuBar(nsnull), mMacMenuID(0), 
   mMacMenu(nil), mNativeMenuItem(nil), mIsEnabled(PR_TRUE),
   mDestroyHandlerCalled(PR_FALSE), mNeedsRebuild(PR_TRUE),
   mConstructed(PR_FALSE), mVisible(PR_TRUE), mXBLAttached(PR_FALSE)
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
+  if (nsToolkit::OnLeopardOrLater() && !gMenuMethodsSwizzled) {
+    nsToolkit::SwizzleMethods([NSMenu class], @selector(_addItem:toTable:),
+                              @selector(nsMenuX_NSMenu_addItem:toTable:), PR_TRUE);
+    nsToolkit::SwizzleMethods([NSMenu class], @selector(_removeItem:fromTable:),
+                              @selector(nsMenuX_NSMenu_removeItem:fromTable:), PR_TRUE);
+    gMenuMethodsSwizzled = PR_TRUE;
+  }
+
   mMenuDelegate = [[MenuDelegate alloc] initWithGeckoMenu:this];
     
   if (!nsMenuBarX::sNativeEventTarget)
     nsMenuBarX::sNativeEventTarget = [[NativeMenuItemTarget alloc] init];
 
   NS_OBJC_END_TRY_ABORT_BLOCK;
 }
 
@@ -1249,8 +1260,172 @@ static OSStatus InstallMyMenuEventHandle
       mHaveInstalledCarbonEvents = TRUE;
     }
   }
 
   NS_OBJC_END_TRY_ABORT_BLOCK;
 }
 
 @end
+
+
+// OS X Leopard (at least as of 10.5.2) has an obscure bug triggered by some
+// behavior that's present in Mozilla.org browsers but not (as best I can
+// tell) in Apple products like Safari.  (It's not yet clear exactly what this
+// behavior is.)
+//
+// The bug is that sometimes you crash on quit in nsMenuX::RemoveAll(), on a
+// call to [NSMenu removeItemAtIndex:].  The crash is caused by trying to
+// access a deleted NSMenuItem object (sometimes (perhaps always?) by trying
+// to send it a _setChangedFlags: message).  Though this object was deleted
+// some time ago, it remains registered as a potential target for a particular
+// key equivalent.  So when [NSMenu removeItemAtIndex:] removes the current
+// target for that same key equivalent, the OS tries to "activate" the
+// previous target.
+//
+// The underlying reason appears to be that NSMenu's _addItem:toTable: and
+// _removeItem:fromTable: methods (which are used to keep a hashtable of
+// registered key equivalents) don't properly "retain" and "release"
+// NSMenuItem objects as they are added to and removed from the hashtable.
+//
+// Our (hackish) workaround is to shadow the OS's hashtable with another
+// hastable of our own (gShadowKeyEquivDB), and use it to "retain" and
+// "release" NSMenuItem objects as needed.  This resolves bmo bugs 422287 and
+// 423669.  When (if) Apple fixes this bug, we can remove this workaround.
+
+static NSMutableDictionary *gShadowKeyEquivDB = nil;
+
+// Class for numerical-index keys in gShadowKeyEquivDB (each is the NSUInteger
+// equivalent of an NSMenuItem's address).  Using this kind of key is more
+// efficient and also less dangerous -- we won't get into trouble if an
+// NSMenuItem object contains invalid pointers (as it appears they sometimes
+// do).
+//
+// Note: NSUInteger is 64-bit safe, but below we must (if we want to be able
+// build on OS X 10.4.X) create IndexNumbers using a method that _isn't_
+// 64-bit safe -- [NSNumber numberWithUnsignedInt:(unsigned int)value].  Once
+// we start compiling 64-bit binaries, we will need to change this to
+// [NSNumber numberWithUnsignedInteger:(NSUInteger)value] (which is only
+// available starting with OS X 10.5).  We should get a compiler error if we
+// try to compile a 64-bit binary without making this change.
+
+@interface IndexNumber : NSNumber
+- (BOOL)isEqual:(id)anObject;
+@end
+
+@implementation IndexNumber
+
+// Ensure that hastable comparisons use an NSNumber object's value -- not its
+// address in memory.
+- (BOOL)isEqual:(id)anObject
+{
+  if ([anObject isKindOfClass:[NSNumber class]])
+    return [self isEqualToNumber:(NSNumber *)anObject];
+  return [super isEqual:anObject];
+}
+
+@end
+
+// Class for values in gShadowKeyEquivDB.
+
+@interface KeyEquivDBItem : NSObject
+{
+  NSMenuItem *mItem;
+  NSMutableIndexSet *mTables;
+}
+
+- (id)initWithItem:(NSMenuItem *)aItem table:(NSMapTable *)aTable;
+- (BOOL)hasTable:(NSMapTable *)aTable;
+- (int)addTable:(NSMapTable *)aTable;
+- (int)removeTable:(NSMapTable *)aTable;
+
+@end
+
+@implementation KeyEquivDBItem
+
+- (id)initWithItem:(NSMenuItem *)aItem table:(NSMapTable *)aTable
+{
+  if (!gShadowKeyEquivDB)
+    gShadowKeyEquivDB = [[NSMutableDictionary alloc] init];
+  self = [super init];
+  if (aItem && aTable) {
+    mTables = [[NSMutableIndexSet alloc] init];
+    mItem = [aItem retain];
+    [mTables addIndex:(NSUInteger)aTable];
+  } else {
+    mTables = nil;
+    mItem = nil;
+  }
+  return self;
+}
+
+- (void)dealloc
+{
+  if (mTables)
+    [mTables release];
+  if (mItem)
+    [mItem release];
+  [super dealloc];
+}
+
+- (BOOL)hasTable:(NSMapTable *)aTable
+{
+  return [mTables containsIndex:(NSUInteger)aTable];
+}
+
+// Does nothing if aTable (its index value) is already present in mTables.
+- (int)addTable:(NSMapTable *)aTable
+{
+  if (aTable)
+    [mTables addIndex:(NSUInteger)aTable];
+  return [mTables count];
+}
+
+- (int)removeTable:(NSMapTable *)aTable
+{
+  if (aTable)
+    [mTables removeIndex:(NSUInteger)aTable];
+  return [mTables count];
+}
+
+@end
+
+@interface NSMenu (MethodSwizzling)
++ (void)nsMenuX_NSMenu_addItem:(NSMenuItem *)aItem toTable:(NSMapTable *)aTable;
++ (void)nsMenuX_NSMenu_removeItem:(NSMenuItem *)aItem fromTable:(NSMapTable *)aTable;
+@end
+
+@implementation NSMenu (MethodSwizzling)
+
++ (void)nsMenuX_NSMenu_addItem:(NSMenuItem *)aItem toTable:(NSMapTable *)aTable
+{
+  if (aItem && aTable) {
+    IndexNumber *itemNumber = (IndexNumber *)
+      [NSNumber numberWithUnsignedInt:(NSUInteger)aItem];
+    KeyEquivDBItem *shadowItem = [gShadowKeyEquivDB objectForKey:itemNumber];
+    if (shadowItem) {
+      [shadowItem addTable:aTable];
+    } else {
+      shadowItem = [[KeyEquivDBItem alloc] initWithItem:aItem table:aTable];
+      [gShadowKeyEquivDB setObject:shadowItem forKey:itemNumber];
+      // Release after [NSMutableDictionary setObject:forKey:] retains it (so
+      // that it will get dealloced when removeObjectForKey: is called).
+      [shadowItem release];
+    }
+  }
+  [self nsMenuX_NSMenu_addItem:aItem toTable:aTable];
+}
+
++ (void)nsMenuX_NSMenu_removeItem:(NSMenuItem *)aItem fromTable:(NSMapTable *)aTable
+{
+  [self nsMenuX_NSMenu_removeItem:aItem fromTable:aTable];
+  if (aItem && aTable) {
+    IndexNumber *itemNumber = (IndexNumber *)
+      [NSNumber numberWithUnsignedInt:(NSUInteger)aItem];
+    KeyEquivDBItem *shadowItem = [gShadowKeyEquivDB objectForKey:itemNumber];
+    if (shadowItem && [shadowItem hasTable:aTable]) {
+      if (![shadowItem removeTable:aTable])
+        [gShadowKeyEquivDB removeObjectForKey:itemNumber];
+    }
+  }
+}
+
+@end
--- a/widget/src/cocoa/nsToolkit.h
+++ b/widget/src/cocoa/nsToolkit.h
@@ -61,17 +61,18 @@ public:
   // Returns the OS X version as returned from Gestalt(gestaltSystemVersion, ...)
   static long        OSXVersion();
   
   // Convenience functions to check the OS version
   static PRBool      OnLeopardOrLater();
   
   static void        PostSleepWakeNotification(const char* aNotification);
 
-  static nsresult    SwizzleMethods(Class aClass, SEL orgMethod, SEL posedMethod);
+  static nsresult    SwizzleMethods(Class aClass, SEL orgMethod, SEL posedMethod,
+                                    PRBool classMethods = PR_FALSE);
 
 protected:
 
   nsresult           RegisterForSleepWakeNotifcations();
   void               RemoveSleepWakeNotifcations();
 
   void               RegisterForAllProcessMouseEvents();
   void               UnregisterAllProcessMouseEventHandlers();
--- a/widget/src/cocoa/nsToolkit.mm
+++ b/widget/src/cocoa/nsToolkit.mm
@@ -430,22 +430,31 @@ PRBool nsToolkit::OnLeopardOrLater()
 //
 // Be aware that, if aClass doesn't have an orgMethod selector but one of its
 // superclasses does, the method substitution will (in effect) take place in
 // that superclass (rather than in aClass itself).  The substitution has
 // effect on the class where it takes place and all of that class's
 // subclasses.  In order for method swizzling to work properly, posedMethod
 // needs to be unique in the class where the substitution takes place and all
 // of its subclasses.
-nsresult nsToolkit::SwizzleMethods(Class aClass, SEL orgMethod, SEL posedMethod)
+nsresult nsToolkit::SwizzleMethods(Class aClass, SEL orgMethod, SEL posedMethod,
+                                   PRBool classMethods)
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
 
-  Method original = class_getInstanceMethod(aClass, orgMethod);
-  Method posed = class_getInstanceMethod(aClass, posedMethod);
+  Method original = nil;
+  Method posed = nil;
+
+  if (classMethods) {
+    original = class_getClassMethod(aClass, orgMethod);
+    posed = class_getClassMethod(aClass, posedMethod);
+  } else {
+    original = class_getInstanceMethod(aClass, orgMethod);
+    posed = class_getInstanceMethod(aClass, posedMethod);
+  }
 
   if (!original || !posed)
     return NS_ERROR_FAILURE;
 
   IMP aMethodImp = original->method_imp;
   original->method_imp = posed->method_imp;
   posed->method_imp = aMethodImp;