From 0f50cd02c996aaf2be1f8a9e10309549a5d5bf64 Mon Sep 17 00:00:00 2001
From: Huy Nguyen <allforone1511@gmail.com>
Date: Fri, 24 Mar 2017 17:35:43 +0000
Subject: [PATCH] Fix locking situation of layout transition (#3217)

- It's not safe to hold the lock of a supernode and:
  - Edit states of its subnodes
  - Call subclass hooks
  - Run completion block
  - Run animation, which can trigger layout passes on subnodes, especially if one of them is a collection view.
---
 Source/ASDisplayNode.mm | 68 +++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 27 deletions(-)

diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm
index d104a090c3..2e5278c184 100644
--- a/Source/ASDisplayNode.mm
+++ b/Source/ASDisplayNode.mm
@@ -1305,7 +1305,11 @@ ASLayoutElementFinalLayoutElementDefault
 - (void)_setCalculatedDisplayNodeLayout:(std::shared_ptr<ASDisplayNodeLayout>)displayNodeLayout
 {
   ASDN::MutexLocker l(__instanceLock__);
-  
+  [self _locked_setCalculatedDisplayNodeLayout:displayNodeLayout];
+}
+
+- (void)_locked_setCalculatedDisplayNodeLayout:(std::shared_ptr<ASDisplayNodeLayout>)displayNodeLayout
+{
   ASDisplayNodeAssertTrue(displayNodeLayout->layout.layoutElement == self);
   ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.width >= 0.0);
   ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.height >= 0.0);
@@ -1313,6 +1317,7 @@ ASLayoutElementFinalLayoutElementDefault
   _calculatedDisplayNodeLayout = displayNodeLayout;
 }
 
+
 - (CGSize)calculatedSize
 {
   ASDN::MutexLocker l(__instanceLock__);
@@ -1540,22 +1545,35 @@ ASLayoutElementFinalLayoutElementDefault
     }
     
     ASPerformBlockOnMainThread(^{
-      // Grab __instanceLock__ here to make sure this transition isn't invalidated
-      // right after it passed the validation test and before it proceeds
-      ASDN::MutexLocker l(__instanceLock__);
-
-      if ([self _shouldAbortTransitionWithID:transitionID]) {
-        return;
+      ASLayoutTransition *pendingLayoutTransition;
+      _ASTransitionContext *pendingLayoutTransitionContext;
+      {
+        // Grab __instanceLock__ here to make sure this transition isn't invalidated
+        // right after it passed the validation test and before it proceeds
+        ASDN::MutexLocker l(__instanceLock__);
+        
+        if ([self _locked_shouldAbortTransitionWithID:transitionID]) {
+          return;
+        }
+        
+        // Update calculated layout
+        auto previousLayout = _calculatedDisplayNodeLayout;
+        auto pendingLayout = std::make_shared<ASDisplayNodeLayout>(
+                                                                   newLayout,
+                                                                   constrainedSize,
+                                                                   constrainedSize.max
+                                                                   );
+        [self _locked_setCalculatedDisplayNodeLayout:pendingLayout];
+        
+        // Setup pending layout transition for animation
+        _pendingLayoutTransition = pendingLayoutTransition = [[ASLayoutTransition alloc] initWithNode:self
+                                                                                        pendingLayout:pendingLayout
+                                                                                       previousLayout:previousLayout];
+        // Setup context for pending layout transition. we need to hold a strong reference to the context
+        _pendingLayoutTransitionContext = pendingLayoutTransitionContext = [[_ASTransitionContext alloc] initWithAnimation:animated
+                                                                                                            layoutDelegate:_pendingLayoutTransition
+                                                                                                        completionDelegate:self];
       }
-
-      // Update calculated layout
-      auto previousLayout = _calculatedDisplayNodeLayout;
-      auto pendingLayout = std::make_shared<ASDisplayNodeLayout>(
-        newLayout,
-        constrainedSize,
-        constrainedSize.max
-      );
-      [self _setCalculatedDisplayNodeLayout:pendingLayout];
       
       // Apply complete layout transitions for all subnodes
       ASDisplayNodePerformBlockOnEverySubnode(self, NO, ^(ASDisplayNode * _Nonnull node) {
@@ -1570,20 +1588,11 @@ ASLayoutElementFinalLayoutElementDefault
         completion();
       }
       
-      // Setup pending layout transition for animation
-      _pendingLayoutTransition = [[ASLayoutTransition alloc] initWithNode:self
-                                                            pendingLayout:pendingLayout
-                                                           previousLayout:previousLayout];
-      // Setup context for pending layout transition. we need to hold a strong reference to the context
-      _pendingLayoutTransitionContext = [[_ASTransitionContext alloc] initWithAnimation:animated
-                                                                         layoutDelegate:_pendingLayoutTransition
-                                                                     completionDelegate:self];
-      
       // Apply the subnode insertion immediately to be able to animate the nodes
-      [_pendingLayoutTransition applySubnodeInsertions];
+      [pendingLayoutTransition applySubnodeInsertions];
       
       // Kick off animating the layout transition
-      [self animateLayoutTransition:_pendingLayoutTransitionContext];
+      [self animateLayoutTransition:pendingLayoutTransitionContext];
       
       // Mark transaction as finished
       [self _finishOrCancelTransition];
@@ -1669,6 +1678,11 @@ ASLayoutElementFinalLayoutElementDefault
 - (BOOL)_shouldAbortTransitionWithID:(int32_t)transitionID
 {
   ASDN::MutexLocker l(__instanceLock__);
+  return [self _locked_shouldAbortTransitionWithID:transitionID];
+}
+
+- (BOOL)_locked_shouldAbortTransitionWithID:(int32_t)transitionID
+{
   return (!_transitionInProgress || _transitionID != transitionID);
 }