From 4b9ee3c64de33e28c242df2e43d3e982938623a4 Mon Sep 17 00:00:00 2001 From: Michael Schneider Date: Thu, 21 Jul 2016 14:41:19 -0700 Subject: [PATCH 1/2] Don't crash if inserting a nil node --- AsyncDisplayKit/ASDisplayNode.mm | 39 ++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index 4dac0e4e5e..18e11365dd 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -1315,7 +1315,7 @@ static inline CATransform3D _calculateTransformFromReferenceToTarget(ASDisplayNo return (id)[NSNull null]; } -#pragma mark - +#pragma mark - Managing the Node Hierarchy static bool disableNotificationsForMovingBetweenParents(ASDisplayNode *from, ASDisplayNode *to) { @@ -1331,9 +1331,11 @@ static bool disableNotificationsForMovingBetweenParents(ASDisplayNode *from, ASD ASDisplayNodeAssertThreadAffinity(self); ASDN::MutexLocker l(__instanceLock__); + ASDisplayNodeAssert(subnode, @"Cannot insert a nil subnode"); ASDisplayNode *oldParent = subnode.supernode; - if (!subnode || subnode == self || oldParent == self) + if (!subnode || subnode == self || oldParent == self) { return; + } // Disable appearance methods during move between supernodes, but make sure we restore their state after we do our thing BOOL isMovingEquivalentParents = disableNotificationsForMovingBetweenParents(oldParent, self); @@ -1342,8 +1344,9 @@ static bool disableNotificationsForMovingBetweenParents(ASDisplayNode *from, ASD } [subnode removeFromSupernode]; - if (!_subnodes) + if (!_subnodes) { _subnodes = [[NSMutableArray alloc] init]; + } [_subnodes addObject:subnode]; @@ -1379,8 +1382,14 @@ static bool disableNotificationsForMovingBetweenParents(ASDisplayNode *from, ASD */ - (void)_insertSubnode:(ASDisplayNode *)subnode atSubnodeIndex:(NSInteger)subnodeIndex sublayerIndex:(NSInteger)sublayerIndex andRemoveSubnode:(ASDisplayNode *)oldSubnode { - if (subnodeIndex == NSNotFound) + if (subnodeIndex == NSNotFound) { return; + } + + ASDisplayNodeAssert(subnode, @"Cannot insert a nil subnode"); + if (!subnode) { + return; + } ASDisplayNode *oldParent = [subnode _deallocSafeSupernode]; // Disable appearance methods during move between supernodes, but make sure we restore their state after we do our thing @@ -1461,12 +1470,14 @@ static NSInteger incrementIfFound(NSInteger i) { ASDN::MutexLocker l(__instanceLock__); ASDisplayNodeAssert(subnode, @"Cannot insert a nil subnode"); - if (!subnode) + if (!subnode) { return; + } ASDisplayNodeAssert([below _deallocSafeSupernode] == self, @"Node to insert below must be a subnode"); - if ([below _deallocSafeSupernode] != self) + if ([below _deallocSafeSupernode] != self) { return; + } ASDisplayNodeAssert(_subnodes, @"You should have subnodes if you have a subnode"); @@ -1504,12 +1515,14 @@ static NSInteger incrementIfFound(NSInteger i) { ASDN::MutexLocker l(__instanceLock__); ASDisplayNodeAssert(subnode, @"Cannot insert a nil subnode"); - if (!subnode) + if (!subnode) { return; + } ASDisplayNodeAssert([above _deallocSafeSupernode] == self, @"Node to insert above must be a subnode"); - if ([above _deallocSafeSupernode] != self) + if ([above _deallocSafeSupernode] != self) { return; + } ASDisplayNodeAssert(_subnodes, @"You should have subnodes if you have a subnode"); @@ -1553,7 +1566,12 @@ static NSInteger incrementIfFound(NSInteger i) { NSString *reason = [NSString stringWithFormat:@"Cannot insert a subnode at index %zd. Count is %zd", idx, _subnodes.count]; @throw [NSException exceptionWithName:NSInvalidArgumentException reason:reason userInfo:nil]; } - + + ASDisplayNodeAssert(subnode, @"Cannot insert a nil subnode"); + if (!subnode) { + return; + } + NSInteger sublayerIndex = NSNotFound; // Account for potentially having other subviews @@ -1601,8 +1619,9 @@ static NSInteger incrementIfFound(NSInteger i) { // Don't call self.supernode here because that will retain/autorelease the supernode. This method -_removeSupernode: is often called while tearing down a node hierarchy, and the supernode in question might be in the middle of its -dealloc. The supernode is never messaged, only compared by value, so this is safe. // The particular issue that triggers this edge case is when a node calls -removeFromSupernode on a subnode from within its own -dealloc method. - if (!subnode || [subnode _deallocSafeSupernode] != self) + if (!subnode || [subnode _deallocSafeSupernode] != self) { return; + } [_subnodes removeObjectIdenticalTo:subnode]; From bbe2fe5f4c3b65d0e32e0b7e49e1a9a72b52270f Mon Sep 17 00:00:00 2001 From: Michael Schneider Date: Thu, 21 Jul 2016 14:54:39 -0700 Subject: [PATCH 2/2] Add and fix tests for adding a nil subnode --- AsyncDisplayKitTests/ASDisplayNodeTests.m | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/AsyncDisplayKitTests/ASDisplayNodeTests.m b/AsyncDisplayKitTests/ASDisplayNodeTests.m index e1efbe7bbc..9be86b49de 100644 --- a/AsyncDisplayKitTests/ASDisplayNodeTests.m +++ b/AsyncDisplayKitTests/ASDisplayNodeTests.m @@ -1127,7 +1127,7 @@ static inline BOOL _CGPointEqualToPointWithEpsilon(CGPoint point1, CGPoint point { ASDisplayNode *parent = [[[ASDisplayNode alloc] init] autorelease]; ASDisplayNode *nilNode = nil; - XCTAssertNoThrow([parent addSubnode:nilNode], @"Don't try to add nil, but we'll deal."); + XCTAssertThrows([parent addSubnode:nilNode], @"Don't try to add nil, but we'll deal with it in production, but throw in development."); XCTAssertNoThrow([parent addSubnode:parent], @"Not good, test that we recover"); XCTAssertEqual(0u, parent.subnodes.count, @"We shouldn't have any subnodes"); } @@ -1320,6 +1320,9 @@ static inline BOOL _CGPointEqualToPointWithEpsilon(CGPoint point1, CGPoint point XCTAssertEqual(3u, parent.subnodes.count, @"Should have the right subnode count"); XCTAssertEqualObjects(nilParent, d.supernode, @"d's parent is messed up"); + // Check insert a nil node + ASDisplayNode *nilNode = nil; + XCTAssertThrows([parent insertSubnode:nilNode atIndex:0], @"Should not allow insertion of nil node. We will throw in development and deal with it in production"); // Check insert at invalid index XCTAssertThrows([parent insertSubnode:d atIndex:NSNotFound], @"Should not allow insertion at invalid index");