[ASDisplayNode] Fix an issue that causes a node to sometimes return an outdated calculated size or size range

CHANGELOG.md

@@ -53,6 +53,7 @@
 - Prevent UITextView from updating contentOffset while deallocating [Michael Schneider](https://github.com/maicki)
 - [ASCollectionNode/ASTableNode] Fix a crash occurs while remeasuring cell nodes. [Huy Nguyen](https://github.com/nguyenhuy) [#917](https://github.com/TextureGroup/Texture/pull/917)
 - Fix an issue where ASConfigurationDelegate would not call out for "control" users. If set, it now receives events whenever an experimental feature decision point occurs, whether it's enabled or not. [Adlai Holler](https://github.com/Adlai-Holler)
+- [ASDisplayNode] Fix an issue that causes a node to sometimes return an outdated calculated size or size range. [Huy Nguyen](https://github.com/nguyenhuy) [#808](https://github.com/TextureGroup/Texture/pull/808)
 
 ## 2.6
 - [Xcode 9] Updated to require Xcode 9 (to fix warnings) [Garrett Moon](https://github.com/garrettmoon)

Source/ASDisplayNode+Layout.mm

@@ -170,7 +170,7 @@ - (ASLayout *)calculatedLayout
 - (CGSize)calculatedSize
 {
   ASDN::MutexLocker l(__instanceLock__);
-  if (_pendingDisplayNodeLayout != nullptr) {
+  if (_pendingDisplayNodeLayout != nullptr && _pendingDisplayNodeLayout->isValid(_layoutVersion)) {
     return _pendingDisplayNodeLayout->layout.size;
   }
   return _calculatedDisplayNodeLayout->layout.size;
@@ -184,7 +184,7 @@ - (ASSizeRange)constrainedSizeForCalculatedLayout
 
 - (ASSizeRange)_locked_constrainedSizeForCalculatedLayout
 {
-  if (_pendingDisplayNodeLayout != nullptr) {
+  if (_pendingDisplayNodeLayout != nullptr && _pendingDisplayNodeLayout->isValid(_layoutVersion)) {
     return _pendingDisplayNodeLayout->constrainedSize;
   }
   return _calculatedDisplayNodeLayout->constrainedSize;
@@ -304,24 +304,22 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds
   }
 
   CGSize boundsSizeForLayout = ASCeilSizeValues(bounds.size);
-  NSUInteger calculatedVersion = _calculatedDisplayNodeLayout->version;
 
   // 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 != nullptr) {
+  if (_pendingDisplayNodeLayout != nullptr && _pendingDisplayNodeLayout->isValid(_layoutVersion)) {
+    NSUInteger calculatedVersion = _calculatedDisplayNodeLayout->version;
     NSUInteger pendingVersion = _pendingDisplayNodeLayout->version;
-    if (pendingVersion >= _layoutVersion) {
-      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 = (calculatedVersion >= _layoutVersion
+    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) {

Source/Private/ASDisplayNodeLayout.h

@@ -47,7 +47,12 @@ struct ASDisplayNodeLayout {
    */
   ASDisplayNodeLayout()
   : layout(nil), constrainedSize({{0, 0}, {0, 0}}), parentSize({0, 0}), requestedLayoutFromAbove(NO), version(0) {};
-
+
+  /**
+   * Returns whether this is valid for a given version
+   */
+  BOOL isValid(NSUInteger version);
+
   /**
    * Returns whether this is valid for a given constrained size, parent size, and version
    */

Source/Private/ASDisplayNodeLayout.mm

@@ -17,10 +17,14 @@
 
 #import <AsyncDisplayKit/ASDisplayNodeLayout.h>
 
+BOOL ASDisplayNodeLayout::isValid(NSUInteger versionArg)
+{
+  return layout != nil && version >= versionArg;
+}
+
 BOOL ASDisplayNodeLayout::isValid(ASSizeRange theConstrainedSize, CGSize theParentSize, NSUInteger versionArg)
 {
-  return version >= versionArg
-      && layout != nil
+  return isValid(versionArg)
       && CGSizeEqualToSize(parentSize, theParentSize)
       && ASSizeRangeEqualToSizeRange(constrainedSize, theConstrainedSize);
 }

Tests/ASLayoutEngineTests.mm

@@ -29,11 +29,12 @@ @implementation ASLayoutEngineTests {
   ASTLayoutFixture *fixture2;
   ASTLayoutFixture *fixture3;
   ASTLayoutFixture *fixture4;
+  ASTLayoutFixture *fixture5;
 
-  // fixtures 1 and 3 share the same exact node A layout spec block.
+  // fixtures 1, 3 and 5 share the same exact node A layout spec block.
   // we don't want the infra to call -setNeedsLayout when we switch fixtures
   // so we need to use the same exact block.
-  ASLayoutSpecBlock fixture1and3NodeALayoutSpecBlock;
+  ASLayoutSpecBlock fixture1and3and5NodeALayoutSpecBlock;
 
   UIWindow *window;
   UIViewController *vc;
@@ -68,11 +69,12 @@ - (void)setUp
   ASLayoutSpecBlock b = ^ASLayoutSpec * _Nonnull(__kindof ASDisplayNode * _Nonnull node, ASSizeRange constrainedSize) {
     return [ASStackLayoutSpec stackLayoutSpecWithDirection:ASStackLayoutDirectionHorizontal spacing:0 justifyContent:ASStackLayoutJustifyContentSpaceBetween alignItems:ASStackLayoutAlignItemsStart children:@[ nodeB, nodeC, nodeD ]];
   };
-  fixture1and3NodeALayoutSpecBlock = b;
+  fixture1and3and5NodeALayoutSpecBlock = b;
   fixture1 = [self createFixture1];
   fixture2 = [self createFixture2];
   fixture3 = [self createFixture3];
   fixture4 = [self createFixture4];
+  fixture5 = [self createFixture5];
 
   nodeA.frame = vc.view.bounds;
   nodeA.autoresizingMask = UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight;
@@ -181,6 +183,55 @@ - (void)testLayoutTransitionWithAsyncMeasurement
   [self verifyFixture:fixture2];
 }
 
+/**
+ * Transition from fixture1 to Fixture2 on node A.
+ *
+ * Expect A and D to calculate once on main, and
+ * to receive calculatedLayoutDidChange on main,
+ * then to get animateLayoutTransition: and didCompleteLayoutTransition: on main.
+ */
+- (void)testLayoutTransitionWithSyncMeasurement
+{
+  [self stubCalculatedLayoutDidChange];
+
+  // Precondition
+  XCTAssertFalse(CGSizeEqualToSize(fixture5.layout.size, fixture1.layout.size));
+
+  // First, apply fixture 5 and run a measurement pass, but don't run a layout pass
+  // After this step, nodes will have pending layouts that are not yet applied
+  [fixture5 apply];
+  [fixture5 withSizeRangesForAllNodesUsingBlock:^(ASLayoutTestNode * _Nonnull node, ASSizeRange sizeRange) {
+    OCMExpect([node.mock calculateLayoutThatFits:sizeRange])
+    .onMainThread();
+  }];
+
+  [nodeA layoutThatFits:ASSizeRangeMake(fixture5.layout.size)];
+
+  // Assert that node A has layout size and size range from fixture 5
+  XCTAssertTrue(CGSizeEqualToSize(fixture5.layout.size, nodeA.calculatedSize));
+  XCTAssertTrue(ASSizeRangeEqualToSizeRange([fixture5 firstSizeRangeForNode:nodeA], nodeA.constrainedSizeForCalculatedLayout));
+
+  // Then switch to fixture 1 and kick off a synchronous layout transition
+  // Unapplied pending layouts from the previous measurement pass will be outdated
+  [fixture1 apply];
+  [fixture1 withSizeRangesForAllNodesUsingBlock:^(ASLayoutTestNode * _Nonnull node, ASSizeRange sizeRange) {
+    OCMExpect([node.mock calculateLayoutThatFits:sizeRange])
+    .onMainThread();
+  }];
+
+  OCMExpect([nodeA.mock animateLayoutTransition:OCMOCK_ANY]).onMainThread();
+  OCMExpect([nodeA.mock didCompleteLayoutTransition:OCMOCK_ANY]).onMainThread();
+
+  [nodeA transitionLayoutWithAnimation:NO shouldMeasureAsync:NO measurementCompletion:nil];
+
+  // Assert that node A picks up new layout size and size range from fixture 1
+  XCTAssertTrue(CGSizeEqualToSize(fixture1.layout.size, nodeA.calculatedSize));
+  XCTAssertTrue(ASSizeRangeEqualToSizeRange([fixture1 firstSizeRangeForNode:nodeA], nodeA.constrainedSizeForCalculatedLayout));
+
+  [window layoutIfNeeded];
+  [self verifyFixture:fixture1];
+}
+
 /**
  * Start at fixture 1.
  * Trigger an async transition to fixture 2.
@@ -351,7 +402,7 @@ - (void)stubCalculatedLayoutDidChange
 /**
  * Fixture 1: A basic horizontal stack, all single-pass.
  *
- * [A: HorizStack([B, C, D])]. B is (1x1), C is (2x1), D is (1x1)
+ * [A: HorizStack([B, C, D])]. A is (10x1), B is (1x1), C is (2x1), D is (1x1)
  */
 - (ASTLayoutFixture *)createFixture1
 {
@@ -373,14 +424,14 @@ - (ASTLayoutFixture *)createFixture1
   auto layoutA = [ASLayout layoutWithLayoutElement:nodeA size:{10,1} position:ASPointNull sublayouts:@[ layoutB, layoutC, layoutD ]];
   fixture.layout = layoutA;
 
-  [fixture.layoutSpecBlocks setObject:fixture1and3NodeALayoutSpecBlock forKey:nodeA];
+  [fixture.layoutSpecBlocks setObject:fixture1and3and5NodeALayoutSpecBlock forKey:nodeA];
   return fixture;
 }
 
 /**
  * Fixture 2: A simple transition away from fixture 1.
  *
- * [A: HorizStack([B, C, E])]. B is (1x1), C is (4x1), E is (1x1)
+ * [A: HorizStack([B, C, E])]. A is (10x1), B is (1x1), C is (4x1), E is (1x1)
  *
  * From fixture 1:
  *   B survives with same layout
@@ -418,7 +469,7 @@ - (ASTLayoutFixture *)createFixture2
 /**
  * Fixture 3: Multipass stack layout
  *
- * [A: HorizStack([B, C, D])]. B is (7x1), C is (2x1), D is (1x1)
+ * [A: HorizStack([B, C, D])]. A is (10x1), B is (7x1), C is (2x1), D is (1x1)
  *
  * nodeB (which has flexShrink=1) will return 8x1 for its size during the first
  * stack pass, and it'll be subject to a second pass where it returns 7x1.
@@ -446,14 +497,14 @@ - (ASTLayoutFixture *)createFixture3
   auto layoutA = [ASLayout layoutWithLayoutElement:nodeA size:{10,1} position:ASPointNull sublayouts:@[ layoutB, layoutC, layoutD ]];
   fixture.layout = layoutA;
 
-  [fixture.layoutSpecBlocks setObject:fixture1and3NodeALayoutSpecBlock forKey:nodeA];
+  [fixture.layoutSpecBlocks setObject:fixture1and3and5NodeALayoutSpecBlock forKey:nodeA];
   return fixture;
 }
 
 /**
  * Fixture 4: A different simple transition away from fixture 1.
  *
- * [A: HorizStack([B, D, E])]. B is (1x1), D is (2x1), E is (1x1)
+ * [A: HorizStack([B, D, E])]. A is (10x1), B is (1x1), D is (2x1), E is (1x1)
  *
  * From fixture 1:
  *   B survives with same layout
@@ -494,18 +545,45 @@ - (ASTLayoutFixture *)createFixture4
   return fixture;
 }
 
+/**
+ * Fixture 5: Same as fixture 1, but with a bigger root node (node A).
+ *
+ * [A: HorizStack([B, C, D])]. A is (15x1), B is (1x1), C is (2x1), D is (1x1)
+ */
+- (ASTLayoutFixture *)createFixture5
+{
+  auto fixture = [[ASTLayoutFixture alloc] init];
+
+  // nodeB
+  [fixture addSizeRange:{{0, 0}, {INFINITY, 1}} forNode:nodeB];
+  auto layoutB = [ASLayout layoutWithLayoutElement:nodeB size:{1,1} position:{0,0} sublayouts:nil];
+
+  // nodeC
+  [fixture addSizeRange:{{0, 0}, {INFINITY, 1}} forNode:nodeC];
+  auto layoutC = [ASLayout layoutWithLayoutElement:nodeC size:{2,1} position:{4,0} sublayouts:nil];
+
+  // nodeD
+  [fixture addSizeRange:{{0, 0}, {INFINITY, 1}} forNode:nodeD];
+  auto layoutD = [ASLayout layoutWithLayoutElement:nodeD size:{1,1} position:{9,0} sublayouts:nil];
+
+  [fixture addSizeRange:{{15, 1}, {15, 1}} forNode:nodeA];
+  auto layoutA = [ASLayout layoutWithLayoutElement:nodeA size:{15,1} position:ASPointNull sublayouts:@[ layoutB, layoutC, layoutD ]];
+  fixture.layout = layoutA;
+
+  [fixture.layoutSpecBlocks setObject:fixture1and3and5NodeALayoutSpecBlock forKey:nodeA];
+  return fixture;
+}
+
 - (void)runFirstLayoutPassWithFixture:(ASTLayoutFixture *)fixture
 {
   [fixture apply];
-  for (ASLayoutTestNode *node in fixture.allNodes) {
-    [fixture withSizeRangesForNode:node block:^(ASSizeRange sizeRange) {
-      OCMExpect([node.mock calculateLayoutThatFits:sizeRange]).onMainThread();
-    }];
+  [fixture withSizeRangesForAllNodesUsingBlock:^(ASLayoutTestNode * _Nonnull node, ASSizeRange sizeRange) {
+    OCMExpect([node.mock calculateLayoutThatFits:sizeRange]).onMainThread();
 
     if (!stubbedCalculatedLayoutDidChange) {
       OCMExpect([node.mock calculatedLayoutDidChange]).onMainThread();
     }
-  }
+  }];
 
   // Trigger CA layout pass.
   [window layoutIfNeeded];

Tests/ASTLayoutFixture.h

@@ -44,6 +44,9 @@ AS_SUBCLASSING_RESTRICTED
 /// Get the first expected size range for the node.
 - (ASSizeRange)firstSizeRangeForNode:(ASLayoutTestNode *)node;
 
+/// Enumerate all the size ranges for all the nodes using the provided block.
+- (void)withSizeRangesForAllNodesUsingBlock:(void (^)(ASLayoutTestNode *node, ASSizeRange sizeRange))block;
+
 /// Enumerate all the size ranges for the node.
 - (void)withSizeRangesForNode:(ASLayoutTestNode *)node block:(void (^)(ASSizeRange sizeRange))block;
 

Tests/ASTLayoutFixture.mm

@@ -58,6 +58,15 @@ - (ASSizeRange)firstSizeRangeForNode:(ASLayoutTestNode *)node
   return r;
 }
 
+- (void)withSizeRangesForAllNodesUsingBlock:(void (^)(ASLayoutTestNode * _Nonnull, ASSizeRange))block
+{
+  for (ASLayoutTestNode *node in self.allNodes) {
+    [self withSizeRangesForNode:node block:^(ASSizeRange sizeRange) {
+      block(node, sizeRange);
+    }];
+  }
+}
+
 - (void)withSizeRangesForNode:(ASLayoutTestNode *)node block:(void (^)(ASSizeRange))block
 {
   for (NSValue *value in [_sizeRanges objectForKey:node]) {