[ASDisplayNode] Fix a crash in insertSubnode (#2122)

* [ASDisplayNode] Fix a crash in insertSubnode

If a node is already a subnode to a supernode, Inserting it again can lead to a crash.

Here is a simple repro of the crash:
```
  ASDisplayNode *subnode = [[ASDisplayNode alloc] init];
  ASDisplayNode *supernode = [[ASDisplayNode alloc] init];

  [supernode addSubnode:subnode];
  // Crash on next line
  [supernode insertSubnode:subnode atIndex:1];
```

The issue is that all the checks around subnode array boundaries are done BEFORE `subnode` is removed from its `supernode`. If it happens that the `supernode` is self, then removing the `subnode` causes all our index checks to no longer be valid.

Here is the relevant code:

```
  __instanceLock__.lock();
    NSUInteger subnodesCount = _subnodes.count;
  __instanceLock__.unlock();
   ////// Here we check our indexes
  if (subnodeIndex > subnodesCount || subnodeIndex < 0) {
    ASDisplayNodeFailAssert(@"Cannot insert a subnode at index %ld. Count is %ld", (long)subnodeIndex, (long)subnodesCount);
    return;
  }

…

  ///////// Here our indexes could invalidate if self subnode’s supernode
  [subnode removeFromSupernode];
  [oldSubnode removeFromSupernode];

  __instanceLock__.lock();
    if (_subnodes == nil) {
      _subnodes = [[NSMutableArray alloc] init];
    }
    ////// Here would can crash if our index is too big
    [_subnodes insertObject:subnode atIndex:subnodeIndex];
    _cachedSubnodes = nil;
  __instanceLock__.unlock();
```

* add a separate check for this special case

Author: rcancro

Source/ASDisplayNode.mm

@@ -2114,11 +2114,18 @@ - (void)_insertSubnode:(ASDisplayNode *)subnode atSubnodeIndex:(NSInteger)subnod
   __instanceLock__.lock();
     NSUInteger subnodesCount = _subnodes.count;
   __instanceLock__.unlock();
+
   if (subnodeIndex > subnodesCount || subnodeIndex < 0) {
     ASDisplayNodeFailAssert(@"Cannot insert a subnode at index %ld. Count is %ld", (long)subnodeIndex, (long)subnodesCount);
     return;
   }
-  
+
+  // Check if subnode is already a in _subnodes. If so make sure the subnodeIndex will not be out of bounds once we call [subnode removeFromSupernode]
+  if (subnode.supernode == self && subnodeIndex >= subnodesCount) {
+    ASDisplayNodeFailAssert(@"node %@ is already a subnode of %@. index %ld will be out of bounds once we call [subnode removeFromSupernode]. This can be caused by using automaticallyManagesSubnodes while also calling addSubnode explicitly.", subnode, self, subnodeIndex);
+    return;
+  }
+
   // Disable appearance methods during move between supernodes, but make sure we restore their state after we do our thing
   ASDisplayNode *oldParent = subnode.supernode;
   BOOL disableNotifications = shouldDisableNotificationsForMovingBetweenParents(oldParent, self);

Tests/ASDisplayNodeTests.mm

@@ -2783,4 +2783,15 @@ - (void)testPlaceholder
   XCTAssertTrue(hasPlaceholderLayer);
 }
 
+- (void)testInsertExistingSubnode
+{
+  DeclareNodeNamed(parent);
+  DeclareNodeNamed(child);
+  
+  [parent addSubnode:child];
+  // Previously this would cause a crash. We now protect inserting an already existing subnode out of bounds
+  XCTAssertThrows([parent insertSubnode:child atIndex:1]);
+  XCTAssertTrue(parent.subnodes.count == 1);
+}
+
 @end