[ASThread][ASDisplayNode] Detect and avoid deadlocks caused by upward lock gathering in didEnter/Exit states (#2764)

* Implement mutex ownership and use it to check potential upward lock gathering

* Don't hold instance lock and call willEnterHierarchy/didExitHierarchy of ASDisplayNode
- This can cause deadlocks (e.g #2605) if subsequent methods, that are implemented by developers, walk up the node tree.
- This is a way to keep the optimizations introduced in 9e87813 while making sure the locking situation is a bit safer.

* More lock ownership assertions in ASDisplayNode

* Document main thread contract  of -clearContents

* ASNetworkImageNode shoud not call setNeedsPreload while holding instance lock
- This helps to avoid potentially deadlocks caused if the node (esp in case it's a subclass of ASNetworkImageNode) walks up the tree in didEnterPreloadState, for example to build logging context.

* ASVideoNode should not call setNeedsPreload while holding instance lock
- This helps to avoid potentially deadlocks caused if the node (esp. if it's a subclass of ASVideoNode) walks up the tree in didEnterPreloadState, for example to build logging context.

* Don't hold instance lock for the entire insert subnode operation
- The recursive lock should not be held throughout `_insertSubnode:atSubnodeIndex:sublayerIndex:andRemoveSubnode:`. The method instead should manage the lock itself and acquire it as shortly as possible. The reason is that this method calls many methods outside the scope of `self`. `[subnode __setSupernode:self]` is especially troublesome because it causes the subnode to migrate to new hierarchy and interface states, which triggers `didEnter/Exit(.*)State` methods. These methods are meant to be overriden by subclasses. Thus they might walk up the node tree and potentially cause deadlocks, or they perform expensive tasks and cause the lock to be held for too long.
- Other methods that call this method should release the lock before doing so.

* Lock getter and setter of `synchronous` flag

* Address comment in ASVideoNode

* Add main thread assertions to methods that change asset and assetURL of ASVideoNode

* Explain CHECK_LOCKING_SAFETY flag

* More thread and locking assertions in ASVideNode
- It's not safe to call `-[subnode __setSupernode:self]` while holding instance lock of soon-to-be supernode (e.g `self`).
- It's also not safe to call `[subnode __setSupernode:nil]` while holding the instance lock of the old supernode (e.g `self`).
- did(Enter|Exit)(.*)State methods are always called on main. Add main thread assertions to indicate that.

* Minor change in explanation of CHECK_LOCKING_SAFETY flag

AsyncDisplayKit/ASDisplayNode+Subclasses.h

@@ -327,7 +327,7 @@ NS_ASSUME_NONNULL_BEGIN
  * Provides an opportunity to clear backing store and other memory-intensive intermediates, such as text layout managers
  * on the current node.
  *
- * @discussion Called by -recursivelyClearContents. Base class implements self.contents = nil, clearing any backing
+ * @discussion Called by -recursivelyClearContents. Always called on main thread. Base class implements self.contents = nil, clearing any backing
  * store, for asynchronous regeneration when needed.
  */
 - (void)clearContents ASDISPLAYNODE_REQUIRES_SUPER;

AsyncDisplayKit/ASDisplayNode.mm

@@ -40,6 +40,19 @@
   #define AS_DEDUPE_LAYOUT_SPEC_TREE 1
 #endif
 
+/**
+ * Assert if the current thread owns a mutex.
+ * This assertion is useful when you want to indicate and enforce the locking policy/expectation of methods.
+ * To determine when and which methods acquired a (recursive) mutex (to debug deadlocks, for example), 
+ * put breakpoints at some of these assertions. When the breakpoints hit, walk through stack trace frames 
+ * and check ownership count of the mutex.
+ */
+#if CHECK_LOCKING_SAFETY
+  #define ASDisplayNodeAssertLockUnownedByCurrentThread(lock) ASDisplayNodeAssertFalse(lock.ownedByCurrentThread());
+#else
+  #define ASDisplayNodeAssertLockUnownedByCurrentThread(lock)
+#endif
+
 NSInteger const ASDefaultDrawingPriority = ASDefaultTransactionPriority;
 NSString * const ASRenderingEngineDidDisplayScheduledNodesNotification = @"ASRenderingEngineDidDisplayScheduledNodes";
 NSString * const ASRenderingEngineDidDisplayNodesScheduledBeforeTimestamp = @"ASRenderingEngineDidDisplayNodesScheduledBeforeTimestamp";
@@ -760,11 +773,13 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c)
 
 - (BOOL)isSynchronous
 {
+  ASDN::MutexLocker l(__instanceLock__);
   return _flags.synchronous;
 }
 
 - (void)setSynchronous:(BOOL)flag
 {
+  ASDN::MutexLocker l(__instanceLock__);
   _flags.synchronous = flag;
 }
 
@@ -1881,9 +1896,9 @@ static inline CATransform3D _calculateTransformFromReferenceToTarget(ASDisplayNo
 
 ASDISPLAYNODE_INLINE bool shouldDisableNotificationsForMovingBetweenParents(ASDisplayNode *from, ASDisplayNode *to) {
   if (!from || !to) return NO;
-  if (from->_flags.synchronous) return NO;
-  if (to->_flags.synchronous) return NO;
-  if (from->_flags.isInHierarchy != to->_flags.isInHierarchy) return NO;
+  if (from.isSynchronous) return NO;
+  if (to.isSynchronous) return NO;
+  if (from.isInHierarchy != to.isInHierarchy) return NO;
   return YES;
 }
 
@@ -1899,12 +1914,11 @@ ASDISPLAYNODE_INLINE BOOL canUseViewAPI(ASDisplayNode *node, ASDisplayNode *subn
 
 /// Returns if node is a member of a rasterized tree
 ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
-  return (node->_flags.shouldRasterizeDescendants || (node->_hierarchyState & ASHierarchyStateRasterized));
+  return (node.shouldRasterizeDescendants || (node.hierarchyState & ASHierarchyStateRasterized));
 }
 
 /*
  * Central private helper method that should eventually be called if submethods add, insert or replace subnodes
- * You must hold __instanceLock__ to call this.
  * This method is called with thread affinity.
  *
  * @param subnode       The subnode to insert
@@ -1915,6 +1929,8 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
  */
 - (void)_insertSubnode:(ASDisplayNode *)subnode atSubnodeIndex:(NSInteger)subnodeIndex sublayerIndex:(NSInteger)sublayerIndex andRemoveSubnode:(ASDisplayNode *)oldSubnode
 {
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
+  
   if (subnode == nil || subnode == self) {
     ASDisplayNodeFailAssert(@"Cannot insert a nil subnode or self as subnode");
     return;
@@ -1925,8 +1941,11 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
     return;
   }
   
-  if (subnodeIndex > _subnodes.count || subnodeIndex < 0) {
-    ASDisplayNodeFailAssert(@"Cannot insert a subnode at index %zd. Count is %zd", subnodeIndex, _subnodes.count);
+  __instanceLock__.lock();
+    NSUInteger subnodesCount = _subnodes.count;
+  __instanceLock__.unlock();
+  if (subnodeIndex > subnodesCount || subnodeIndex < 0) {
+    ASDisplayNodeFailAssert(@"Cannot insert a subnode at index %zd. Count is %zd", subnodeIndex, subnodesCount);
     return;
   }
   
@@ -1940,11 +1959,12 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
   [subnode _removeFromSupernode];
   [oldSubnode _removeFromSupernode];
   
-  if (_subnodes == nil) {
-    _subnodes = [[NSMutableArray alloc] init];
-  }
-    
-  [_subnodes insertObject:subnode atIndex:subnodeIndex];
+  __instanceLock__.lock();
+    if (_subnodes == nil) {
+      _subnodes = [[NSMutableArray alloc] init];
+    }
+    [_subnodes insertObject:subnode atIndex:subnodeIndex];
+  __instanceLock__.unlock();
   
   // This call will apply our .hierarchyState to the new subnode.
   // If we are a managed hierarchy, as in ASCellNode trees, it will also apply our .interfaceState.
@@ -1958,7 +1978,7 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
         [node __unloadNode];
       }
     });
-    if (_flags.isInHierarchy) {
+    if (self.isInHierarchy) {
       [subnode __enterHierarchy];
     }
   } else if (self.nodeLoaded) {
@@ -1974,7 +1994,6 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
 
 /*
  * Inserts the view or layer of the given node at the given index
- * You must hold __instanceLock__ to call this.
  *
  * @param subnode       The subnode to insert
  * @param idx           The index in _view.subviews or _layer.sublayers at which to insert the subnode.view or
@@ -1990,6 +2009,9 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
     return;
   }
   
+  // Because the view and layer can only be created and destroyed on Main, that is also the only thread
+  // where the view and layer can change. We can avoid locking.
+
   // If we can use view API, do. Due to an apple bug, -insertSubview:atIndex: actually wants a LAYER index, which we pass in
   if (canUseViewAPI(self, subnode)) {
     [_view insertSubview:subnode.view atIndex:idx];
@@ -2009,7 +2031,6 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
 - (void)_addSubnode:(ASDisplayNode *)subnode
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASDN::MutexLocker l(__instanceLock__);
   
   ASDisplayNodeAssert(subnode, @"Cannot insert a nil subnode");
     
@@ -2019,7 +2040,15 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
     return;
   }
 
-  [self _insertSubnode:subnode atSubnodeIndex:_subnodes.count sublayerIndex:_layer.sublayers.count andRemoveSubnode:nil];
+  NSUInteger subnodesIndex;
+  NSUInteger sublayersIndex;
+  {
+    ASDN::MutexLocker l(__instanceLock__);
+    subnodesIndex = _subnodes.count;
+    sublayersIndex = _layer.sublayers.count;
+  }
+  
+  [self _insertSubnode:subnode atSubnodeIndex:subnodesIndex sublayerIndex:sublayersIndex andRemoveSubnode:nil];
 }
 
 - (void)_addSubnodeViewsAndLayers
@@ -2048,7 +2077,6 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
 - (void)_replaceSubnode:(ASDisplayNode *)oldSubnode withSubnode:(ASDisplayNode *)replacementSubnode
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASDN::MutexLocker l(__instanceLock__);
 
   if (replacementSubnode == nil) {
     ASDisplayNodeFailAssert(@"Invalid subnode to replace");
@@ -2061,19 +2089,24 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
   }
 
   ASDisplayNodeAssert(!(self.nodeLoaded && !oldSubnode.nodeLoaded), @"We have view loaded, but child node does not.");
-  ASDisplayNodeAssert(_subnodes, @"You should have subnodes if you have a subnode");
 
-  NSInteger subnodeIndex = [_subnodes indexOfObjectIdenticalTo:oldSubnode];
+  NSInteger subnodeIndex;
   NSInteger sublayerIndex = NSNotFound;
-
-  // Don't bother figuring out the sublayerIndex if in a rasterized subtree, because there are no layers in the
-  // hierarchy and none of this could possibly work.
-  if (nodeIsInRasterizedTree(self) == NO) {
-    if (_layer) {
-      sublayerIndex = [_layer.sublayers indexOfObjectIdenticalTo:oldSubnode.layer];
-      ASDisplayNodeAssert(sublayerIndex != NSNotFound, @"Somehow oldSubnode's supernode is self, yet we could not find it in our layers to replace");
-      if (sublayerIndex == NSNotFound) {
-        return;
+  {
+    ASDN::MutexLocker l(__instanceLock__);
+    ASDisplayNodeAssert(_subnodes, @"You should have subnodes if you have a subnode");
+    
+    subnodeIndex = [_subnodes indexOfObjectIdenticalTo:oldSubnode];
+    
+    // Don't bother figuring out the sublayerIndex if in a rasterized subtree, because there are no layers in the
+    // hierarchy and none of this could possibly work.
+    if (nodeIsInRasterizedTree(self) == NO) {
+      if (_layer) {
+        sublayerIndex = [_layer.sublayers indexOfObjectIdenticalTo:oldSubnode.layer];
+        ASDisplayNodeAssert(sublayerIndex != NSNotFound, @"Somehow oldSubnode's supernode is self, yet we could not find it in our layers to replace");
+        if (sublayerIndex == NSNotFound) {
+          return;
+        }
       }
     }
   }
@@ -2092,7 +2125,6 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
 - (void)_insertSubnode:(ASDisplayNode *)subnode belowSubnode:(ASDisplayNode *)below
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASDN::MutexLocker l(__instanceLock__);
 
   if (subnode == nil) {
     ASDisplayNodeFailAssert(@"Cannot insert a nil subnode");
@@ -2104,35 +2136,38 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
     return;
   }
 
-  ASDisplayNodeAssert(_subnodes, @"You should have subnodes if you have a subnode");
-
-  NSInteger belowSubnodeIndex = [_subnodes indexOfObjectIdenticalTo:below];
+  NSInteger belowSubnodeIndex;
   NSInteger belowSublayerIndex = NSNotFound;
-
-  
-  // Don't bother figuring out the sublayerIndex if in a rasterized subtree, because there are no layers in the
-  // hierarchy and none of this could possibly work.
-  if (nodeIsInRasterizedTree(self) == NO) {
-    if (_layer) {
-      belowSublayerIndex = [_layer.sublayers indexOfObjectIdenticalTo:below.layer];
-      ASDisplayNodeAssert(belowSublayerIndex != NSNotFound, @"Somehow below's supernode is self, yet we could not find it in our layers to reference");
-      if (belowSublayerIndex == NSNotFound)
-        return;
-    }
+  {
+    ASDN::MutexLocker l(__instanceLock__);
+    ASDisplayNodeAssert(_subnodes, @"You should have subnodes if you have a subnode");
     
-    ASDisplayNodeAssert(belowSubnodeIndex != NSNotFound, @"Couldn't find above in subnodes");
+    belowSubnodeIndex = [_subnodes indexOfObjectIdenticalTo:below];
     
-    // If the subnode is already in the subnodes array / sublayers and it's before the below node, removing it to
-    // insert it will mess up our calculation
-    if (subnode.supernode == self) {
-      NSInteger currentIndexInSubnodes = [_subnodes indexOfObjectIdenticalTo:subnode];
-      if (currentIndexInSubnodes < belowSubnodeIndex) {
-        belowSubnodeIndex--;
-      }
+    // Don't bother figuring out the sublayerIndex if in a rasterized subtree, because there are no layers in the
+    // hierarchy and none of this could possibly work.
+    if (nodeIsInRasterizedTree(self) == NO) {
       if (_layer) {
-        NSInteger currentIndexInSublayers = [_layer.sublayers indexOfObjectIdenticalTo:subnode.layer];
-        if (currentIndexInSublayers < belowSublayerIndex) {
-          belowSublayerIndex--;
+        belowSublayerIndex = [_layer.sublayers indexOfObjectIdenticalTo:below.layer];
+        ASDisplayNodeAssert(belowSublayerIndex != NSNotFound, @"Somehow below's supernode is self, yet we could not find it in our layers to reference");
+        if (belowSublayerIndex == NSNotFound)
+          return;
+      }
+      
+      ASDisplayNodeAssert(belowSubnodeIndex != NSNotFound, @"Couldn't find above in subnodes");
+      
+      // If the subnode is already in the subnodes array / sublayers and it's before the below node, removing it to
+      // insert it will mess up our calculation
+      if (subnode.supernode == self) {
+        NSInteger currentIndexInSubnodes = [_subnodes indexOfObjectIdenticalTo:subnode];
+        if (currentIndexInSubnodes < belowSubnodeIndex) {
+          belowSubnodeIndex--;
+        }
+        if (_layer) {
+          NSInteger currentIndexInSublayers = [_layer.sublayers indexOfObjectIdenticalTo:subnode.layer];
+          if (currentIndexInSublayers < belowSublayerIndex) {
+            belowSublayerIndex--;
+          }
         }
       }
     }
@@ -2154,7 +2189,6 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
 - (void)_insertSubnode:(ASDisplayNode *)subnode aboveSubnode:(ASDisplayNode *)above
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASDN::MutexLocker l(__instanceLock__);
 
   if (subnode == nil) {
     ASDisplayNodeFailAssert(@"Cannot insert a nil subnode");
@@ -2166,34 +2200,38 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
     return;
   }
 
-  ASDisplayNodeAssert(_subnodes, @"You should have subnodes if you have a subnode");
-
-  NSInteger aboveSubnodeIndex = [_subnodes indexOfObjectIdenticalTo:above];
+  NSInteger aboveSubnodeIndex;
   NSInteger aboveSublayerIndex = NSNotFound;
-
-  // Don't bother figuring out the sublayerIndex if in a rasterized subtree, because there are no layers in the
-  // hierarchy and none of this could possibly work.
-  if (nodeIsInRasterizedTree(self) == NO) {
-    if (_layer) {
-      aboveSublayerIndex = [_layer.sublayers indexOfObjectIdenticalTo:above.layer];
-      ASDisplayNodeAssert(aboveSublayerIndex != NSNotFound, @"Somehow above's supernode is self, yet we could not find it in our layers to replace");
-      if (aboveSublayerIndex == NSNotFound)
-        return;
-    }
+  {
+    ASDN::MutexLocker l(__instanceLock__);
+    ASDisplayNodeAssert(_subnodes, @"You should have subnodes if you have a subnode");
     
-    ASDisplayNodeAssert(aboveSubnodeIndex != NSNotFound, @"Couldn't find above in subnodes");
-
-    // If the subnode is already in the subnodes array / sublayers and it's before the below node, removing it to
-    // insert it will mess up our calculation
-    if (subnode.supernode == self) {
-      NSInteger currentIndexInSubnodes = [_subnodes indexOfObjectIdenticalTo:subnode];
-      if (currentIndexInSubnodes <= aboveSubnodeIndex) {
-        aboveSubnodeIndex--;
-      }
+    aboveSubnodeIndex = [_subnodes indexOfObjectIdenticalTo:above];
+    
+    // Don't bother figuring out the sublayerIndex if in a rasterized subtree, because there are no layers in the
+    // hierarchy and none of this could possibly work.
+    if (nodeIsInRasterizedTree(self) == NO) {
       if (_layer) {
-        NSInteger currentIndexInSublayers = [_layer.sublayers indexOfObjectIdenticalTo:subnode.layer];
-        if (currentIndexInSublayers <= aboveSublayerIndex) {
-          aboveSublayerIndex--;
+        aboveSublayerIndex = [_layer.sublayers indexOfObjectIdenticalTo:above.layer];
+        ASDisplayNodeAssert(aboveSublayerIndex != NSNotFound, @"Somehow above's supernode is self, yet we could not find it in our layers to replace");
+        if (aboveSublayerIndex == NSNotFound)
+          return;
+      }
+      
+      ASDisplayNodeAssert(aboveSubnodeIndex != NSNotFound, @"Couldn't find above in subnodes");
+      
+      // If the subnode is already in the subnodes array / sublayers and it's before the below node, removing it to
+      // insert it will mess up our calculation
+      if (subnode.supernode == self) {
+        NSInteger currentIndexInSubnodes = [_subnodes indexOfObjectIdenticalTo:subnode];
+        if (currentIndexInSubnodes <= aboveSubnodeIndex) {
+          aboveSubnodeIndex--;
+        }
+        if (_layer) {
+          NSInteger currentIndexInSublayers = [_layer.sublayers indexOfObjectIdenticalTo:subnode.layer];
+          if (currentIndexInSublayers <= aboveSublayerIndex) {
+            aboveSublayerIndex--;
+          }
         }
       }
     }
@@ -2213,30 +2251,32 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
 - (void)_insertSubnode:(ASDisplayNode *)subnode atIndex:(NSInteger)idx
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASDN::MutexLocker l(__instanceLock__);
   
   if (subnode == nil) {
     ASDisplayNodeFailAssert(@"Cannot insert a nil subnode");
     return;
   }
 
-  if (idx > _subnodes.count || idx < 0) {
-    ASDisplayNodeFailAssert(@"Cannot insert a subnode at index %zd. Count is %zd", idx, _subnodes.count);
-    return;
-  }
-  
   NSInteger sublayerIndex = NSNotFound;
-
-  // Don't bother figuring out the sublayerIndex if in a rasterized subtree, because there are no layers in the
-  // hierarchy and none of this could possibly work.
-  if (nodeIsInRasterizedTree(self) == NO) {
-    // Account for potentially having other subviews
-    if (_layer && idx == 0) {
-      sublayerIndex = 0;
-    } else if (_layer) {
-      ASDisplayNode *positionInRelationTo = (_subnodes.count > 0 && idx > 0) ? _subnodes[idx - 1] : nil;
-      if (positionInRelationTo) {
-        sublayerIndex = incrementIfFound([_layer.sublayers indexOfObjectIdenticalTo:positionInRelationTo.layer]);
+  {
+    ASDN::MutexLocker l(__instanceLock__);
+    
+    if (idx > _subnodes.count || idx < 0) {
+      ASDisplayNodeFailAssert(@"Cannot insert a subnode at index %zd. Count is %zd", idx, _subnodes.count);
+      return;
+    }
+    
+    // Don't bother figuring out the sublayerIndex if in a rasterized subtree, because there are no layers in the
+    // hierarchy and none of this could possibly work.
+    if (nodeIsInRasterizedTree(self) == NO) {
+      // Account for potentially having other subviews
+      if (_layer && idx == 0) {
+        sublayerIndex = 0;
+      } else if (_layer) {
+        ASDisplayNode *positionInRelationTo = (_subnodes.count > 0 && idx > 0) ? _subnodes[idx - 1] : nil;
+        if (positionInRelationTo) {
+          sublayerIndex = incrementIfFound([_layer.sublayers indexOfObjectIdenticalTo:positionInRelationTo.layer]);
+        }
       }
     }
   }
@@ -2247,15 +2287,17 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
 - (void)_removeSubnode:(ASDisplayNode *)subnode
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASDN::MutexLocker l(__instanceLock__);
-
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
+  
   // 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.supernode != self) {
     return;
   }
 
-  [_subnodes removeObjectIdenticalTo:subnode];
+  __instanceLock__.lock();
+    [_subnodes removeObjectIdenticalTo:subnode];
+  __instanceLock__.unlock();
 
   [subnode __setSupernode:nil];
 }
@@ -2267,10 +2309,11 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
   [self _removeFromSupernode];
 }
 
-// NOTE: You must not called this method while holding the receiver's property lock. This may cause deadlocks.
+// NOTE: You must not called this method while holding the receiver's instance lock. This may cause deadlocks.
 - (void)_removeFromSupernode
 {
   ASDisplayNodeAssertThreadAffinity(self);
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
   
   __instanceLock__.lock();
     __weak ASDisplayNode *supernode = _supernode;
@@ -2338,21 +2381,29 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
 {
   ASDisplayNodeAssertMainThread();
   ASDisplayNodeAssert(!_flags.isEnteringHierarchy, @"Should not cause recursive __enterHierarchy");
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
   
   // Profiling has shown that locking this method is beneficial, so each of the property accesses don't have to lock and unlock.
-  ASDN::MutexLocker l(__instanceLock__);
+  __instanceLock__.lock();
   
   if (!_flags.isInHierarchy && !_flags.visibilityNotificationsDisabled && ![self __selfOrParentHasVisibilityNotificationsDisabled]) {
     _flags.isEnteringHierarchy = YES;
     _flags.isInHierarchy = YES;
 
-    [self willEnterHierarchy];
-    for (ASDisplayNode *subnode in self.subnodes) {
-      [subnode __enterHierarchy];
-    }
+    // Don't call -willEnterHierarchy while holding __instanceLock__.
+    // This method and subsequent ones (i.e -interfaceState and didEnter(.*)State)
+    // don't expect that they are called while the lock is being held.
+    // More importantly, didEnter(.*)State methods are meant to be overriden by clients.
+    // And so they can potentially walk up the node tree and cause deadlocks, or do expensive tasks and cause the lock to be held for too long.
+    __instanceLock__.unlock();
+      [self willEnterHierarchy];
+      for (ASDisplayNode *subnode in self.subnodes) {
+        [subnode __enterHierarchy];
+      }
+    __instanceLock__.lock();
+    
     _flags.isEnteringHierarchy = NO;
 
-    
     // If we don't have contents finished drawing by the time we are on screen, immediately add the placeholder (if it is enabled and we do have something to draw).
     if (self.contents == nil) {
       CALayer *layer = self.layer;
@@ -2368,15 +2419,18 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
       }
     }
   }
+  
+  __instanceLock__.unlock();
 }
 
 - (void)__exitHierarchy
 {
   ASDisplayNodeAssertMainThread();
   ASDisplayNodeAssert(!_flags.isExitingHierarchy, @"Should not cause recursive __exitHierarchy");
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
   
   // Profiling has shown that locking this method is beneficial, so each of the property accesses don't have to lock and unlock.
-  ASDN::MutexLocker l(__instanceLock__);
+  __instanceLock__.lock();
   
   if (_flags.isInHierarchy && !_flags.visibilityNotificationsDisabled && ![self __selfOrParentHasVisibilityNotificationsDisabled]) {
     _flags.isExitingHierarchy = YES;
@@ -2384,12 +2438,22 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
 
     [self.asyncLayer cancelAsyncDisplay];
 
-    [self didExitHierarchy];
-    for (ASDisplayNode *subnode in self.subnodes) {
-      [subnode __exitHierarchy];
-    }
+    // Don't call -didExitHierarchy while holding __instanceLock__.
+    // This method and subsequent ones (i.e -interfaceState and didExit(.*)State)
+    // don't expect that they are called while the lock is being held.
+    // More importantly, didExit(.*)State methods are meant to be overriden by clients.
+    // And so they can potentially walk up the node tree and cause deadlocks, or do expensive tasks and cause the lock to be held for too long.
+    __instanceLock__.unlock();
+      [self didExitHierarchy];
+      for (ASDisplayNode *subnode in self.subnodes) {
+        [subnode __exitHierarchy];
+      }
+    __instanceLock__.lock();
+    
     _flags.isExitingHierarchy = NO;
   }
+  
+  __instanceLock__.unlock();
 }
 
 - (void)_recursiveWillEnterHierarchy
@@ -2431,6 +2495,12 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
 // NOTE: This method must be dealloc-safe (should not retain self).
 - (ASDisplayNode *)supernode
 {
+#if CHECK_LOCKING_SAFETY
+  if (__instanceLock__.ownedByCurrentThread()) {
+    NSLog(@"WARNING: Accessing supernode while holding recursive instance lock of this node is worrisome. It's likely that you will soon try to acquire the supernode's lock, and this can easily cause deadlocks.");
+  }
+#endif
+  
   ASDN::MutexLocker l(__instanceLock__);
   return _supernode;
 }
@@ -2556,6 +2626,7 @@ ASDISPLAYNODE_INLINE BOOL nodeIsInRasterizedTree(ASDisplayNode *node) {
 // For details look at the comment on the canCallSetNeedsDisplayOfLayer flag
 - (BOOL)__canCallSetNeedsDisplayOfLayer
 {
+  ASDN::MutexLocker l(__instanceLock__);
   return _flags.canCallSetNeedsDisplayOfLayer;
 }
 
@@ -2899,7 +2970,8 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock)
   ASDisplayNodeAssertMainThread();
   ASDisplayNodeAssert(_flags.isEnteringHierarchy, @"You should never call -willEnterHierarchy directly. Appearance is automatically managed by ASDisplayNode");
   ASDisplayNodeAssert(!_flags.isExitingHierarchy, @"ASDisplayNode inconsistency. __enterHierarchy and __exitHierarchy are mutually exclusive");
-
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
+  
   if (![self supportsRangeManagedInterfaceState]) {
     self.interfaceState = ASInterfaceStateInHierarchy;
   }
@@ -2910,6 +2982,7 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock)
   ASDisplayNodeAssertMainThread();
   ASDisplayNodeAssert(_flags.isExitingHierarchy, @"You should never call -didExitHierarchy directly. Appearance is automatically managed by ASDisplayNode");
   ASDisplayNodeAssert(!_flags.isEnteringHierarchy, @"ASDisplayNode inconsistency. __enterHierarchy and __exitHierarchy are mutually exclusive");
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
   
   if (![self supportsRangeManagedInterfaceState]) {
     self.interfaceState = ASInterfaceStateNone;
@@ -2921,12 +2994,17 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock)
     // TODO: This approach could be optimized by only performing the dispatch for root elements + recursively apply the interface state change. This would require a closer
     // integration with _ASDisplayLayer to ensure that the superlayer pointer has been cleared by this stage (to check if we are root or not), or a different delegate call.
     
-    if (ASInterfaceStateIncludesVisible(_interfaceState)) {
+    if (ASInterfaceStateIncludesVisible(self.interfaceState)) {
       dispatch_async(dispatch_get_main_queue(), ^{
         // This block intentionally retains self.
-        ASDN::MutexLocker l(__instanceLock__);
-        if (!_flags.isInHierarchy && ASInterfaceStateIncludesVisible(_interfaceState)) {
-          self.interfaceState = (_interfaceState & ~ASInterfaceStateVisible);
+        __instanceLock__.lock();
+          unsigned isInHierarchy = _flags.isInHierarchy;
+          BOOL isVisible = ASInterfaceStateIncludesVisible(_interfaceState);
+          ASInterfaceState newState = (_interfaceState & ~ASInterfaceStateVisible);
+        __instanceLock__.unlock();
+        
+        if (!isInHierarchy && isVisible) {
+          self.interfaceState = newState;
         }
       });
     }
@@ -2937,6 +3015,7 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock)
 
 - (void)clearContents
 {
+  ASDisplayNodeAssertMainThread();
   if (_flags.canClearContentsOfLayer) {
     // No-op if these haven't been created yet, as that guarantees they don't have contents that needs to be released.
     _layer.contents = nil;
@@ -2977,25 +3056,36 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock)
 - (void)didEnterVisibleState
 {
   // subclass override
+  ASDisplayNodeAssertMainThread();
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
 }
 
 - (void)didExitVisibleState
 {
   // subclass override
+  ASDisplayNodeAssertMainThread();
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
 }
 
 - (void)didEnterDisplayState
 {
   // subclass override
+  ASDisplayNodeAssertMainThread();
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
 }
 
 - (void)didExitDisplayState
 {
   // subclass override
+  ASDisplayNodeAssertMainThread();
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
 }
 
 - (void)didEnterPreloadState
 {
+  ASDisplayNodeAssertMainThread();
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
+  
   if (_methodOverrides & ASDisplayNodeMethodOverrideFetchData) {
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
@@ -3006,6 +3096,9 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock)
 
 - (void)didExitPreloadState
 {
+  ASDisplayNodeAssertMainThread();
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
+  
   if (_methodOverrides & ASDisplayNodeMethodOverrideClearFetchedData) {
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
@@ -3020,6 +3113,7 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock)
  */
 - (BOOL)supportsRangeManagedInterfaceState
 {
+  ASDN::MutexLocker l(__instanceLock__);
   return ASHierarchyStateIncludesRangeManaged(_hierarchyState);
 }
 
@@ -3054,6 +3148,9 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock)
   ASDisplayNodeAssertMainThread();
   // It should never be possible for a node to be visible but not be allowed / expected to display.
   ASDisplayNodeAssertFalse(ASInterfaceStateIncludesVisible(newState) && !ASInterfaceStateIncludesDisplay(newState));
+  // This method manages __instanceLock__ itself, to ensure the lock is not held while didEnter/Exit(.*)State methods are called, thus avoid potential deadlocks
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
+  
   ASInterfaceState oldState = ASInterfaceStateNone;
   {
     ASDN::MutexLocker l(__instanceLock__);
@@ -3156,6 +3253,7 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock)
 - (void)interfaceStateDidChange:(ASInterfaceState)newState fromState:(ASInterfaceState)oldState
 {
   // subclass hook
+  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
 }
 
 - (void)enterInterfaceState:(ASInterfaceState)interfaceState

AsyncDisplayKit/ASNetworkImageNode.mm

@@ -131,28 +131,30 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0};
 
 - (void)setURL:(NSURL *)URL resetToDefault:(BOOL)reset
 {
-  ASDN::MutexLocker l(__instanceLock__);
-
-  _imageWasSetExternally = NO;
-
-  if (ASObjectIsEqual(URL, _URL)) {
-    return;
-  }
-
-  [self _cancelImageDownload];
-  _imageLoaded = NO;
-
-  _URL = URL;
-
-  BOOL hasURL = _URL == nil;
-  if (reset || hasURL) {
-    [self _setImage:_defaultImage];
-    /* We want to maintain the order that currentImageQuality is set regardless of the calling thread,
-     so always use a dispatch_async to ensure that we queue the operations in the correct order.
-     (see comment in displayDidFinish) */
-    dispatch_async(dispatch_get_main_queue(), ^{
-      self.currentImageQuality = hasURL ? 0.0 : 1.0;
-    });
+  {
+    ASDN::MutexLocker l(__instanceLock__);
+    
+    _imageWasSetExternally = NO;
+    
+    if (ASObjectIsEqual(URL, _URL)) {
+      return;
+    }
+    
+    [self _cancelImageDownload];
+    _imageLoaded = NO;
+    
+    _URL = URL;
+    
+    BOOL hasURL = _URL == nil;
+    if (reset || hasURL) {
+      [self _setImage:_defaultImage];
+      /* We want to maintain the order that currentImageQuality is set regardless of the calling thread,
+       so always use a dispatch_async to ensure that we queue the operations in the correct order.
+       (see comment in displayDidFinish) */
+      dispatch_async(dispatch_get_main_queue(), ^{
+        self.currentImageQuality = hasURL ? 0.0 : 1.0;
+      });
+    }
   }
 
   [self setNeedsPreload];

AsyncDisplayKit/ASVideoNode.mm

@@ -117,6 +117,7 @@ static NSString * const kRate = @"rate";
 
 - (AVPlayerItem *)constructPlayerItem
 {
+  ASDisplayNodeAssertMainThread();
   ASDN::MutexLocker l(__instanceLock__);
 
   AVPlayerItem *playerItem = nil;
@@ -134,6 +135,8 @@ static NSString * const kRate = @"rate";
 
 - (void)prepareToPlayAsset:(AVAsset *)asset withKeys:(NSArray<NSString *> *)requestedKeys
 {
+  ASDisplayNodeAssertMainThread();
+  
   for (NSString *key in requestedKeys) {
     NSError *error = nil;
     AVKeyValueStatus keyStatus = [asset statusOfValueForKey:key error:&error];
@@ -467,10 +470,10 @@ static NSString * const kRate = @"rate";
 
 - (void)setAssetURL:(NSURL *)assetURL
 {
-  ASDN::MutexLocker l(__instanceLock__);
-
+  ASDisplayNodeAssertMainThread();
+  
   if (ASObjectIsEqual(assetURL, self.assetURL) == NO) {
-    [self locked_setAndFetchAsset:[AVURLAsset assetWithURL:assetURL] url:assetURL];
+    [self setAndFetchAsset:[AVURLAsset assetWithURL:assetURL] url:assetURL];
   }
 }
 
@@ -489,10 +492,10 @@ static NSString * const kRate = @"rate";
 
 - (void)setAsset:(AVAsset *)asset
 {
-  ASDN::MutexLocker l(__instanceLock__);
+  ASDisplayNodeAssertMainThread();
   
-  if (ASAssetIsEqual(asset, _asset) == NO) {
-    [self locked_setAndFetchAsset:asset url:nil];
+  if (ASAssetIsEqual(asset, self.asset) == NO) {
+    [self setAndFetchAsset:asset url:nil];
   }
 }
 
@@ -502,12 +505,19 @@ static NSString * const kRate = @"rate";
   return _asset;
 }
 
-- (void)locked_setAndFetchAsset:(AVAsset *)asset url:(NSURL *)assetURL
+- (void)setAndFetchAsset:(AVAsset *)asset url:(NSURL *)assetURL
 {
+  ASDisplayNodeAssertMainThread();
+  
   [self didExitPreloadState];
-  self.videoPlaceholderImage = nil;
-  _asset = asset;
-  _assetURL = assetURL;
+  
+  {
+    ASDN::MutexLocker l(__instanceLock__);
+    self.videoPlaceholderImage = nil;
+    _asset = asset;
+    _assetURL = assetURL;
+  }
+  
   [self setNeedsPreload];
 }
 
@@ -610,21 +620,25 @@ static NSString * const kRate = @"rate";
 
 - (void)play
 {
-  ASDN::MutexLocker l(__instanceLock__);
+  __instanceLock__.lock();
 
   if (![self isStateChangeValid:ASVideoNodePlayerStatePlaying]) {
+    __instanceLock__.unlock();
     return;
   }
 
   if (_player == nil) {
-    [self setNeedsPreload];
+    __instanceLock__.unlock();
+      [self setNeedsPreload];
+    __instanceLock__.lock();
   }
 
   if (_playerNode == nil) {
     _playerNode = [self constructPlayerNode];
 
-    [self addSubnode:_playerNode];
-
+    __instanceLock__.unlock();
+      [self addSubnode:_playerNode];
+    __instanceLock__.lock();
       
     [self setNeedsLayout];
   }
@@ -632,6 +646,7 @@ static NSString * const kRate = @"rate";
   
   [_player play];
   _shouldBePlaying = YES;
+  __instanceLock__.unlock();
 }
 
 - (BOOL)ready

AsyncDisplayKit/Details/ASThread.h

@@ -29,6 +29,17 @@ static inline BOOL ASDisplayNodeThreadIsMain()
 #ifdef __cplusplus
 
 #define TIME_LOCKER 0
+/**
+ * Enable this flag to collect information on the owning thread and ownership level of a mutex.
+ * These properties are useful to determine if a mutext has been acquired and in case of a recursive mutex, how many times that happened.
+ * 
+ * This flag also enable locking assertions (e.g ASDisplayNodeAssertLockUnownedByCurrentThread(node)).
+ * The assertions are useful when you want to indicate and enforce the locking policy/expectation of methods.
+ * To determine when and which methods acquired a (recursive) mutex (to debug deadlocks, for example),
+ * put breakpoints at some assertions. When the breakpoints hit, walk through stack trace frames 
+ * and check ownership count of the mutex.
+ */
+#define CHECK_LOCKING_SAFETY 0
 
 #if TIME_LOCKER
 #import <QuartzCore/QuartzCore.h>
@@ -174,6 +185,10 @@ namespace ASDN {
 
     ~Mutex () {
       ASDISPLAYNODE_THREAD_ASSERT_ON_ERROR(pthread_mutex_destroy (&_m));
+#if CHECK_LOCKING_SAFETY
+      _owner = 0;
+      _count = 0;
+#endif
     }
 
     Mutex (const Mutex&) = delete;
@@ -181,14 +196,40 @@ namespace ASDN {
 
     void lock () {
       ASDISPLAYNODE_THREAD_ASSERT_ON_ERROR(pthread_mutex_lock (this->mutex()));
+#if CHECK_LOCKING_SAFETY
+      mach_port_t thread_id = pthread_mach_thread_np(pthread_self());
+      if (thread_id != _owner) {
+        assert(0 == _owner);
+        assert(0 == _count);
+        _owner = thread_id;
+      } else {
+        assert(_count > 0);
+      }
+      ++_count;
+#endif
     }
 
     void unlock () {
+#if CHECK_LOCKING_SAFETY
+      mach_port_t thread_id = pthread_mach_thread_np(pthread_self());
+      assert(thread_id == _owner);
+      assert(_count > 0);
+      --_count;
+      if (0 == _count) {
+        _owner = 0;
+      }
+#endif
       ASDISPLAYNODE_THREAD_ASSERT_ON_ERROR(pthread_mutex_unlock (this->mutex()));
     }
 
     pthread_mutex_t *mutex () { return &_m; }
 
+#if CHECK_LOCKING_SAFETY
+    bool ownedByCurrentThread() {
+      return _count > 0 && pthread_mach_thread_np(pthread_self()) == _owner;
+    }
+#endif
+    
   protected:
     explicit Mutex (bool recursive) {
       if (!recursive) {
@@ -200,10 +241,18 @@ namespace ASDN {
         ASDISPLAYNODE_THREAD_ASSERT_ON_ERROR(pthread_mutex_init (&_m, &attr));
         ASDISPLAYNODE_THREAD_ASSERT_ON_ERROR(pthread_mutexattr_destroy (&attr));
       }
+#if CHECK_LOCKING_SAFETY
+      _owner = 0;
+      _count = 0;
+#endif
     }
 
   private:
     pthread_mutex_t _m;
+#if CHECK_LOCKING_SAFETY
+    mach_port_t _owner;
+    uint32_t _count;
+#endif
   };
 
   /**