Stricter locking assertions

CHANGELOG.md

@@ -18,6 +18,7 @@
 - [IGListKit] Adds missing UIScrollViewDelegate method to DataSource proxy [Sergey Pronin](https://github.com/wannabehero) 
 - Fix misleading/scary stack trace shown when an assertion occurs during node measurement [Huy Nguyen](https://github.com/nguyenhuy) [#1022](https://github.com/TextureGroup/Texture/pull/1022)
 - Fix build on 32-bit simulator in Xcode 9.3 and later, caused by `Thread-local storage is not supported on this architecture.` [Adlai Holler](https://github.com/Adlai-Holler)
+- Enable locking assertions (and add some more) to improve and enforce locking safety within the framework [Huy Nguyen](https://github.com/nguyenhuy) [#1024](https://github.com/TextureGroup/Texture/pull/1024)
 
 ## 2.7
 - Fix pager node for interface coalescing. [Max Wang](https://github.com/wsdwsd0829) [#877](https://github.com/TextureGroup/Texture/pull/877)

Source/ASDisplayNode+Layout.mm

@@ -184,6 +184,7 @@ - (ASSizeRange)constrainedSizeForCalculatedLayout
 
 - (ASSizeRange)_locked_constrainedSizeForCalculatedLayout
 {
+  ASAssertLocked(__instanceLock__);
   if (_pendingDisplayNodeLayout != nullptr && _pendingDisplayNodeLayout->isValid(_layoutVersion)) {
     return _pendingDisplayNodeLayout->constrainedSize;
   }
@@ -218,9 +219,10 @@ @implementation ASDisplayNode (ASLayoutInternal)
  */
 - (void)_u_setNeedsLayoutFromAbove
 {
-  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock);
-  as_activity_create_for_scope("Set needs layout from above");
   ASDisplayNodeAssertThreadAffinity(self);
+  ASAssertUnlocked(__instanceLock__);
+
+  as_activity_create_for_scope("Set needs layout from above");
 
   // Mark the node for layout in the next layout pass
   [self setNeedsLayout];
@@ -243,7 +245,7 @@ - (void)_u_setNeedsLayoutFromAbove
 - (void)_rootNodeDidInvalidateSize
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
+  ASAssertUnlocked(__instanceLock__);
 
   __instanceLock__.lock();
 
@@ -273,7 +275,7 @@ - (void)_rootNodeDidInvalidateSize
 - (void)displayNodeDidInvalidateSizeNewSize:(CGSize)size
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
+  ASAssertUnlocked(__instanceLock__);
 
   // The default implementation of display node changes the size of itself to the new size
   CGRect oldBounds = self.bounds;
@@ -295,19 +297,19 @@ - (void)displayNodeDidInvalidateSizeNewSize:(CGSize)size
 
 - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds
 {
-  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock);
+  ASAssertUnlocked(__instanceLock__);
 
   BOOL isInLayoutPendingState = NO;
   {
     ASDN::MutexLocker l(__instanceLock__);
     // Check if we are a subnode in a layout transition.
     // In this case no measurement is needed as it's part of the layout transition
-    if ([self _isLayoutTransitionInvalid]) {
+    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;
@@ -328,38 +330,42 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds
     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.load());
+    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
     std::shared_ptr<ASDisplayNodeLayout> nextLayout = _pendingDisplayNodeLayout;
-#define layoutSizeDifferentFromBounds !CGSizeEqualToSize(nextLayout->layout.size, boundsSizeForLayout)
-    
+    #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 == nullptr) {
       as_log_verbose(ASLayoutLog(), "No pending layout.");
-    } else if (nextLayout->version < _layoutVersion) {
+    } else if (nextLayout->isValid(_layoutVersion) == NO) {
       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).
@@ -373,11 +379,11 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds
       // Release it and any orphaned subnodes it retains
       _pendingDisplayNodeLayout = nullptr;
     }
-    
+
     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
@@ -390,41 +396,52 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)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();
+
+      {
+        ASDN::MutexUnlocker u(__instanceLock__);
+        [self _u_setNeedsLayoutFromAbove];
+      }
+
       // 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);
   }
-  
+
   // If a parent is currently executing a layout transition, perform our layout application after it.
   if (isInLayoutPendingState == NO) {
     // If no transition, apply our new layout immediately (common case).
     [self _completePendingLayoutTransition];
   }
 }
 
+- (ASSizeRange)_constrainedSizeForLayoutPass
+{
+  ASDN::MutexLocker l(__instanceLock__);
+  return [self _locked_constrainedSizeForLayoutPass];
+}
+
 - (ASSizeRange)_locked_constrainedSizeForLayoutPass
 {
   // TODO: The logic in -_u_setNeedsLayoutFromAbove seems correct and doesn't use this method.
   // logic seems correct.  For what case does -this method need to do the CGSizeEqual checks?
   // IF WE CAN REMOVE BOUNDS CHECKS HERE, THEN WE CAN ALSO REMOVE "REQUESTED FROM ABOVE" CHECK
-
+
+  ASAssertLocked(__instanceLock__);
+
   CGSize boundsSizeForLayout = ASCeilSizeValues(self.threadSafeBounds.size);
-  
+
   // Checkout if constrained size of pending or calculated display node layout can be used
   if (_pendingDisplayNodeLayout != nullptr
       && (_pendingDisplayNodeLayout->requestedLayoutFromAbove
-           || CGSizeEqualToSize(_pendingDisplayNodeLayout->layout.size, boundsSizeForLayout))) {
+          || CGSizeEqualToSize(_pendingDisplayNodeLayout->layout.size, boundsSizeForLayout))) {
     // We assume the size from the last returned layoutThatFits: layout was applied so use the pending display node
     // layout constrained size
     return _pendingDisplayNodeLayout->constrainedSize;
@@ -444,7 +461,7 @@ - (ASSizeRange)_locked_constrainedSizeForLayoutPass
 - (void)_layoutSublayouts
 {
   ASDisplayNodeAssertThreadAffinity(self);
-  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
+  ASAssertUnlocked(__instanceLock__);
 
   ASLayout *layout;
   {
@@ -503,6 +520,7 @@ - (BOOL)_isLayoutTransitionInvalid
 
 - (BOOL)_locked_isLayoutTransitionInvalid
 {
+  ASAssertLocked(__instanceLock__);
   if (ASHierarchyStateIncludesLayoutPending(_hierarchyState)) {
     ASLayoutElementContext *context = ASLayoutElementGetCurrentContext();
     if (context == nil || _pendingTransitionID != context.transitionID) {
@@ -535,18 +553,10 @@ - (void)transitionLayoutWithAnimation:(BOOL)animated
                 measurementCompletion:(void(^)())completion
 {
   ASDisplayNodeAssertMainThread();
-
-  ASSizeRange sizeRange;
-  {
-    ASDN::MutexLocker l(__instanceLock__);
-    sizeRange = [self _locked_constrainedSizeForLayoutPass];
-  }
-
-  [self transitionLayoutWithSizeRange:sizeRange
+  [self transitionLayoutWithSizeRange:[self _constrainedSizeForLayoutPass]
                              animated:animated
                    shouldMeasureAsync:shouldMeasureAsync
                 measurementCompletion:completion];
-
 }
 
 - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize
@@ -653,9 +663,9 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize
         // Update calculated layout
         let previousLayout = _calculatedDisplayNodeLayout;
         let pendingLayout = std::make_shared<ASDisplayNodeLayout>(newLayout,
-                                                                   constrainedSize,
-                                                                   constrainedSize.max,
-                                                                   newLayoutVersion);
+                                                                  constrainedSize,
+                                                                  constrainedSize.max,
+                                                                  newLayoutVersion);
         [self _locked_setCalculatedDisplayNodeLayout:pendingLayout];
 
         // Setup pending layout transition for animation
@@ -865,8 +875,8 @@ - (void)didCompleteLayoutTransition:(id<ASContextTransitioning>)context
  */
 - (void)_completePendingLayoutTransition
 {
-  ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock);
-  
+  ASAssertUnlocked(__instanceLock__);
+
   ASLayoutTransition *pendingLayoutTransition = nil;
   {
     ASDN::MutexLocker l(__instanceLock__);
@@ -875,7 +885,7 @@ - (void)_completePendingLayoutTransition
       [self _locked_setCalculatedDisplayNodeLayout:pendingLayoutTransition.pendingLayout];
     }
   }
-  
+
   if (pendingLayoutTransition != nil) {
     [self _completeLayoutTransition:pendingLayoutTransition];
     [self _pendingLayoutTransitionDidComplete];
@@ -895,6 +905,8 @@ - (void)_completeLayoutTransition:(ASLayoutTransition *)layoutTransition
 
   // 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__);
     [layoutTransition commitTransition];
   } else {
     // Subnode insertions and removals need to happen always on the main thread if at least one subnode is already loaded
@@ -948,12 +960,13 @@ - (void)_assertSubnodeState
 - (void)_pendingLayoutTransitionDidComplete
 {
   // This assertion introduces a breaking behavior for nodes that has ASM enabled but also manually manage some subnodes.
-  // Let's gate it behind YOGA flag and remove it right after a branch cut.
+  // Let's gate it behind YOGA flag.
 #if YOGA
   [self _assertSubnodeState];
 #endif
 
   // Subclass hook
+  ASAssertUnlocked(__instanceLock__);
   [self calculatedLayoutDidChange];
 
   // Grab lock after calling out to subclass
@@ -998,6 +1011,7 @@ - (void)_setCalculatedDisplayNodeLayout:(std::shared_ptr<ASDisplayNodeLayout>)di
 
 - (void)_locked_setCalculatedDisplayNodeLayout:(std::shared_ptr<ASDisplayNodeLayout>)displayNodeLayout
 {
+  ASAssertLocked(__instanceLock__);
   ASDisplayNodeAssertTrue(displayNodeLayout->layout.layoutElement == self);
   ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.width >= 0.0);
   ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.height >= 0.0);