iOS: gate CodePush-triggered reloads on a Fabric readiness signal

Author: ofalvai

ios/CodePush/CodePush.m

@@ -1,6 +1,7 @@
 #if __has_include(<React/RCTAssert.h>)
 #import <React/RCTAssert.h>
 #import <React/RCTBridgeModule.h>
+#import <React/RCTConstants.h>
 #import <React/RCTConvert.h>
 #import <React/RCTEventDispatcher.h>
 #import <React/RCTRootView.h>
@@ -36,6 +37,11 @@ @implementation CodePush {
     BOOL _allowed;
     BOOL _restartInProgress;
     NSMutableArray *_restartQueue;
+
+    // Reload readiness tracking. See -registerSettleObservers for details.
+    BOOL _instanceSettled;
+    // YES exactly while a reload is parked waiting for the instance to settle.
+    BOOL _reloadPending;
 }
 
 RCT_EXPORT_MODULE()
@@ -45,6 +51,11 @@ @implementation CodePush {
 // These constants represent emitted events
 static NSString *const DownloadProgressEvent = @"CodePushDownloadProgress";
 
+// How long a parked reload waits for a settle signal before reloading anyway.
+// See -registerSettleObservers for why this exists and why the exact value is
+// not critical.
+static const NSTimeInterval SettleTimeout = 2.0;
+
 // These constants represent valid deployment statuses
 static NSString *const DeploymentFailed = @"DeploymentFailed";
 static NSString *const DeploymentSucceeded = @"DeploymentSucceeded";
@@ -395,15 +406,104 @@ - (instancetype)init
     _allowed = YES;
     _restartInProgress = NO;
     _restartQueue = [NSMutableArray arrayWithCapacity:1];
-    
+
     self = [super init];
     if (self) {
+        [self registerSettleObservers];
         [self initializeUpdateAfterRestart];
     }
 
     return self;
 }
 
+#pragma mark - Reload readiness tracking
+
+/*
+ * Why a reload can be deferred, and why it can also time out.
+ *
+ * 1. The problem. A CodePush reload tears down the current RCTInstance. Right
+ *    after bundle evaluation, RN enqueues -[RCTFabricSurface start] on a
+ *    background queue for that same instance. Tearing the instance down while
+ *    that block is in flight crashes inside RN's mounting layer. The window
+ *    opens when the block is enqueued and closes a couple of queue hops later,
+ *    so it lasts under a millisecond when idle and tens of milliseconds under
+ *    load. CI simulators are exactly where it stretches.
+ *
+ * 2. So we wait it out. This module is initialized *during* bundle evaluation,
+ *    which is before the window opens, so every instance starts out unsettled
+ *    and -loadBundle parks the reload until we see the surface get past its
+ *    startup.
+ *
+ * 3. What we wait for. The two signals below are the terminal outcomes of an
+ *    instance's startup, the same pair expo-updates watches in its error
+ *    recovery flow:
+ *
+ *      - RCTContentDidAppearNotification: Fabric posts this from
+ *        RCTRootComponentView on the first child mount. That needs a JS render
+ *        and commit, so it lands well after the block we're racing.
+ *      - RCTJavaScriptDidFailToLoadNotification: the bundle never evaluated, so
+ *        no surface startup will follow.
+ *
+ * 4. Neither signal is guaranteed. A first render that returns nil mounts no
+ *    child, so RCTContentDidAppear never arrives — think of a splash gate or a
+ *    fonts/auth loader. That app shape also runs sync() with an IMMEDIATE
+ *    install before it shows any UI, which makes it the most likely caller to
+ *    hit the parked path in the first place.
+ *
+ * 5. Hence the timeout in -loadBundle. Without an escape, a parked reload never
+ *    fires, and since _restartInProgress stays YES it also blocks every later
+ *    restartApp() for the lifetime of the process. Letting the timeout expire
+ *    is safe: the window from step 1 shut long before it. SettleTimeout is
+ *    therefore generous on purpose, so the real signal wins whenever it exists
+ *    and the timeout stays a last resort.
+ *
+ * There is deliberately nothing here to mark an instance unsettled again. A
+ * reload from any source destroys the RCTInstance and rebuilds its TurboModules,
+ * so the module observing these notifications never survives one.
+ */
+- (void)registerSettleObservers
+{
+    NSNotificationCenter *center = [NSNotificationCenter defaultCenter];
+    [center addObserver:self
+               selector:@selector(instanceDidSettle)
+                   name:RCTContentDidAppearNotification
+                 object:nil];
+    [center addObserver:self
+               selector:@selector(instanceDidSettle)
+                   name:RCTJavaScriptDidFailToLoadNotification
+                 object:nil];
+}
+
+- (void)instanceDidSettle
+{
+    // A reload tears the instance down synchronously, so releasing the reload
+    // inline would destroy the surface partway through the mount that just
+    //  notified us.
+    dispatch_async(dispatch_get_main_queue(), ^{
+        self->_instanceSettled = YES;
+
+        if (self->_reloadPending) {
+            CPLog(@"Instance settled. Restarting app.");
+            [self releasePendingReload];
+        }
+    });
+}
+
+/*
+ * Fires a parked reload, if one is still parked. Called both by the settle
+ * signal and by the timeout in -loadBundle, so whichever arrives first wins and
+ * the other becomes a no-op. Main queue only.
+ */
+- (void)releasePendingReload
+{
+    if (!_reloadPending) {
+        return;
+    }
+
+    _reloadPending = NO;
+    [self performBundleReload];
+}
+
 /*
  * This method is used when the app is started to either
  * initialize a pending update or rollback a faulty update
@@ -547,18 +647,49 @@ - (void)loadBundle
     // This needs to be async dispatched because the bridge is not set on init
     // when the app first starts, therefore rollbacks will not take effect.
     dispatch_async(dispatch_get_main_queue(), ^{
-        // If the current bundle URL is using http(s), then assume the dev
-        // is debugging and therefore, shouldn't be redirected to a local
-        // file (since Chrome wouldn't support it). Otherwise, update
-        // the current bundle URL to point at the latest update
-        if ([CodePush isUsingTestConfiguration] || ![super.bridge.bundleURL.scheme hasPrefix:@"http"]) {
-            [super.bridge setValue:[CodePush bundleURL] forKey:@"bundleURL"];
+        if (!self->_instanceSettled) {
+            // Reloading now could tear this instance down while its Fabric surface
+            // is still starting up, so park the reload until the instance settles.
+            // The timeout below is the escape hatch for a settle signal that never
+            // comes. Both are explained in -registerSettleObservers.
+            CPLog(@"Restart deferred until the current instance has settled.");
+            self->_reloadPending = YES;
+
+            // Weak, so that a module outliving the teardown of its own instance
+            // cannot reload against a bridge that no longer belongs to it.
+            __weak __typeof(self) weakSelf = self;
+            dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(SettleTimeout * NSEC_PER_SEC)),
+                           dispatch_get_main_queue(), ^{
+                __typeof(self) strongSelf = weakSelf;
+                if (strongSelf && strongSelf->_reloadPending) {
+                    CPLog(@"Timed out waiting for the current instance to settle. Restarting app anyway.");
+                    [strongSelf releasePendingReload];
+                }
+            });
+            return;
         }
 
-        RCTTriggerReloadCommandListeners(@"react-native-code-push: Restart");
+        [self performBundleReload];
     });
 }
 
+/*
+ * Performs the actual reload. Must be called on the main queue, and only once the
+ * current instance has settled or the wait for it timed out. See -loadBundle.
+ */
+- (void)performBundleReload
+{
+    // If the current bundle URL is using http(s), then assume the dev
+    // is debugging and therefore, shouldn't be redirected to a local
+    // file (since Chrome wouldn't support it). Otherwise, update
+    // the current bundle URL to point at the latest update
+    if ([CodePush isUsingTestConfiguration] || ![super.bridge.bundleURL.scheme hasPrefix:@"http"]) {
+        [super.bridge setValue:[CodePush bundleURL] forKey:@"bundleURL"];
+    }
+
+    RCTTriggerReloadCommandListeners(@"react-native-code-push: Restart");
+}
+
 /*
  * This method is used when a pending update never finished loading (i.e. it
  * crashed before calling notifyApplicationReady) and needs to be rolled back
@@ -804,7 +935,7 @@ - (void)restartAppInternal:(BOOL)onlyIfUpdateIsPending
 
     _restartInProgress = NO;
     if ([_restartQueue count] > 0) {
-        BOOL buf = [_restartQueue valueForKey: @"@firstObject"];
+        BOOL buf = [[_restartQueue firstObject] boolValue];
         [_restartQueue removeObjectAtIndex:0];
         [self restartAppInternal:buf];
     }
@@ -1010,7 +1141,7 @@ - (void)restartAppInternal:(BOOL)onlyIfUpdateIsPending
 
     if ([_restartQueue count] > 0) {
         CPLog(@"Executing pending restart.");
-        BOOL buf = [_restartQueue valueForKey: @"@firstObject"];
+        BOOL buf = [[_restartQueue firstObject] boolValue];
         [_restartQueue removeObjectAtIndex:0];
         [self restartAppInternal:buf];
     }