Fix crash when system retains an accessibility object.

The system might retain instances to BrowserAccessibilityCocoa
objects even after they're no longer used. Previously, we handled
this by having BrowserAccessibilityCocoa own BrowserAccessibilityMac
so that its underlying data would persist as long as the wrapper.
However, this led to crashes when the system calls methods that
explore the tree hierarchy, like GetLocalBoundsRect.

The proper fix is that BrowserAccessibilityCocoa should detach
from BrowserAccessibilityMac with the latter is deleted, and
then do a NULL check in any NSAccessibility protocol implementation
so that it won't crash when a deleted object is queried.

BUG=242944
R=dtseng@chromium.org

Review URL: https://codereview.chromium.org/17261008

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

4 files changed, 85 insertions, 18 deletions

diff --git a/content/browser/accessibility/browser_accessibility_cocoa.h b/content/browser/accessibility/browser_accessibility_cocoa.h
index 0d401a0..387c5b2 100644
--- a/content/browser/accessibility/browser_accessibility_cocoa.h
+++ b/content/browser/accessibility/browser_accessibility_cocoa.h
@@ -30,6 +30,11 @@
 - (id)initWithObject:(content::BrowserAccessibility*)accessibility
             delegate:(id<BrowserAccessibilityDelegateCocoa>)delegate;
 
+// Clear this object's pointer to the wrapped BrowserAccessibility object
+// because the wrapped object has been deleted, but this object may
+// persist if the system still has references to it.
+- (void)detach;
+
 // Invalidate children for a non-ignored ancestor (including self).
 - (void)childrenChanged;
 
diff --git a/content/browser/accessibility/browser_accessibility_cocoa.mm b/content/browser/accessibility/browser_accessibility_cocoa.mm
index a09e8dd..9d49767 100644
--- a/content/browser/accessibility/browser_accessibility_cocoa.mm
+++ b/content/browser/accessibility/browser_accessibility_cocoa.mm
@@ -344,15 +344,11 @@ NSDictionary* attributeToMethodNameMap = nil;
   return self;
 }
 
-// Deletes our associated BrowserAccessibilityMac.
-- (void)dealloc {
+- (void)detach {
   if (browserAccessibility_) {
     NSAccessibilityUnregisterUniqueIdForUIElement(self);
-    delete browserAccessibility_;
     browserAccessibility_ = NULL;
   }
-
-  [super dealloc];
 }
 
 - (NSString*)accessKey {
@@ -997,6 +993,9 @@ NSDictionary* attributeToMethodNameMap = nil;
 // Returns the accessibility value for the given attribute.  If the value isn't
 // supported this will return nil.
 - (id)accessibilityAttributeValue:(NSString*)attribute {
+  if (!browserAccessibility_)
+    return nil;
+
   SEL selector =
       NSSelectorFromString([self methodNameForAttribute:attribute]);
   if (selector)
@@ -1036,6 +1035,9 @@ NSDictionary* attributeToMethodNameMap = nil;
 // value isn't supported this will return nil.
 - (id)accessibilityAttributeValue:(NSString*)attribute
                      forParameter:(id)parameter {
+  if (!browserAccessibility_)
+    return nil;
+
   const std::vector<int32>& line_breaks = browserAccessibility_->line_breaks();
   int len = static_cast<int>(browserAccessibility_->value().size());
 
@@ -1143,6 +1145,9 @@ NSDictionary* attributeToMethodNameMap = nil;
 // Returns an array of parameterized attributes names that this object will
 // respond to.
 - (NSArray*)accessibilityParameterizedAttributeNames {
+  if (!browserAccessibility_)
+    return nil;
+
   if ([[self role] isEqualToString:NSAccessibilityTableRole] ||
       [[self role] isEqualToString:NSAccessibilityGridRole]) {
     return [NSArray arrayWithObjects:
@@ -1168,6 +1173,9 @@ NSDictionary* attributeToMethodNameMap = nil;
 
 // Returns an array of action names that this object will respond to.
 - (NSArray*)accessibilityActionNames {
+  if (!browserAccessibility_)
+    return nil;
+
   NSMutableArray* ret =
       [NSMutableArray arrayWithObject:NSAccessibilityShowMenuAction];
   NSString* role = [self role];
@@ -1190,6 +1198,9 @@ NSDictionary* attributeToMethodNameMap = nil;
 - (NSArray*)accessibilityArrayAttributeValues:(NSString*)attribute
                                         index:(NSUInteger)index
                                      maxCount:(NSUInteger)maxCount {
+  if (!browserAccessibility_)
+    return nil;
+
   NSArray* fullArray = [self accessibilityAttributeValue:attribute];
   if (!fullArray)
     return nil;
@@ -1207,12 +1218,18 @@ NSDictionary* attributeToMethodNameMap = nil;
 
 // Returns the count of the specified accessibility array attribute.
 - (NSUInteger)accessibilityArrayAttributeCount:(NSString*)attribute {
+  if (!browserAccessibility_)
+    return nil;
+
   NSArray* fullArray = [self accessibilityAttributeValue:attribute];
   return [fullArray count];
 }
 
 // Returns the list of accessibility attributes that this object supports.
 - (NSArray*)accessibilityAttributeNames {
+  if (!browserAccessibility_)
+    return nil;
+
   // General attributes.
   NSMutableArray* ret = [NSMutableArray arrayWithObjects:
       NSAccessibilityChildrenAttribute,
@@ -1348,6 +1365,9 @@ NSDictionary* attributeToMethodNameMap = nil;
 
 // Returns the index of the child in this objects array of children.
 - (NSUInteger)accessibilityGetIndexOf:(id)child {
+  if (!browserAccessibility_)
+    return nil;
+
   NSUInteger index = 0;
   for (BrowserAccessibilityCocoa* childToCheck in [self children]) {
     if ([child isEqual:childToCheck])
@@ -1360,6 +1380,9 @@ NSDictionary* attributeToMethodNameMap = nil;
 // Returns whether or not the specified attribute can be set by the
 // accessibility API via |accessibilitySetValue:forAttribute:|.
 - (BOOL)accessibilityIsAttributeSettable:(NSString*)attribute {
+  if (!browserAccessibility_)
+    return nil;
+
   if ([attribute isEqualToString:NSAccessibilityFocusedAttribute])
     return GetState(browserAccessibility_,
         AccessibilityNodeData::STATE_FOCUSABLE);
@@ -1380,12 +1403,18 @@ NSDictionary* attributeToMethodNameMap = nil;
 // Returns whether or not this object should be ignored in the accessibilty
 // tree.
 - (BOOL)accessibilityIsIgnored {
+  if (!browserAccessibility_)
+    return true;
+
   return [self isIgnored];
 }
 
 // Performs the given accessibilty action on the webkit accessibility object
 // that backs this object.
 - (void)accessibilityPerformAction:(NSString*)action {
+  if (!browserAccessibility_)
+    return;
+
   // TODO(feldstein): Support more actions.
   if ([action isEqualToString:NSAccessibilityPressAction])
     [delegate_ doDefaultAction:browserAccessibility_->renderer_id()];
@@ -1395,6 +1424,9 @@ NSDictionary* attributeToMethodNameMap = nil;
 
 // Returns the description of the given action.
 - (NSString*)accessibilityActionDescription:(NSString*)action {
+  if (!browserAccessibility_)
+    return nil;
+
   return NSAccessibilityActionDescription(action);
 }
 
@@ -1407,6 +1439,9 @@ NSDictionary* attributeToMethodNameMap = nil;
 
 // Sets the value for an accessibility attribute via the accessibility API.
 - (void)accessibilitySetValue:(id)value forAttribute:(NSString*)attribute {
+  if (!browserAccessibility_)
+    return;
+
   if ([attribute isEqualToString:NSAccessibilityFocusedAttribute]) {
     NSNumber* focusedNumber = value;
     BOOL focused = [focusedNumber intValue];
@@ -1424,8 +1459,12 @@ NSDictionary* attributeToMethodNameMap = nil;
 
 // Returns the deepest accessibility child that should not be ignored.
 // It is assumed that the hit test has been narrowed down to this object
-// or one of its children, so this will never return nil.
+// or one of its children, so this will never return nil unless this
+// object is invalid.
 - (id)accessibilityHitTest:(NSPoint)point {
+  if (!browserAccessibility_)
+    return nil;
+
   BrowserAccessibilityCocoa* hit = self;
   for (BrowserAccessibilityCocoa* child in [self children]) {
     NSPoint origin = [child origin];
diff --git a/content/browser/accessibility/browser_accessibility_mac.mm b/content/browser/accessibility/browser_accessibility_mac.mm
index 0725e91..11595a1 100644
--- a/content/browser/accessibility/browser_accessibility_mac.mm
+++ b/content/browser/accessibility/browser_accessibility_mac.mm
@@ -37,17 +37,14 @@ void BrowserAccessibilityMac::PreInitialize() {
 }
 
 void BrowserAccessibilityMac::NativeReleaseReference() {
-  if (browser_accessibility_cocoa_) {
-    BrowserAccessibilityCocoa* temp = browser_accessibility_cocoa_;
-    browser_accessibility_cocoa_ = nil;
-    // Relinquish ownership of the cocoa obj.
-    [temp release];
-    // At this point, other processes may have a reference to
-    // the cocoa object. When the retain count hits zero, it will
-    // destroy us in dealloc.
-    // For that reason, do *not* make any more calls here after
-    // as we might have been deleted.
-  }
+  // Detach this object from |browser_accessibility_cocoa_| so it
+  // no longer has a pointer to this object.
+  [browser_accessibility_cocoa_ detach];
+  // Now, release it - but at this point, other processes may have a
+  // reference to the cocoa object.
+  [browser_accessibility_cocoa_ release];
+  // Finally, it's safe to delete this since we've detached.
+  delete this;
 }
 
 bool BrowserAccessibilityMac::IsNative() const {
diff --git a/content/browser/accessibility/browser_accessibility_mac_unittest.mm b/content/browser/accessibility/browser_accessibility_mac_unittest.mm
index 30a6de9..292c628 100644
--- a/content/browser/accessibility/browser_accessibility_mac_unittest.mm
+++ b/content/browser/accessibility/browser_accessibility_mac_unittest.mm
@@ -58,6 +58,11 @@ class BrowserAccessibilityTest : public ui::CocoaTest {
  public:
   virtual void SetUp() {
     CocoaTest::SetUp();
+    RebuildAccessibilityTree();
+  }
+
+ protected:
+  void RebuildAccessibilityTree() {
     AccessibilityNodeData root;
     root.id = 1000;
     root.location.set_width(500);
@@ -90,7 +95,6 @@ class BrowserAccessibilityTest : public ui::CocoaTest {
         retain]);
   }
 
- protected:
   scoped_nsobject<MockAccessibilityDelegate> delegate_;
   scoped_nsobject<BrowserAccessibilityCocoa> accessibility_;
   scoped_ptr<BrowserAccessibilityManager> manager_;
@@ -136,4 +140,26 @@ TEST_F(BrowserAccessibilityTest, InvalidAttributeTest) {
   EXPECT_TRUE(shouldBeNil == nil);
 }
 
+TEST_F(BrowserAccessibilityTest, RetainedDetachedObjectsReturnNil) {
+  // Get the first child.
+  BrowserAccessibilityCocoa* retainedFirstChild =
+      [accessibility_ accessibilityHitTest:NSMakePoint(50, 50)];
+  EXPECT_NSEQ(@"Child1", [retainedFirstChild
+      accessibilityAttributeValue:NSAccessibilityTitleAttribute]);
+
+  // Retain it. This simulates what the system might do with an
+  // accessibility object.
+  [retainedFirstChild retain];
+
+  // Rebuild the accessibility tree, which should detach |retainedFirstChild|.
+  RebuildAccessibilityTree();
+
+  // Now any attributes we query should return nil.
+  EXPECT_EQ(nil, [retainedFirstChild
+      accessibilityAttributeValue:NSAccessibilityTitleAttribute]);
+
+  // Don't leak memory in the test.
+  [retainedFirstChild release];
+}
+
 }  // namespace content