Deterministic onLayout event ordering for iOS Paper (#40748)

Summary: The ordering of `onLayout` events is non-deterministic on iOS Paper, due to nodes being added to an `NSHashTable` before iteration, instead of an ordered collection. We don't do any lookups on the collection, so I think this was chosen over `NSMutableArray` for the sake of `[NSHashTable weakObjectsHashTable]`, to avoid retain/release. Using a collection which does retain/release seems to cause a crash due to double release or similar, so those semantics seem intentional (though I'm not super familiar with the model here). We can replicate the memory semantics with ordering by using `NSPointerArray` (which is unfortunately not parameterized). This change does that, so we get consistently top-down layout events (matching Fabric, and Android Paper as of D49627996). This lets us use multiple layout events to calculate right/bottom edge insets deterministically. Changelog: [iOS][Changed] - Deterministic onLayout event ordering for iOS Paper Reviewed By: luluwu2032 Differential Revision: D50093411
facebook · Oct 10, 2023 · 9cc8cb4 · 9cc8cb4
1 parent 44f5989
commit 9cc8cb4
Show file tree

Hide file tree

Showing 8 changed files with 9 additions and 9 deletions.
diff --git a/packages/react-native/React/Base/Surface/RCTSurfaceRootShadowView.h b/packages/react-native/React/Base/Surface/RCTSurfaceRootShadowView.h
@@ -27,6 +27,6 @@
  */
 @property (nonatomic, assign) YGDirection baseDirection;
 
-- (void)layoutWithAffectedShadowViews:(NSHashTable<RCTShadowView *> *)affectedShadowViews;
+- (void)layoutWithAffectedShadowViews:(NSPointerArray *)affectedShadowViews;
 
 @end
diff --git a/packages/react-native/React/Base/Surface/RCTSurfaceRootShadowView.m b/packages/react-native/React/Base/Surface/RCTSurfaceRootShadowView.m
@@ -41,7 +41,7 @@ - (void)insertReactSubview:(RCTShadowView *)subview atIndex:(NSInteger)atIndex
   }
 }
 
-- (void)layoutWithAffectedShadowViews:(NSHashTable<RCTShadowView *> *)affectedShadowViews
+- (void)layoutWithAffectedShadowViews:(NSPointerArray *)affectedShadowViews
 {
   NSHashTable<NSString *> *other = [NSHashTable new];
 

diff --git a/packages/react-native/React/Modules/RCTUIManager.m b/packages/react-native/React/Modules/RCTUIManager.m
@@ -534,7 +534,7 @@ - (RCTViewManagerUIBlock)uiBlockWithLayoutUpdateForRootView:(RCTRootShadowView *
 {
   RCTAssertUIManagerQueue();
 
-  NSHashTable<RCTShadowView *> *affectedShadowViews = [NSHashTable weakObjectsHashTable];
+  NSPointerArray *affectedShadowViews = [NSPointerArray weakObjectsPointerArray];
   [rootShadowView layoutWithAffectedShadowViews:affectedShadowViews];
 
   if (!affectedShadowViews.count) {

diff --git a/packages/react-native/React/Views/RCTLayout.h b/packages/react-native/React/Views/RCTLayout.h
@@ -31,7 +31,7 @@ typedef struct CG_BOXABLE RCTLayoutMetrics RCTLayoutMetrics;
 
 struct RCTLayoutContext {
   CGPoint absolutePosition;
-  __unsafe_unretained NSHashTable<RCTShadowView *> *_Nonnull affectedShadowViews;
+  __unsafe_unretained NSPointerArray *_Nonnull affectedShadowViews;
   __unsafe_unretained NSHashTable<NSString *> *_Nonnull other;
 };
 typedef struct CG_BOXABLE RCTLayoutContext RCTLayoutContext;

diff --git a/packages/react-native/React/Views/RCTRootShadowView.h b/packages/react-native/React/Views/RCTRootShadowView.h
@@ -29,6 +29,6 @@
  */
 @property (nonatomic, assign) YGDirection baseDirection;
 
-- (void)layoutWithAffectedShadowViews:(NSHashTable<RCTShadowView *> *)affectedShadowViews;
+- (void)layoutWithAffectedShadowViews:(NSPointerArray *)affectedShadowViews;
 
 @end
diff --git a/packages/react-native/React/Views/RCTRootShadowView.m b/packages/react-native/React/Views/RCTRootShadowView.m
@@ -23,7 +23,7 @@ - (instancetype)init
   return self;
 }
 
-- (void)layoutWithAffectedShadowViews:(NSHashTable<RCTShadowView *> *)affectedShadowViews
+- (void)layoutWithAffectedShadowViews:(NSPointerArray *)affectedShadowViews
 {
   NSHashTable<NSString *> *other = [NSHashTable new];
 

diff --git a/packages/react-native/React/Views/RCTShadowView.m b/packages/react-native/React/Views/RCTShadowView.m
@@ -304,7 +304,7 @@ - (void)layoutWithMetrics:(RCTLayoutMetrics)layoutMetrics layoutContext:(RCTLayo
 {
   if (!RCTLayoutMetricsEqualToLayoutMetrics(self.layoutMetrics, layoutMetrics)) {
     self.layoutMetrics = layoutMetrics;
-    [layoutContext.affectedShadowViews addObject:self];
+    [layoutContext.affectedShadowViews addPointer:((__bridge void *)self)];
   }
 }
 

diff --git a/packages/rn-tester/RNTesterUnitTests/RCTShadowViewTests.m b/packages/rn-tester/RNTesterUnitTests/RCTShadowViewTests.m
@@ -84,7 +84,7 @@ - (void)testApplyingLayoutRecursivelyToShadowView
   [self.parentView insertReactSubview:mainView atIndex:1];
   [self.parentView insertReactSubview:footerView atIndex:2];
 
-  [self.parentView layoutWithAffectedShadowViews:[NSHashTable weakObjectsHashTable]];
+  [self.parentView layoutWithAffectedShadowViews:[NSPointerArray weakObjectsPointerArray]];
 
   XCTAssertTrue(
       CGRectEqualToRect([self.parentView measureLayoutRelativeToAncestor:self.parentView], CGRectMake(0, 0, 440, 440)));
@@ -187,7 +187,7 @@ - (void)_withShadowViewWithStyle:(void (^)(YGNodeRef node))configBlock
   RCTShadowView *view = [self _shadowViewWithConfig:configBlock];
   [self.parentView insertReactSubview:view atIndex:0];
   view.intrinsicContentSize = contentSize;
-  [self.parentView layoutWithAffectedShadowViews:[NSHashTable weakObjectsHashTable]];
+  [self.parentView layoutWithAffectedShadowViews:[NSPointerArray weakObjectsPointerArray]];
   CGRect actualRect = [view measureLayoutRelativeToAncestor:self.parentView];
   XCTAssertTrue(
       CGRectEqualToRect(expectedRect, actualRect),
-Original file line number
+Diff line change
@@ Expand Up @@
       }
     }
-    - (void)layoutWithAffectedShadowViews:(NSHashTable<RCTShadowView *> *)affectedShadowViews
+    - (void)layoutWithAffectedShadowViews:(NSPointerArray *)affectedShadowViews
     {
       NSHashTable<NSString *> *other = [NSHashTable new];
@@ Expand Down @@