Fix Case Where Node Is Deallocated While Visible (#2171)

* Attempt to reproduce supplementary crash

* Get closer with supplementary issue testing

* Alright! We have a repro!

* The investigation continues

* Fixed!

AsyncDisplayKit/ASDisplayNode.mm

@@ -1468,7 +1468,7 @@ static inline CATransform3D _calculateTransformFromReferenceToTarget(ASDisplayNo
   }
 
   ASDisplayNodeAssert(_flags.layerBacked, @"We shouldn't get called back here if there is no layer");
-  return (id<CAAction>)[NSNull null];
+  return (id)kCFNull;
 }
 
 #pragma mark - Managing the Node Hierarchy

AsyncDisplayKit/Details/_ASDisplayView.mm

@@ -78,16 +78,7 @@
   UIView *currentSuperview = self.superview;
   if (!currentSuperview && newSuperview) {
     self.keepalive_node = _node;
-  } else if (currentSuperview && !newSuperview) {
-    // Clearing keepalive_node may cause deallocation of the node.  In this case, __exitHierarchy may not have an opportunity (e.g. _node will be cleared
-    // by the time -didMoveToWindow occurs after this) to clear the Visible interfaceState, which we need to do before deallocation to meet an API guarantee.
-    if (_node.inHierarchy) {
-      [_node __exitHierarchy];
-    }
-    self.keepalive_node = nil;
   }
-    
-  ASDisplayNodeAssert(self.keepalive_node == nil || newSuperview != nil, @"Keepalive reference should not exist if there is no superview.");
   
   if (newSuperview) {
     ASDisplayNode *supernode = _node.supernode;
@@ -122,12 +113,21 @@
 
 - (void)didMoveToSuperview
 {
+  UIView *superview = self.superview;
+  if (superview == nil) {
+    // Clearing keepalive_node may cause deallocation of the node.  In this case, __exitHierarchy may not have an opportunity (e.g. _node will be cleared
+    // by the time -didMoveToWindow occurs after this) to clear the Visible interfaceState, which we need to do before deallocation to meet an API guarantee.
+    if (_node.inHierarchy) {
+      [_node __exitHierarchy];
+    }
+    self.keepalive_node = nil;
+  }
+
   ASDisplayNode *supernode = _node.supernode;
   ASDisplayNodeAssert(!supernode.isLayerBacked, @"Shouldn't be possible for superview's node to be layer-backed.");
   
   if (supernode) {
     ASDisplayNodeAssertTrue(_node.nodeLoaded);
-    UIView *superview = self.superview;
     BOOL supernodeLoaded = supernode.nodeLoaded;
     BOOL needsSupernodeRemoval = NO;
     

AsyncDisplayKitTests/ASCollectionViewTests.mm

@@ -348,6 +348,55 @@
   } completion:nil]);
 }
 
+/**
+ * https://github.com/facebook/AsyncDisplayKit/issues/2011
+ *
+ * If this ever becomes a pain to maintain, drop it. The underlying issue is tested by testThatLayerBackedSubnodesAreMarkedInvisibleBeforeDeallocWhenSupernodesViewIsRemovedFromHierarchyWhileBeingRetained
+ */
+- (void)testThatDisappearingSupplementariesWithLayerBackedNodesDontFailAssert
+{
+  UIWindow *window = [[UIWindow alloc] initWithFrame:[UIScreen mainScreen].bounds];
+  UICollectionViewLayout *layout = [[UICollectionViewFlowLayout alloc] init];
+  ASCollectionView *cv = [[ASCollectionView alloc] initWithFrame:window.bounds collectionViewLayout:layout];
+
+
+  __unused NSMutableSet *keepaliveNodes = [NSMutableSet set];
+  id dataSource = [OCMockObject niceMockForProtocol:@protocol(ASCollectionDataSource)];
+  static int nodeIdx = 0;
+  [[[dataSource stub] andDo:^(NSInvocation *invocation) {
+    __autoreleasing ASCellNode *suppNode = [[ASCellNode alloc] init];
+    int thisNodeIdx = nodeIdx++;
+    suppNode.name = [NSString stringWithFormat:@"Cell #%d", thisNodeIdx];
+    [keepaliveNodes addObject:suppNode];
+
+    ASDisplayNode *layerBacked = [[ASDisplayNode alloc] init];
+    layerBacked.layerBacked = YES;
+    layerBacked.name = [NSString stringWithFormat:@"Subnode #%d", thisNodeIdx];
+    [suppNode addSubnode:layerBacked];
+    [invocation setReturnValue:&suppNode];
+  }] collectionView:cv nodeForSupplementaryElementOfKind:UICollectionElementKindSectionHeader atIndexPath:OCMOCK_ANY];
+  [[[dataSource stub] andReturnValue:[NSNumber numberWithInteger:1]] numberOfSectionsInCollectionView:cv];
+  cv.asyncDataSource = dataSource;
+
+  id delegate = [OCMockObject niceMockForProtocol:@protocol(UICollectionViewDelegateFlowLayout)];
+  [[[delegate stub] andReturnValue:[NSValue valueWithCGSize:CGSizeMake(100, 100)]] collectionView:cv layout:OCMOCK_ANY referenceSizeForHeaderInSection:0];
+  cv.asyncDelegate = delegate;
+
+  [cv registerSupplementaryNodeOfKind:UICollectionElementKindSectionHeader];
+  [window addSubview:cv];
+
+  [window makeKeyAndVisible];
+
+  for (NSInteger i = 0; i < 2; i++) {
+    // NOTE: waitUntilAllUpdatesAreCommitted or reloadDataImmediately is not sufficient here!!
+    XCTestExpectation *done = [self expectationWithDescription:[NSString stringWithFormat:@"Reload #%td complete", i]];
+    [cv reloadDataWithCompletion:^{
+      [done fulfill];
+    }];
+    [self waitForExpectationsWithTimeout:1 handler:nil];
+  }
+
+}
 
 - (void)testThatNodeCalculatedSizesAreUpdatedBeforeFirstPrepareLayoutAfterRotation
 {

AsyncDisplayKitTests/ASDisplayNodeTests.m

@@ -1941,4 +1941,27 @@ static bool stringContainsPointer(NSString *description, id p) {
   XCTAssertEqual(contentsAfterRedisplay, node.contents);
 }
 
+// Underlying issue for: https://github.com/facebook/AsyncDisplayKit/issues/2011
+- (void)testThatLayerBackedSubnodesAreMarkedInvisibleBeforeDeallocWhenSupernodesViewIsRemovedFromHierarchyWhileBeingRetained
+{
+  UIWindow *window = [[UIWindow alloc] initWithFrame:[UIScreen mainScreen].bounds];
+
+  NS_VALID_UNTIL_END_OF_SCOPE UIView *nodeView = nil;
+  {
+    NS_VALID_UNTIL_END_OF_SCOPE ASDisplayNode *node = [[ASDisplayNode alloc] init];
+    nodeView = node.view;
+    node.name = @"Node";
+
+    NS_VALID_UNTIL_END_OF_SCOPE ASDisplayNode *subnode = [[ASDisplayNode alloc] init];
+    subnode.layerBacked = YES;
+    [node addSubnode:subnode];
+    subnode.name = @"Subnode";
+    
+    [window addSubview:nodeView];
+  }
+
+  // nodeView must continue to be retained across this call, but the nodes must not.
+  XCTAssertNoThrow([nodeView removeFromSuperview]);
+}
+
 @end