Deterministic onLayout event ordering for iOS Paper (#40748)

NickGerleman · facebook-github-bot · commit 56ddace9c8ff · 2023-10-10T11:43:14.000-07:00
Summary: Pull Request resolved: #40748 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 fbshipit-source-id: f6a9d5c973b97aede879baa8b952cc1be2447f28
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 number	Diff line change
`@@ -41,7 +41,7 @@ - (void)insertReactSubview:(RCTShadowView *)subview atIndex:(NSInteger)atIndex`
`41`	`41`	`}`
`42`	`42`	`}`
`43`	`43`
`44`		`-- (void)layoutWithAffectedShadowViews:(NSHashTable<RCTShadowView > )affectedShadowViews`
	`44`	`+- (void)layoutWithAffectedShadowViews:(NSPointerArray *)affectedShadowViews`
`45`	`45`	`{`
`46`	`46`	`NSHashTable<NSString > other = [NSHashTable new];`
`47`	`47`
Original file line number	Diff line number	Diff line change
`@@ -534,7 +534,7 @@ - (RCTViewManagerUIBlock)uiBlockWithLayoutUpdateForRootView:(RCTRootShadowView *`
`534`	`534`	`{`
`535`	`535`	`RCTAssertUIManagerQueue();`
`536`	`536`
`537`		`- NSHashTable<RCTShadowView > affectedShadowViews = [NSHashTable weakObjectsHashTable];`
	`537`	`+ NSPointerArray *affectedShadowViews = [NSPointerArray weakObjectsPointerArray];`
`538`	`538`	`[rootShadowView layoutWithAffectedShadowViews:affectedShadowViews];`
`539`	`539`
`540`	`540`	`if (!affectedShadowViews.count) {`
Original file line number	Diff line number	Diff line change
`@@ -23,7 +23,7 @@ - (instancetype)init`
`23`	`23`	`return self;`
`24`	`24`	`}`
`25`	`25`
`26`		`-- (void)layoutWithAffectedShadowViews:(NSHashTable<RCTShadowView > )affectedShadowViews`
	`26`	`+- (void)layoutWithAffectedShadowViews:(NSPointerArray *)affectedShadowViews`
`27`	`27`	`{`
`28`	`28`	`NSHashTable<NSString > other = [NSHashTable new];`
`29`	`29`
Original file line number	Diff line number	Diff line change
`@@ -304,7 +304,7 @@ - (void)layoutWithMetrics:(RCTLayoutMetrics)layoutMetrics layoutContext:(RCTLayo`
`304`	`304`	`{`
`305`	`305`	`if (!RCTLayoutMetricsEqualToLayoutMetrics(self.layoutMetrics, layoutMetrics)) {`
`306`	`306`	`self.layoutMetrics = layoutMetrics;`
`307`		`- [layoutContext.affectedShadowViews addObject:self];`
	`307`	`+ [layoutContext.affectedShadowViews addPointer:((__bridge void *)self)];`
`308`	`308`	`}`
`309`	`309`	`}`
`310`	`310`