Revert #1023 (#1204)

We will revert #1023. The current solution introduces problems if we are unlocking before calling _completePendingLayoutTransition. _completePendingLayoutTransition needs to be happening in one transaction if called from _u_measureNodeWithBoundsIfNecessary.

Source/ASDisplayNode+Layout.mm

@@ -296,125 +296,121 @@ ASLayoutElementStyleExtensibilityForwarding
 {
   ASAssertUnlocked(__instanceLock__);
   
-  BOOL isInLayoutPendingState = NO;
+  ASDN::MutexLocker l(__instanceLock__);
-  {
+  // Check if we are a subnode in a layout transition.
-    ASDN::MutexLocker l(__instanceLock__);
+  // In this case no measurement is needed as it's part of the layout transition
-    // Check if we are a subnode in a layout transition.
+  if ([self _locked_isLayoutTransitionInvalid]) {
-    // In this case no measurement is needed as it's part of the layout transition
+    return;
-    if ([self _locked_isLayoutTransitionInvalid]) {
-      return;
-    }
-
-    CGSize boundsSizeForLayout = ASCeilSizeValues(bounds.size);
-
-    // Prefer a newer and not yet applied _pendingDisplayNodeLayout over _calculatedDisplayNodeLayout
-    // If there is no such _pending, check if _calculated is valid to reuse (avoiding recalculation below).
-    BOOL pendingLayoutIsPreferred = NO;
-    if (_pendingDisplayNodeLayout.isValid(_layoutVersion)) {
-      NSUInteger calculatedVersion = _calculatedDisplayNodeLayout.version;
-      NSUInteger pendingVersion = _pendingDisplayNodeLayout.version;
-      if (pendingVersion > calculatedVersion) {
-        pendingLayoutIsPreferred = YES; // Newer _pending
-      } else if (pendingVersion == calculatedVersion
-                 && !ASSizeRangeEqualToSizeRange(_pendingDisplayNodeLayout.constrainedSize,
-                                                 _calculatedDisplayNodeLayout.constrainedSize)) {
-                   pendingLayoutIsPreferred = YES; // _pending with a different constrained size
-                 }
-    }
-    BOOL calculatedLayoutIsReusable = (_calculatedDisplayNodeLayout.isValid(_layoutVersion)
-                                       && (_calculatedDisplayNodeLayout.requestedLayoutFromAbove
-                                           || CGSizeEqualToSize(_calculatedDisplayNodeLayout.layout.size, boundsSizeForLayout)));
-    if (!pendingLayoutIsPreferred && calculatedLayoutIsReusable) {
-      return;
-    }
-
-    as_activity_create_for_scope("Update node layout for current bounds");
-    as_log_verbose(ASLayoutLog(), "Node %@, bounds size %@, calculatedSize %@, calculatedIsDirty %d",
-                   self,
-                   NSStringFromCGSize(boundsSizeForLayout),
-                   NSStringFromCGSize(_calculatedDisplayNodeLayout->layout.size),
-                   _calculatedDisplayNodeLayout->version < _layoutVersion);
-    // _calculatedDisplayNodeLayout is not reusable we need to transition to a new one
-    [self cancelLayoutTransition];
-
-    BOOL didCreateNewContext = NO;
-    ASLayoutElementContext *context = ASLayoutElementGetCurrentContext();
-    if (context == nil) {
-      context = [[ASLayoutElementContext alloc] init];
-      ASLayoutElementPushContext(context);
-      didCreateNewContext = YES;
-    }
-
-    // Figure out previous and pending layouts for layout transition
-    ASDisplayNodeLayout nextLayout = _pendingDisplayNodeLayout;
-    #define layoutSizeDifferentFromBounds !CGSizeEqualToSize(nextLayout.layout.size, boundsSizeForLayout)
-
-    // nextLayout was likely created by a call to layoutThatFits:, check if it is valid and can be applied.
-    // If our bounds size is different than it, or invalid, recalculate.  Use #define to avoid nullptr->
-    BOOL pendingLayoutApplicable = NO;
-    if (nextLayout.layout == nil) {
-      as_log_verbose(ASLayoutLog(), "No pending layout.");
-    } else if (!nextLayout.isValid(_layoutVersion)) {
-      as_log_verbose(ASLayoutLog(), "Pending layout is stale.");
-    } else if (layoutSizeDifferentFromBounds) {
-      as_log_verbose(ASLayoutLog(), "Pending layout size %@ doesn't match bounds size.", NSStringFromCGSize(nextLayout->layout.size));
-    } else {
-      as_log_verbose(ASLayoutLog(), "Using pending layout %@.", nextLayout->layout);
-      pendingLayoutApplicable = YES;
-    }
-
-    if (!pendingLayoutApplicable) {
-      as_log_verbose(ASLayoutLog(), "Measuring with previous constrained size.");
-      // Use the last known constrainedSize passed from a parent during layout (if never, use bounds).
-      NSUInteger version = _layoutVersion;
-      ASSizeRange constrainedSize = [self _locked_constrainedSizeForLayoutPass];
-      ASLayout *layout = [self calculateLayoutThatFits:constrainedSize
-                                      restrictedToSize:self.style.size
-                                  relativeToParentSize:boundsSizeForLayout];
-      nextLayout = ASDisplayNodeLayout(layout, constrainedSize, boundsSizeForLayout, version);
-      // Now that the constrained size of pending layout might have been reused, the layout is useless
-      // Release it and any orphaned subnodes it retains
-      _pendingDisplayNodeLayout.layout = nil;
-    }
-
-    if (didCreateNewContext) {
-      ASLayoutElementPopContext();
-    }
-
-    // If our new layout's desired size for self doesn't match current size, ask our parent to update it.
-    // This can occur for either pre-calculated or newly-calculated layouts.
-    if (nextLayout.requestedLayoutFromAbove == NO
-        && CGSizeEqualToSize(boundsSizeForLayout, nextLayout.layout.size) == NO) {
-      as_log_verbose(ASLayoutLog(), "Layout size doesn't match bounds size. Requesting layout from above.");
-      // The layout that we have specifies that this node (self) would like to be a different size
-      // than it currently is.  Because that size has been computed within the constrainedSize, we
-      // expect that calling setNeedsLayoutFromAbove will result in our parent resizing us to this.
-      // However, in some cases apps may manually interfere with this (setting a different bounds).
-      // In this case, we need to detect that we've already asked to be resized to match this
-      // particular ASLayout object, and shouldn't loop asking again unless we have a different ASLayout.
-      nextLayout.requestedLayoutFromAbove = YES;
-
-      {
-        __instanceLock__.unlock();
-        [self _u_setNeedsLayoutFromAbove];
-        __instanceLock__.lock();
-      }
-
-      // Update the layout's version here because _u_setNeedsLayoutFromAbove calls __setNeedsLayout which in turn increases _layoutVersion
-      // Failing to do this will cause the layout to be invalid immediately
-      nextLayout.version = _layoutVersion;
-    }
-
-    // Prepare to transition to nextLayout
-    ASDisplayNodeAssertNotNil(nextLayout.layout, @"nextLayout->layout should not be nil! %@", self);
-    _pendingLayoutTransition = [[ASLayoutTransition alloc] initWithNode:self
-                                                          pendingLayout:nextLayout
-                                                         previousLayout:_calculatedDisplayNodeLayout];
-    isInLayoutPendingState = ASHierarchyStateIncludesLayoutPending(_hierarchyState);
   }
 
+  CGSize boundsSizeForLayout = ASCeilSizeValues(bounds.size);
+
+  // Prefer a newer and not yet applied _pendingDisplayNodeLayout over _calculatedDisplayNodeLayout
+  // If there is no such _pending, check if _calculated is valid to reuse (avoiding recalculation below).
+  BOOL pendingLayoutIsPreferred = NO;
+  if (_pendingDisplayNodeLayout.isValid(_layoutVersion)) {
+    NSUInteger calculatedVersion = _calculatedDisplayNodeLayout.version;
+    NSUInteger pendingVersion = _pendingDisplayNodeLayout.version;
+    if (pendingVersion > calculatedVersion) {
+      pendingLayoutIsPreferred = YES; // Newer _pending
+    } else if (pendingVersion == calculatedVersion
+               && !ASSizeRangeEqualToSizeRange(_pendingDisplayNodeLayout.constrainedSize,
+                                               _calculatedDisplayNodeLayout.constrainedSize)) {
+                 pendingLayoutIsPreferred = YES; // _pending with a different constrained size
+               }
+  }
+  BOOL calculatedLayoutIsReusable = (_calculatedDisplayNodeLayout.isValid(_layoutVersion)
+                                     && (_calculatedDisplayNodeLayout.requestedLayoutFromAbove
+                                         || CGSizeEqualToSize(_calculatedDisplayNodeLayout.layout.size, boundsSizeForLayout)));
+  if (!pendingLayoutIsPreferred && calculatedLayoutIsReusable) {
+    return;
+  }
+
+  as_activity_create_for_scope("Update node layout for current bounds");
+  as_log_verbose(ASLayoutLog(), "Node %@, bounds size %@, calculatedSize %@, calculatedIsDirty %d",
+                 self,
+                 NSStringFromCGSize(boundsSizeForLayout),
+                 NSStringFromCGSize(_calculatedDisplayNodeLayout->layout.size),
+                 _calculatedDisplayNodeLayout->version < _layoutVersion);
+  // _calculatedDisplayNodeLayout is not reusable we need to transition to a new one
+  [self cancelLayoutTransition];
+
+  BOOL didCreateNewContext = NO;
+  ASLayoutElementContext *context = ASLayoutElementGetCurrentContext();
+  if (context == nil) {
+    context = [[ASLayoutElementContext alloc] init];
+    ASLayoutElementPushContext(context);
+    didCreateNewContext = YES;
+  }
+
+  // Figure out previous and pending layouts for layout transition
+  ASDisplayNodeLayout nextLayout = _pendingDisplayNodeLayout;
+  #define layoutSizeDifferentFromBounds !CGSizeEqualToSize(nextLayout.layout.size, boundsSizeForLayout)
+
+  // nextLayout was likely created by a call to layoutThatFits:, check if it is valid and can be applied.
+  // If our bounds size is different than it, or invalid, recalculate.  Use #define to avoid nullptr->
+  BOOL pendingLayoutApplicable = NO;
+  if (nextLayout.layout == nil) {
+    as_log_verbose(ASLayoutLog(), "No pending layout.");
+  } else if (!nextLayout.isValid(_layoutVersion)) {
+    as_log_verbose(ASLayoutLog(), "Pending layout is stale.");
+  } else if (layoutSizeDifferentFromBounds) {
+    as_log_verbose(ASLayoutLog(), "Pending layout size %@ doesn't match bounds size.", NSStringFromCGSize(nextLayout->layout.size));
+  } else {
+    as_log_verbose(ASLayoutLog(), "Using pending layout %@.", nextLayout->layout);
+    pendingLayoutApplicable = YES;
+  }
+
+  if (!pendingLayoutApplicable) {
+    as_log_verbose(ASLayoutLog(), "Measuring with previous constrained size.");
+    // Use the last known constrainedSize passed from a parent during layout (if never, use bounds).
+    NSUInteger version = _layoutVersion;
+    ASSizeRange constrainedSize = [self _locked_constrainedSizeForLayoutPass];
+    ASLayout *layout = [self calculateLayoutThatFits:constrainedSize
+                                    restrictedToSize:self.style.size
+                                relativeToParentSize:boundsSizeForLayout];
+    nextLayout = ASDisplayNodeLayout(layout, constrainedSize, boundsSizeForLayout, version);
+    // Now that the constrained size of pending layout might have been reused, the layout is useless
+    // Release it and any orphaned subnodes it retains
+    _pendingDisplayNodeLayout.layout = nil;
+  }
+
+  if (didCreateNewContext) {
+    ASLayoutElementPopContext();
+  }
+
+  // If our new layout's desired size for self doesn't match current size, ask our parent to update it.
+  // This can occur for either pre-calculated or newly-calculated layouts.
+  if (nextLayout.requestedLayoutFromAbove == NO
+      && CGSizeEqualToSize(boundsSizeForLayout, nextLayout.layout.size) == NO) {
+    as_log_verbose(ASLayoutLog(), "Layout size doesn't match bounds size. Requesting layout from above.");
+    // The layout that we have specifies that this node (self) would like to be a different size
+    // than it currently is.  Because that size has been computed within the constrainedSize, we
+    // expect that calling setNeedsLayoutFromAbove will result in our parent resizing us to this.
+    // However, in some cases apps may manually interfere with this (setting a different bounds).
+    // In this case, we need to detect that we've already asked to be resized to match this
+    // particular ASLayout object, and shouldn't loop asking again unless we have a different ASLayout.
+    nextLayout.requestedLayoutFromAbove = YES;
+
+    {
+      __instanceLock__.unlock();
+      [self _u_setNeedsLayoutFromAbove];
+      __instanceLock__.lock();
+    }
+
+    // Update the layout's version here because _u_setNeedsLayoutFromAbove calls __setNeedsLayout which in turn increases _layoutVersion
+    // Failing to do this will cause the layout to be invalid immediately
+    nextLayout.version = _layoutVersion;
+  }
+
+  // Prepare to transition to nextLayout
+  ASDisplayNodeAssertNotNil(nextLayout.layout, @"nextLayout->layout should not be nil! %@", self);
+  _pendingLayoutTransition = [[ASLayoutTransition alloc] initWithNode:self
+                                                        pendingLayout:nextLayout
+                                                       previousLayout:_calculatedDisplayNodeLayout];
+
   // If a parent is currently executing a layout transition, perform our layout application after it.
-  if (isInLayoutPendingState == NO) {
+  if (ASHierarchyStateIncludesLayoutPending(_hierarchyState) == NO) {
     // If no transition, apply our new layout immediately (common case).
     [self _completePendingLayoutTransition];
   }
@@ -872,18 +868,12 @@ ASLayoutElementStyleExtensibilityForwarding
  */
 - (void)_completePendingLayoutTransition
 {
-  ASAssertUnlocked(__instanceLock__);
+  __instanceLock__.lock();
-
+  ASLayoutTransition *pendingLayoutTransition = _pendingLayoutTransition;
-  ASLayoutTransition *pendingLayoutTransition;
+  __instanceLock__.unlock();
-  {
-    ASDN::MutexLocker l(__instanceLock__);
-    pendingLayoutTransition = _pendingLayoutTransition;
-    if (pendingLayoutTransition != nil) {
-      [self _locked_setCalculatedDisplayNodeLayout:pendingLayoutTransition.pendingLayout];
-    }
-  }
 
   if (pendingLayoutTransition != nil) {
+    [self _setCalculatedDisplayNodeLayout:pendingLayoutTransition.pendingLayout];
     [self _completeLayoutTransition:pendingLayoutTransition];
     [self _pendingLayoutTransitionDidComplete];
   }
@@ -903,7 +893,8 @@ ASLayoutElementStyleExtensibilityForwarding
   // Trampoline to the main thread if necessary
   if (ASDisplayNodeThreadIsMain() || layoutTransition.isSynchronous == NO) {
     // Committing the layout transition will result in subnode insertions and removals, both of which must be called without the lock held
-    ASAssertUnlocked(__instanceLock__);
+    // TODO: Disabled due to PR: https://github.com/TextureGroup/Texture/pull/1204
+    // ASAssertUnlocked(__instanceLock__);
     [layoutTransition commitTransition];
   } else {
     // Subnode insertions and removals need to happen always on the main thread if at least one subnode is already loaded
@@ -963,7 +954,8 @@ ASLayoutElementStyleExtensibilityForwarding
 #endif
 
   // Subclass hook
-  ASAssertUnlocked(__instanceLock__);
+  // TODO: Disabled due to PR: https://github.com/TextureGroup/Texture/pull/1204
+  // ASAssertUnlocked(__instanceLock__);
   [self calculatedLayoutDidChange];
 
   // Grab lock after calling out to subclass

Source/ASDisplayNode.mm

@@ -2215,7 +2215,8 @@ ASDISPLAYNODE_INLINE BOOL subtreeIsRasterized(ASDisplayNode *node) {
 - (void)_insertSubnode:(ASDisplayNode *)subnode atSubnodeIndex:(NSInteger)subnodeIndex sublayerIndex:(NSInteger)sublayerIndex andRemoveSubnode:(ASDisplayNode *)oldSubnode
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASAssertUnlocked(__instanceLock__);
+  // TODO: Disabled due to PR: https://github.com/TextureGroup/Texture/pull/1204
+  // ASAssertUnlocked(__instanceLock__);
   
   as_log_verbose(ASNodeLog(), "Insert subnode %@ at index %zd of %@ and remove subnode %@", subnode, subnodeIndex, self, oldSubnode);
   
@@ -2431,7 +2432,8 @@ ASDISPLAYNODE_INLINE BOOL subtreeIsRasterized(ASDisplayNode *node) {
 - (void)_insertSubnode:(ASDisplayNode *)subnode belowSubnode:(ASDisplayNode *)below
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASAssertUnlocked(__instanceLock__);
+  // TODO: Disabled due to PR: https://github.com/TextureGroup/Texture/pull/1204
+  // ASAssertUnlocked(__instanceLock__);
 
   if (subnode == nil) {
     ASDisplayNodeFailAssert(@"Cannot insert a nil subnode");
@@ -2495,7 +2497,8 @@ ASDISPLAYNODE_INLINE BOOL subtreeIsRasterized(ASDisplayNode *node) {
 - (void)_insertSubnode:(ASDisplayNode *)subnode aboveSubnode:(ASDisplayNode *)above
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASAssertUnlocked(__instanceLock__);
+  // TODO: Disabled due to PR: https://github.com/TextureGroup/Texture/pull/1204
+  // ASAssertUnlocked(__instanceLock__);
 
   if (subnode == nil) {
     ASDisplayNodeFailAssert(@"Cannot insert a nil subnode");
@@ -2557,7 +2560,8 @@ ASDISPLAYNODE_INLINE BOOL subtreeIsRasterized(ASDisplayNode *node) {
 - (void)_insertSubnode:(ASDisplayNode *)subnode atIndex:(NSInteger)idx
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASAssertUnlocked(__instanceLock__);
+  // TODO: Disabled due to PR: https://github.com/TextureGroup/Texture/pull/1204
+  // ASAssertUnlocked(__instanceLock__);
   
   if (subnode == nil) {
     ASDisplayNodeFailAssert(@"Cannot insert a nil subnode");
@@ -2594,7 +2598,8 @@ ASDISPLAYNODE_INLINE BOOL subtreeIsRasterized(ASDisplayNode *node) {
 - (void)_removeSubnode:(ASDisplayNode *)subnode
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASAssertUnlocked(__instanceLock__);
+  // TODO: Disabled due to PR: https://github.com/TextureGroup/Texture/pull/1204
+  // ASAssertUnlocked(__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.
@@ -2620,7 +2625,8 @@ ASDISPLAYNODE_INLINE BOOL subtreeIsRasterized(ASDisplayNode *node) {
 - (void)_removeFromSupernode
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASAssertUnlocked(__instanceLock__);
+  // TODO: Disabled due to PR: https://github.com/TextureGroup/Texture/pull/1204
+  // ASAssertUnlocked(__instanceLock__);
   
   __instanceLock__.lock();
     __weak ASDisplayNode *supernode = _supernode;
@@ -2634,7 +2640,8 @@ ASDISPLAYNODE_INLINE BOOL subtreeIsRasterized(ASDisplayNode *node) {
 - (void)_removeFromSupernodeIfEqualTo:(ASDisplayNode *)supernode
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASAssertUnlocked(__instanceLock__);
+  // TODO: Disabled due to PR: https://github.com/TextureGroup/Texture/pull/1204
+  // ASAssertUnlocked(__instanceLock__);
   
   __instanceLock__.lock();