[Mac] Use object_destructInstance() if available.

chrome/browser/ui/cocoa/objc_zombie.mm

Index: chrome/browser/ui/cocoa/objc_zombie.mm
diff --git a/chrome/browser/ui/cocoa/objc_zombie.mm b/chrome/browser/ui/cocoa/objc_zombie.mm
index 31d2a62a9f4d6c1ddbd121963dc02c57e8c5cc32..452789cb29ceb8b744670cbb95fcb1087e10242c 100644
--- a/chrome/browser/ui/cocoa/objc_zombie.mm
+++ b/chrome/browser/ui/cocoa/objc_zombie.mm
@@ -33,12 +33,13 @@
 
 namespace {
 
-// |object_cxxDestruct()| is an Objective-C runtime function which
-// traverses the object's class tree for ".cxxdestruct" methods which
-// are run to call C++ destructors as part of |-dealloc|.  The
-// function is not public, so must be looked up using nlist.
+// Function which destroys the contents of an object without freeing
+// the object.  On 10.5 this is |object_cxxDestruct()|, which
+// traverses the class hierarchy to run the C++ destructors.  On 10.6
+// this is |objc_destructInstance()| which calls
+// |object_cxxDestruct()| and removes associative references.
 typedef void DestructFn(id obj);

[Mark Mentovai, 2011/05/28 13:20:46]

Technically the function pointer types are incompatible, because
objc_destructInstance returns void* and object_cxxDestruct returns void. Since
we don’t care about the return value, it should be safe to type both as void as
you’ve done here to keep the code simpler, and that ought to be fine on the
architectures we care about.

[Scott Hess - ex-Googler, 2011/05/31 20:22:30]

Augmented the comment to that end.

-DestructFn* g_object_cxxDestruct = NULL;
+DestructFn* g_objectDestruct = NULL;
 
 // The original implementation for |-[NSObject dealloc]|.
 IMP g_originalDeallocIMP = NULL;
@@ -70,9 +71,9 @@ typedef struct {
 
 ZombieRecord* g_zombies = NULL;
 
-// Lookup the private |object_cxxDestruct| function and return a
-// pointer to it.  Returns |NULL| on failure.
-DestructFn* LookupObjectCxxDestruct() {
+// Lookup the private object destruction function and return a pointer
+// to it.  Returns |NULL| on failure.
+DestructFn* LookupObjectDestruct() {
 #if ARCH_CPU_64_BITS

[Mark Mentovai, 2011/05/28 13:20:46]

Put this below the dlsym (but keep it before the nlist).

Since I doubt we’ll ever support 10.5/64, maybe also get rid of the
NOTIMPLEMENTED and resolve bug 44021.

[Scott Hess - ex-Googler, 2011/05/31 20:22:30]

You sure?  I think it's the right long-term decision, but the main reason I put
this in here is so that someone revisits this file when we finally make the
64-bit transition.  I didn't originally fix the problem as described because
without anyone actually exercising the code, it seemed likely that it would
bitrot.

[Mark Mentovai, 2011/05/31 21:00:57]

shess wrote:
Yeah. objc_destructInstance and the dlsym to get it will work on 10.6 x86_64. I
seriously doubt we’ll need to support 10.5 x86_64. If there’s another problem
with the code on x86_64, we’d have to find it anyway. I don’t see any reason to
declare up front that “this code might not work in 64-bit mode because it’s
untested” when the reality is that all of our code is untested in 64-bit mode
and thus might not work. Why single this file out as special?

The way it was before, it was a good idea to have the ARCH_CPU_64_BITS check,
because you knew for a fact that nlist wouldn’t work in 64-bit mode. But now, I
think it’ll be more of a distraction to the person doing the 64-bit porting
(probably me) than anything else. We don’t know for a fact that it won’t work,
and in fact, it most likely will work.

However, what I wouldn’t mind seeing would be wrapping all of the nlist code
inside #if ARCH_CPU_32_BITS.

[Scott Hess - ex-Googler, 2011/06/07 21:24:51]

Done.  AFAICT, nlist() isn't present for 64-bit, and nlist_64 is only present
for purposes of iterating the symbol table segment.  So supporting this for
64-bit 10.5 would require adding code to iterate the symbols ourselves.  Fun!

   // TODO(shess): Port to 64-bit.  I believe using struct nlist_64
   // will suffice.  http://crbug.com/44021 .
@@ -80,6 +81,10 @@ DestructFn* LookupObjectCxxDestruct() {
   return NULL;
 #endif
 
+  void* fn = dlsym(RTLD_DEFAULT, "objc_destructInstance");

[Mark Mentovai, 2011/05/29 22:02:31]

RTLD_DEFAULT kind of sucks for a few reasons. It’ll search all loaded libraries’
symbol tables to find a match (and we wouldn’t expect a match on 10.5, so it’d
search everything each time) and it defeats the “two-level” lookup that dyld
normally does when it binds symbols for us, subjecting us to possibly picking up
the wrong symbol here.

I think we should use a real handle from dlopen. The dlopen can either use the
path to libobjc.dylib determined by a dladdr call to look up something known to
be in libobjc, or can just hard-code the known path to libobjc.dylib.

[Scott Hess - ex-Googler, 2011/05/31 20:22:30]

Done.  I do not know what I'm doing in this area, though, so review thoroughly.

+  if (fn)
+    return (DestructFn*)fn;

[Scott Hess - ex-Googler, 2011/05/28 05:44:35]

I need to clean up the casting in this function, and I'll need to wait until I'm
at the office next week to test against 10.5.

[Scott Hess - ex-Googler, 2011/05/31 20:22:30]

OK, casting is so annoying that I'm not sure I'm doing things right.  Is
reinterpret_cast<>() the way to go, or am I missing something?

AFAICT, 10.5 still works.

+
   struct nlist nl[3];

[Mark Mentovai, 2011/05/29 22:02:31]

If we can’t find objc_destructInstance with dlsym and we’re NOT on 10.5, I think
we should LOG(WARNING) something. Otherwise, since we’re dealing with private
APIs, we might not know if things stop working as we intend in some future
system version.

[Scott Hess - ex-Googler, 2011/05/31 20:22:30]

Earlier when I put in the whitelist, I left it way up in BrowserCrApplication
because it felt wrong to have such dependencies way down here.  But your
suggestion isn't so much a change to operation as a diagnostic output...

Given the tightly-bound nature versus the runtime, it's probably reasonable to
have it failsafe.  So the nlist path would ONLY be used for 10.5, the dlsym path
for 10.6 with no fallback to nlist, and whatever is appropriate for future
versions.  WDYT?

Maybe with a histogram to track failures?

[Mark Mentovai, 2011/05/31 21:00:57]

shess wrote:
I think I can get behind that.

I’d just use the 10.6 path as “10.6 and up,” taking care to warn if you can’t
access the function you want, whether you used dlsym or nlist.
I doubt that’s necessary, but I won’t stop you if you want to.

   bzero(&nl, sizeof(nl));
 
@@ -109,12 +114,12 @@ void ZombieDealloc(id self, SEL _cmd) {
     return;
   }
 
-  // Use the original |-dealloc| if |object_cxxDestruct| was never
+  // Use the original |-dealloc| if |g_objectDestruct| was never
   // initialized, because otherwise C++ destructors won't be called.
   // This case should be impossible, but doing it wrong would cause
   // terrible problems.
-  DCHECK(g_object_cxxDestruct);
-  if (!g_object_cxxDestruct) {
+  DCHECK(g_objectDestruct);
+  if (!g_objectDestruct) {
     g_originalDeallocIMP(self, _cmd);
     return;
   }
@@ -124,7 +129,11 @@ void ZombieDealloc(id self, SEL _cmd) {
 
   // Destroy the instance by calling C++ destructors and clearing it
   // to something unlikely to work well if someone references it.
-  (*g_object_cxxDestruct)(self);
+  // NOTE(shess): |object_dispose()| will call this again when the
+  // zombie falls off the treadmill!  But by then |isa| will be a
+  // class without C++ destructors or associative references, so it
+  // won't hurt anything.

[Scott Hess - ex-Googler, 2011/05/28 05:44:35]

Sorry, I think this comment snuck over when I broke my git branch into two CLs.

[Mark Mentovai, 2011/05/28 13:20:46]

shess wrote:
That’s OK, it’s obvious that this patch isn’t based on the other, but when you
do rebase this one, this will show up as a change to the comment you wrote in
the other, because you can’t mention object_cxxDestruct here with this change as
you did there.

+  (*g_objectDestruct)(self);
   memset(self, '!', size);
 
   // If the instance is big enough, make it into a fat zombie and have
@@ -219,7 +228,7 @@ BOOL ZombieInit() {
 
   Class rootClass = [NSObject class];
 
-  g_object_cxxDestruct = LookupObjectCxxDestruct();
+  g_objectDestruct = LookupObjectDestruct();
   g_originalDeallocIMP =
       class_getMethodImplementation(rootClass, @selector(dealloc));
   // objc_getClass() so CrZombie doesn't need +class.
@@ -227,7 +236,7 @@ BOOL ZombieInit() {
   g_fatZombieClass = objc_getClass("CrFatZombie");
   g_fatZombieSize = class_getInstanceSize(g_fatZombieClass);
 
-  if (!g_object_cxxDestruct || !g_originalDeallocIMP ||
+  if (!g_objectDestruct || !g_originalDeallocIMP ||
       !g_zombieClass || !g_fatZombieClass)
     return NO;