Fix ReleasableInstanceName to account for the fact that the V property

attribute isn't always last, despite what the documentation says.

Add a CHECK that trips when a property's attributes indicates that it has
a named instance variable backing it but no such instance variable is found
in the object.

Beef up the Sesame Street unit test to give ReleasableInstanceName a more
thorough test.

BUG=none
TEST=none
Review URL: http://codereview.chromium.org/6902035

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@83054 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 234 insertions, 83 deletions

diff --git a/base/mac/objc_property_releaser.mm b/base/mac/objc_property_releaser.mm
index 879dee9..bd7a750 100644
--- a/base/mac/objc_property_releaser.mm
+++ b/base/mac/objc_property_releaser.mm
@@ -7,6 +7,8 @@
 #import <objc/runtime.h>
 #include <stdlib.h>
 
+#include <string>
+
 #include "base/logging.h"
 
 namespace base {
@@ -18,8 +20,8 @@ namespace {
 // if the property is marked "retain" or "copy". If the instance variable name
 // is not known (perhaps because it was not automatically associated with the
 // property by @synthesize) or if the property is not "retain" or "copy",
-// returns NULL.
-const char* ReleasableInstanceName(objc_property_t property) {
+// returns an empty string.
+std::string ReleasableInstanceName(objc_property_t property) {
   // TODO(mark): Starting in newer system releases, the Objective-C runtime
   // provides a function to break the property attribute string into
   // individual attributes (property_copyAttributeList), as well as a function
@@ -34,9 +36,17 @@ const char* ReleasableInstanceName(objc_property_t property) {
   // http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/ObjCRuntimeGuide/Articles/ocrtPropertyIntrospection.html#//apple_ref/doc/uid/TP40008048-CH101-SW6
   const char* property_attributes = property_getAttributes(property);
 
+  std::string instance_name;
   bool releasable = false;
   while (*property_attributes) {
-    switch (*property_attributes) {
+    char name = *property_attributes;
+
+    const char* value = ++property_attributes;
+    while (*property_attributes && *property_attributes != ',') {
+      ++property_attributes;
+    }
+
+    switch (name) {
       // It might seem intelligent to check the type ('T') attribute to verify
       // that it identifies an NSObject-derived type (the attribute value
       // begins with '@'.) This is a bad idea beacuse it fails to identify
@@ -55,22 +65,25 @@ const char* ReleasableInstanceName(objc_property_t property) {
       case '&':  // retain
         releasable = true;
         break;
-      case 'V':
-        // 'V' is specified as the last attribute to occur, so if releasable
-        // wasn't set to true already, it won't be set to true now. Since 'V'
-        // is last, its value up to the end of the attribute string is the
-        // name of the instance variable backing the property.
-        if (!releasable || !*++property_attributes) {
-          return NULL;
-        }
-        return property_attributes;
+      case 'V':  // instance variable name
+        // 'V' is specified as the last attribute to occur in the
+        // documentation, but empirically, it's not always the last. In
+        // GC-supported or GC-required code, the 'P' (GC-eligible) attribute
+        // occurs after 'V'.
+        instance_name.assign(value, property_attributes - value);
+        break;
     }
-    char previous_char = *property_attributes;
-    while (*++property_attributes && previous_char != ',') {
-      previous_char = *property_attributes;
+
+    if (*property_attributes) {
+      ++property_attributes;
     }
   }
-  return NULL;
+
+  if (releasable) {
+    return instance_name;
+  }
+
+  return std::string();
 }
 
 }  // namespace
@@ -78,7 +91,7 @@ const char* ReleasableInstanceName(objc_property_t property) {
 void ObjCPropertyReleaser::Init(id object, Class classy) {
   DCHECK(!object_);
   DCHECK(!class_);
-  DCHECK([object isKindOfClass:classy]);
+  CHECK([object isKindOfClass:classy]);
 
   object_ = object;
   class_ = classy;
@@ -95,11 +108,13 @@ void ObjCPropertyReleaser::ReleaseProperties() {
        property_index < property_count;
        ++property_index) {
     objc_property_t property = properties[property_index];
-    const char* instance_name = ReleasableInstanceName(property);
-    if (instance_name) {
+    std::string instance_name = ReleasableInstanceName(property);
+    if (!instance_name.empty()) {
       id instance_value = nil;
-      object_getInstanceVariable(object_, instance_name,
-                                 (void**)&instance_value);
+      Ivar instance_variable =
+          object_getInstanceVariable(object_, instance_name.c_str(),
+                                     (void**)&instance_value);
+      CHECK(instance_variable);
       [instance_value release];
     }
   }
diff --git a/base/mac/objc_property_releaser_unittest.mm b/base/mac/objc_property_releaser_unittest.mm
index d5fee58..04f13c2 100644
--- a/base/mac/objc_property_releaser_unittest.mm
+++ b/base/mac/objc_property_releaser_unittest.mm
@@ -11,8 +11,29 @@
 // "When I'm alone, I count myself."
 //   --Count von Count, http://www.youtube.com/watch?v=FKzszqa9WA4
 
+namespace {
+
 // The number of CountVonCounts outstanding.
-static int ah_ah_ah;
+int ah_ah_ah;
+
+// NumberHolder exists to exercise the property attribute string parser by
+// providing a named struct and an anonymous union.
+struct NumberHolder {
+  union {
+    long long sixty_four;
+    int thirty_two;
+    short sixteen;
+    char eight;
+  } what;
+  enum {
+    SIXTY_FOUR,
+    THIRTY_TWO,
+    SIXTEEN,
+    EIGHT
+  } how;
+};
+
+}  // namespace
 
 @interface CountVonCount : NSObject<NSCopying>
 
@@ -44,26 +65,48 @@ static int ah_ah_ah;
 
 @interface ObjCPropertyTestBase : NSObject {
  @private
-  CountVonCount* cvcBaseRetain_;
-  CountVonCount* cvcBaseCopy_;
-  CountVonCount* cvcBaseAssign_;
-  CountVonCount* cvcBaseNotProperty_;
+  CountVonCount* baseCvcRetain_;
+  CountVonCount* baseCvcCopy_;
+  CountVonCount* baseCvcAssign_;
+  CountVonCount* baseCvcNotProperty_;
+  CountVonCount* baseCvcNil_;
+  CountVonCount* baseCvcCustom_;
+  int baseInt_;
+  double baseDouble_;
+  void* basePointer_;
+  NumberHolder baseStruct_;
+
   base::mac::ObjCPropertyReleaser propertyReleaser_ObjCPropertyTestBase_;
 }
 
-@property(retain, nonatomic) CountVonCount* cvcBaseRetain;
-@property(copy, nonatomic) CountVonCount* cvcBaseCopy;
-@property(assign, nonatomic) CountVonCount* cvcBaseAssign;
+@property(retain, nonatomic) CountVonCount* baseCvcRetain;
+@property(copy, nonatomic) CountVonCount* baseCvcCopy;
+@property(assign, nonatomic) CountVonCount* baseCvcAssign;
+@property(retain, nonatomic) CountVonCount* baseCvcNil;
+@property(retain, nonatomic, getter=baseCustom, setter=setBaseCustom:)
+    CountVonCount* baseCvcCustom;
+@property(retain, nonatomic) CountVonCount* baseCvcDynamic;
+@property(assign, nonatomic) int baseInt;
+@property(assign, nonatomic) double baseDouble;
+@property(assign, nonatomic) void* basePointer;
+@property(assign, nonatomic) NumberHolder baseStruct;
 
-- (void)setCvcBaseNotProperty:(CountVonCount*)cvc;
+- (void)setBaseCvcNotProperty:(CountVonCount*)cvc;
 
 @end  // @interface ObjCPropertyTestBase
 
 @implementation ObjCPropertyTestBase
 
-@synthesize cvcBaseRetain = cvcBaseRetain_;
-@synthesize cvcBaseCopy = cvcBaseCopy_;
-@synthesize cvcBaseAssign = cvcBaseAssign_;
+@synthesize baseCvcRetain = baseCvcRetain_;
+@synthesize baseCvcCopy = baseCvcCopy_;
+@synthesize baseCvcAssign = baseCvcAssign_;
+@synthesize baseCvcNil = baseCvcNil_;
+@synthesize baseCvcCustom = baseCvcCustom_;
+@dynamic baseCvcDynamic;
+@synthesize baseInt = baseInt_;
+@synthesize baseDouble = baseDouble_;
+@synthesize basePointer = basePointer_;
+@synthesize baseStruct = baseStruct_;
 
 - (id)init {
   if ((self = [super init])) {
@@ -74,14 +117,14 @@ static int ah_ah_ah;
 }
 
 - (void)dealloc {
-  [cvcBaseNotProperty_ release];
+  [baseCvcNotProperty_ release];
   [super dealloc];
 }
 
-- (void)setCvcBaseNotProperty:(CountVonCount*)cvc {
-  if (cvc != cvcBaseNotProperty_) {
-    [cvcBaseNotProperty_ release];
-    cvcBaseNotProperty_ = [cvc retain];
+- (void)setBaseCvcNotProperty:(CountVonCount*)cvc {
+  if (cvc != baseCvcNotProperty_) {
+    [baseCvcNotProperty_ release];
+    baseCvcNotProperty_ = [cvc retain];
   }
 }
 
@@ -89,41 +132,86 @@ static int ah_ah_ah;
 
 @protocol ObjCPropertyTestProtocol
 
-@property(retain, nonatomic) CountVonCount* cvcProtoRetain;
-@property(copy, nonatomic) CountVonCount* cvcProtoCopy;
-@property(assign, nonatomic) CountVonCount* cvcProtoAssign;
+@property(retain, nonatomic) CountVonCount* protoCvcRetain;
+@property(copy, nonatomic) CountVonCount* protoCvcCopy;
+@property(assign, nonatomic) CountVonCount* protoCvcAssign;
+@property(retain, nonatomic) CountVonCount* protoCvcNil;
+@property(retain, nonatomic, getter=protoCustom, setter=setProtoCustom:)
+    CountVonCount* protoCvcCustom;
+@property(retain, nonatomic) CountVonCount* protoCvcDynamic;
+@property(assign, nonatomic) int protoInt;
+@property(assign, nonatomic) double protoDouble;
+@property(assign, nonatomic) void* protoPointer;
+@property(assign, nonatomic) NumberHolder protoStruct;
 
 @end  // @protocol ObjCPropertyTestProtocol
 
 @interface ObjCPropertyTestDerived
     : ObjCPropertyTestBase<ObjCPropertyTestProtocol> {
  @private
-  CountVonCount* cvcDerivedRetain_;
-  CountVonCount* cvcDerivedCopy_;
-  CountVonCount* cvcDerivedAssign_;
-  CountVonCount* cvcDerivedNotProperty_;
-  CountVonCount* cvcProtoRetain_;
-  CountVonCount* cvcProtoCopy_;
-  CountVonCount* cvcProtoAssign_;
+  CountVonCount* derivedCvcRetain_;
+  CountVonCount* derivedCvcCopy_;
+  CountVonCount* derivedCvcAssign_;
+  CountVonCount* derivedCvcNotProperty_;
+  CountVonCount* derivedCvcNil_;
+  CountVonCount* derivedCvcCustom_;
+  int derivedInt_;
+  double derivedDouble_;
+  void* derivedPointer_;
+  NumberHolder derivedStruct_;
+
+  CountVonCount* protoCvcRetain_;
+  CountVonCount* protoCvcCopy_;
+  CountVonCount* protoCvcAssign_;
+  CountVonCount* protoCvcNil_;
+  CountVonCount* protoCvcCustom_;
+  int protoInt_;
+  double protoDouble_;
+  void* protoPointer_;
+  NumberHolder protoStruct_;
+
   base::mac::ObjCPropertyReleaser propertyReleaser_ObjCPropertyTestDerived_;
 }
 
-@property(retain, nonatomic) CountVonCount* cvcDerivedRetain;
-@property(copy, nonatomic) CountVonCount* cvcDerivedCopy;
-@property(assign, nonatomic) CountVonCount* cvcDerivedAssign;
+@property(retain, nonatomic) CountVonCount* derivedCvcRetain;
+@property(copy, nonatomic) CountVonCount* derivedCvcCopy;
+@property(assign, nonatomic) CountVonCount* derivedCvcAssign;
+@property(retain, nonatomic) CountVonCount* derivedCvcNil;
+@property(retain, nonatomic, getter=derivedCustom, setter=setDerivedCustom:)
+    CountVonCount* derivedCvcCustom;
+@property(retain, nonatomic) CountVonCount* derivedCvcDynamic;
+@property(assign, nonatomic) int derivedInt;
+@property(assign, nonatomic) double derivedDouble;
+@property(assign, nonatomic) void* derivedPointer;
+@property(assign, nonatomic) NumberHolder derivedStruct;
 
-- (void)setCvcDerivedNotProperty:(CountVonCount*)cvc;
+- (void)setDerivedCvcNotProperty:(CountVonCount*)cvc;
 
 @end  // @interface ObjCPropertyTestDerived
 
 @implementation ObjCPropertyTestDerived
 
-@synthesize cvcDerivedRetain = cvcDerivedRetain_;
-@synthesize cvcDerivedCopy = cvcDerivedCopy_;
-@synthesize cvcDerivedAssign = cvcDerivedAssign_;
-@synthesize cvcProtoRetain = cvcProtoRetain_;
-@synthesize cvcProtoCopy = cvcProtoCopy_;
-@synthesize cvcProtoAssign = cvcProtoAssign_;
+@synthesize derivedCvcRetain = derivedCvcRetain_;
+@synthesize derivedCvcCopy = derivedCvcCopy_;
+@synthesize derivedCvcAssign = derivedCvcAssign_;
+@synthesize derivedCvcNil = derivedCvcNil_;
+@synthesize derivedCvcCustom = derivedCvcCustom_;
+@dynamic derivedCvcDynamic;
+@synthesize derivedInt = derivedInt_;
+@synthesize derivedDouble = derivedDouble_;
+@synthesize derivedPointer = derivedPointer_;
+@synthesize derivedStruct = derivedStruct_;
+
+@synthesize protoCvcRetain = protoCvcRetain_;
+@synthesize protoCvcCopy = protoCvcCopy_;
+@synthesize protoCvcAssign = protoCvcAssign_;
+@synthesize protoCvcNil = protoCvcNil_;
+@synthesize protoCvcCustom = protoCvcCustom_;
+@dynamic protoCvcDynamic;
+@synthesize protoInt = protoInt_;
+@synthesize protoDouble = protoDouble_;
+@synthesize protoPointer = protoPointer_;
+@synthesize protoStruct = protoStruct_;
 
 - (id)init {
   if ((self = [super init])) {
@@ -134,14 +222,14 @@ static int ah_ah_ah;
 }
 
 - (void)dealloc {
-  [cvcDerivedNotProperty_ release];
+  [derivedCvcNotProperty_ release];
   [super dealloc];
 }
 
-- (void)setCvcDerivedNotProperty:(CountVonCount*)cvc {
-  if (cvc != cvcDerivedNotProperty_) {
-    [cvcDerivedNotProperty_ release];
-    cvcDerivedNotProperty_ = [cvc retain];
+- (void)setDerivedCvcNotProperty:(CountVonCount*)cvc {
+  if (cvc != derivedCvcNotProperty_) {
+    [derivedCvcNotProperty_ release];
+    derivedCvcNotProperty_ = [cvc retain];
   }
 }
 
@@ -154,6 +242,7 @@ TEST(ObjCPropertyReleaserTest, SesameStreet) {
 
   // Assure a clean slate.
   EXPECT_EQ(0, ah_ah_ah);
+  EXPECT_EQ(1U, [test_object retainCount]);
 
   CountVonCount* baseAssign = [[CountVonCount alloc] init];
   CountVonCount* derivedAssign = [[CountVonCount alloc] init];
@@ -165,35 +254,82 @@ TEST(ObjCPropertyReleaserTest, SesameStreet) {
   {
     base::mac::ScopedNSAutoreleasePool pool;
 
-    test_object.cvcBaseRetain = [CountVonCount countVonCount];
-    test_object.cvcBaseCopy = [CountVonCount countVonCount];
-    test_object.cvcBaseAssign = baseAssign;
-    [test_object setCvcBaseNotProperty:[CountVonCount countVonCount]];
+    test_object.baseCvcRetain = [CountVonCount countVonCount];
+    test_object.baseCvcCopy = [CountVonCount countVonCount];
+    test_object.baseCvcAssign = baseAssign;
+    test_object.baseCvcCustom = [CountVonCount countVonCount];
+    [test_object setBaseCvcNotProperty:[CountVonCount countVonCount]];
 
-    // That added 3 objects, plus 1 more that was copied.
-    EXPECT_EQ(7, ah_ah_ah);
+    // That added 4 objects, plus 1 more that was copied.
+    EXPECT_EQ(8, ah_ah_ah);
 
-    test_object.cvcDerivedRetain = [CountVonCount countVonCount];
-    test_object.cvcDerivedCopy = [CountVonCount countVonCount];
-    test_object.cvcDerivedAssign = derivedAssign;
-    [test_object setCvcDerivedNotProperty:[CountVonCount countVonCount]];
+    test_object.derivedCvcRetain = [CountVonCount countVonCount];
+    test_object.derivedCvcCopy = [CountVonCount countVonCount];
+    test_object.derivedCvcAssign = derivedAssign;
+    test_object.derivedCvcCustom = [CountVonCount countVonCount];
+    [test_object setDerivedCvcNotProperty:[CountVonCount countVonCount]];
+
+    // That added 4 objects, plus 1 more that was copied.
+    EXPECT_EQ(13, ah_ah_ah);
+
+    test_object.protoCvcRetain = [CountVonCount countVonCount];
+    test_object.protoCvcCopy = [CountVonCount countVonCount];
+    test_object.protoCvcAssign = protoAssign;
+    test_object.protoCvcCustom = [CountVonCount countVonCount];
 
     // That added 3 objects, plus 1 more that was copied.
-    EXPECT_EQ(11, ah_ah_ah);
+    EXPECT_EQ(17, ah_ah_ah);
+  }
 
-    test_object.cvcProtoRetain = [CountVonCount countVonCount];
-    test_object.cvcProtoCopy = [CountVonCount countVonCount];
-    test_object.cvcProtoAssign = protoAssign;
+  // Now that the autorelease pool has been popped, the 3 objects that were
+  // copied when placed into the test object will have been deallocated.
+  EXPECT_EQ(14, ah_ah_ah);
+
+  // Make sure that the setters work and have the expected semantics.
+  test_object.baseCvcRetain = nil;
+  test_object.baseCvcCopy = nil;
+  test_object.baseCvcAssign = nil;
+  test_object.baseCvcCustom = nil;
+  test_object.derivedCvcRetain = nil;
+  test_object.derivedCvcCopy = nil;
+  test_object.derivedCvcAssign = nil;
+  test_object.derivedCvcCustom = nil;
+  test_object.protoCvcRetain = nil;
+  test_object.protoCvcCopy = nil;
+  test_object.protoCvcAssign = nil;
+  test_object.protoCvcCustom = nil;
+
+  // The CountVonCounts marked retain and copy should have been deallocated.
+  // Those marked assign should not have been. The only ones that should exist
+  // now are the ones marked "assign" and the ones held in non-property
+  // instance variables.
+  EXPECT_EQ(5, ah_ah_ah);
+
+  {
+    base::mac::ScopedNSAutoreleasePool pool;
 
-    // That added 2 objects, plus 1 more that was copied.
-    EXPECT_EQ(14, ah_ah_ah);
+    // Put things back to how they were.
+    test_object.baseCvcRetain = [CountVonCount countVonCount];
+    test_object.baseCvcCopy = [CountVonCount countVonCount];
+    test_object.baseCvcAssign = baseAssign;
+    test_object.baseCvcCustom = [CountVonCount countVonCount];
+    test_object.derivedCvcRetain = [CountVonCount countVonCount];
+    test_object.derivedCvcCopy = [CountVonCount countVonCount];
+    test_object.derivedCvcAssign = derivedAssign;
+    test_object.derivedCvcCustom = [CountVonCount countVonCount];
+    test_object.protoCvcRetain = [CountVonCount countVonCount];
+    test_object.protoCvcCopy = [CountVonCount countVonCount];
+    test_object.protoCvcAssign = protoAssign;
+    test_object.protoCvcCustom = [CountVonCount countVonCount];
+
+    // 9 more CountVonCounts, 3 of which were copied.
+    EXPECT_EQ(17, ah_ah_ah);
   }
 
-  // Now that the autorelease pool has been popped, there should be 11
-  // CountVonCounts. The ones that were copied to place into the test objects
-  // will now have been deallocated.
-  EXPECT_EQ(11, ah_ah_ah);
+  // Now that the autorelease pool has been popped, the 3 copies are gone.
+  EXPECT_EQ(14, ah_ah_ah);
 
+  // Releasing the test object should get rid of everything that it owns.
   [test_object release];
 
   // The property releaser should have released all of the CountVonCounts