[ASDisplayNode] Fix a crash in insertSubnode

[rcancro]

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();

The fix is to add another case to exiting early because our subnodeIndex is out of bounds of _subnodes. After this check:

  if (subnodeIndex > subnodesCount || subnodeIndex < 0) {
    ASDisplayNodeFailAssert(@"Cannot insert a subnode at index %ld. Count is %ld", (long)subnodeIndex, (long)subnodesCount);
    return;
  }

I've added a new check/early 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;
  }

[raycsh017]

Not adding subnode to the node hierarchy if it's already in it makes sense, approving.