Merge pull request facebookarchive#451 from eanagel/setDelegate-fix

appleguy · appleguy · commit dbb8c2e42166 · 2015-05-13T22:50:19.000-10:00
Fix a crash in setAsyncDelegate: that happens randomly (UIScrollViewAccessibility or NSProxy)
diff --git a/AsyncDisplayKit/ASCollectionView.mm b/AsyncDisplayKit/ASCollectionView.mm
@@ -78,11 +78,17 @@ - (instancetype)initWithTarget:(id<NSObject>)target interceptor:(ASCollectionVie
 
 - (BOOL)respondsToSelector:(SEL)aSelector
 {
+  ASDisplayNodeAssert(_target, @"target must not be nil"); // catch weak ref's being nilled early
+  ASDisplayNodeAssert(_interceptor, @"interceptor must not be nil");
+
   return (_isInterceptedSelector(aSelector) || [_target respondsToSelector:aSelector]);
 }
 
 - (id)forwardingTargetForSelector:(SEL)aSelector
 {
+  ASDisplayNodeAssert(_target, @"target must not be nil"); // catch weak ref's being nilled early
+  ASDisplayNodeAssert(_interceptor, @"interceptor must not be nil");
+
   if (_isInterceptedSelector(aSelector)) {
     return _interceptor;
   }
@@ -159,6 +165,12 @@ - (instancetype)initWithFrame:(CGRect)frame collectionViewLayout:(UICollectionVi
   return self;
 }
 
+-(void)dealloc {
+  // a little defense move here.
+  super.delegate  = nil;
+  super.dataSource = nil;
+}
+
 #pragma mark -
 #pragma mark Overrides.
 
@@ -189,13 +201,15 @@ - (void)setDelegate:(id<UICollectionViewDelegate>)delegate
 
 - (void)setAsyncDataSource:(id<ASCollectionViewDataSource>)asyncDataSource
 {
-  if (_asyncDataSource == asyncDataSource)
-    return;
+  // Note: It's common to check if the value hasn't changed and short-circuit but we aren't doing that here to handle
+  // the (common) case of nilling the asyncDataSource in the ViewController's dealloc. In this case our _asyncDataSource
+  // will return as nil (ARC magic) even though the _proxyDataSource still exists. It's really important to nil out
+  // super.dataSource in this case because calls to _ASTableViewProxy will start failing and cause crashes.
 
   if (asyncDataSource == nil) {
+    super.dataSource = nil;
     _asyncDataSource = nil;
     _proxyDataSource = nil;
-    super.dataSource = nil;
   } else {
     _asyncDataSource = asyncDataSource;
     _proxyDataSource = [[_ASCollectionViewProxy alloc] initWithTarget:_asyncDataSource interceptor:self];
@@ -205,13 +219,17 @@ - (void)setAsyncDataSource:(id<ASCollectionViewDataSource>)asyncDataSource
 
 - (void)setAsyncDelegate:(id<ASCollectionViewDelegate>)asyncDelegate
 {
-  if (_asyncDelegate == asyncDelegate)
-    return;
+  // Note: It's common to check if the value hasn't changed and short-circuit but we aren't doing that here to handle
+  // the (common) case of nilling the asyncDelegate in the ViewController's dealloc. In this case our _asyncDelegate
+  // will return as nil (ARC magic) even though the _proxyDelegate still exists. It's really important to nil out
+  // super.delegate in this case because calls to _ASTableViewProxy will start failing and cause crashes.
 
   if (asyncDelegate == nil) {
+    // order is important here, the delegate must be callable while nilling super.delegate to avoid random crashes
+    // in UIScrollViewAccessibility.
+    super.delegate = nil;
     _asyncDelegate = nil;
     _proxyDelegate = nil;
-    super.delegate = nil;
   } else {
     _asyncDelegate = asyncDelegate;
     _proxyDelegate = [[_ASCollectionViewProxy alloc] initWithTarget:_asyncDelegate interceptor:self];
diff --git a/AsyncDisplayKit/ASTableView.mm b/AsyncDisplayKit/ASTableView.mm
@@ -77,11 +77,17 @@ - (instancetype)initWithTarget:(id<NSObject>)target interceptor:(ASTableView *)i
 
 - (BOOL)respondsToSelector:(SEL)aSelector
 {
+  ASDisplayNodeAssert(_target, @"target must not be nil"); // catch weak ref's being nilled early
+  ASDisplayNodeAssert(_interceptor, @"interceptor must not be nil");
+
   return (_isInterceptedSelector(aSelector) || [_target respondsToSelector:aSelector]);
 }
 
 - (id)forwardingTargetForSelector:(SEL)aSelector
 {
+  ASDisplayNodeAssert(_target, @"target must not be nil"); // catch weak ref's being nilled early
+  ASDisplayNodeAssert(_interceptor, @"interceptor must not be nil");
+
   if (_isInterceptedSelector(aSelector)) {
     return _interceptor;
   }
@@ -159,6 +165,12 @@ - (instancetype)initWithFrame:(CGRect)frame style:(UITableViewStyle)style asyncD
   return self;
 }
 
+-(void)dealloc {
+  // a little defense move here.
+  super.delegate  = nil;
+  super.dataSource = nil;
+}
+
 #pragma mark -
 #pragma mark Overrides
 
@@ -175,13 +187,15 @@ - (void)setDelegate:(id<UITableViewDelegate>)delegate
 
 - (void)setAsyncDataSource:(id<ASTableViewDataSource>)asyncDataSource
 {
-  if (_asyncDataSource == asyncDataSource)
-    return;
+  // Note: It's common to check if the value hasn't changed and short-circuit but we aren't doing that here to handle
+  // the (common) case of nilling the asyncDataSource in the ViewController's dealloc. In this case our _asyncDataSource
+  // will return as nil (ARC magic) even though the _proxyDataSource still exists. It's really important to nil out
+  // super.dataSource in this case because calls to _ASTableViewProxy will start failing and cause crashes.
 
   if (asyncDataSource == nil) {
+    super.dataSource = nil;
     _asyncDataSource = nil;
     _proxyDataSource = nil;
-    super.dataSource = nil;
   } else {
     _asyncDataSource = asyncDataSource;
     _proxyDataSource = [[_ASTableViewProxy alloc] initWithTarget:_asyncDataSource interceptor:self];
@@ -191,13 +205,17 @@ - (void)setAsyncDataSource:(id<ASTableViewDataSource>)asyncDataSource
 
 - (void)setAsyncDelegate:(id<ASTableViewDelegate>)asyncDelegate
 {
-  if (_asyncDelegate == asyncDelegate)
-    return;
+  // Note: It's common to check if the value hasn't changed and short-circuit but we aren't doing that here to handle
+  // the (common) case of nilling the asyncDelegate in the ViewController's dealloc. In this case our _asyncDelegate
+  // will return as nil (ARC magic) even though the _proxyDelegate still exists. It's really important to nil out
+  // super.delegate in this case because calls to _ASTableViewProxy will start failing and cause crashes.
 
   if (asyncDelegate == nil) {
-    _asyncDelegate = nil;
-    _proxyDelegate = nil;
+    // order is important here, the delegate must be callable while nilling super.delegate to avoid random crashes
+    // in UIScrollViewAccessibility.
     super.delegate = nil;
+    _asyncDelegate = nil;
+    _proxyDelegate = nil; 
   } else {
     _asyncDelegate = asyncDelegate;
     _proxyDelegate = [[_ASTableViewProxy alloc] initWithTarget:asyncDelegate interceptor:self];
