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
--- 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;