From ecd2feeea0f69a78e59a19344824bb6961fc7f50 Mon Sep 17 00:00:00 2001
From: Ramanpreet Nara <ramanpreet@meta.com>
Date: Mon, 21 Aug 2023 12:43:42 -0700
Subject: [PATCH] Fix: React Native teardown crashes app

Summary:
During React Native teardown, we should stop all React surfaces. Otherwise, the app could crash with this error:

```
08-06 14:54:08.644 14843 14843 F DEBUG   : Abort message: 'xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/scheduler/Scheduler.cpp:171: function ~Scheduler: assertion failed (surfaceIds.empty() && "Scheduler was destroyed with outstanding Surfaces.")'
```

When can teardown occur? One case: an exception occurs on the NativeModules thread.

NOTE: This diff impacts the **new** Bridgeless mode lifecycle methods.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D47926966

fbshipit-source-id: 62be90eb49091773976dfb18db5c2a7c0668c382
---
 .../react/bridgeless/ReactHostImpl.java       | 73 ++++++++++++++-----
 1 file changed, 54 insertions(+), 19 deletions(-)

diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridgeless/ReactHostImpl.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridgeless/ReactHostImpl.java
index ce900d70e80..4d136512d09 100644
--- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridgeless/ReactHostImpl.java
+++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridgeless/ReactHostImpl.java
@@ -1116,6 +1116,25 @@ public class ReactHostImpl implements ReactHost {
     mBridgelessReactStateTracker.enterState("ReactHost{" + mId + "}." + method);
   }
 
+  private void stopAttachedSurfaces(String method, ReactInstance reactInstance) {
+    log(method, "Stopping all React Native surfaces");
+    synchronized (mAttachedSurfaces) {
+      for (ReactSurfaceImpl surface : mAttachedSurfaces) {
+        reactInstance.stopSurface(surface);
+        surface.clear();
+      }
+    }
+  }
+
+  private void startAttachedSurfaces(String method, ReactInstance reactInstance) {
+    log(method, "Restarting previously running React Native Surfaces");
+    synchronized (mAttachedSurfaces) {
+      for (ReactSurfaceImpl surface : mAttachedSurfaces) {
+        reactInstance.startSurface(surface);
+      }
+    }
+  }
+
   @ThreadConfined("ReactHost")
   private @Nullable Task<ReactInstance> mReloadTask = null;
 
@@ -1179,18 +1198,11 @@ public class ReactHostImpl implements ReactHost {
               .continueWithTask(
                   task -> {
                     final ReactInstance reactInstance = task.getResult();
-
-                    log(method, "Stopping all React Native surfaces");
-                    synchronized (mAttachedSurfaces) {
-                      for (ReactSurfaceImpl surface : mAttachedSurfaces) {
-                        if (reactInstance != null) {
-                          reactInstance.stopSurface(surface);
-                        }
-
-                        surface.clear();
-                      }
+                    if (reactInstance == null) {
+                      return task;
                     }
 
+                    stopAttachedSurfaces(method, reactInstance);
                     return task;
                   },
                   mBGExecutor)
@@ -1240,15 +1252,11 @@ public class ReactHostImpl implements ReactHost {
               .onSuccess(
                   task -> {
                     final ReactInstance reactInstance = task.getResult();
-                    if (reactInstance != null) {
-                      log(method, "Restarting previously running React Native Surfaces");
-
-                      synchronized (mAttachedSurfaces) {
-                        for (ReactSurfaceImpl surface : mAttachedSurfaces) {
-                          reactInstance.startSurface(surface);
-                        }
-                      }
+                    if (reactInstance == null) {
+                      return null;
                     }
+
+                    startAttachedSurfaces(method, reactInstance);
                     return reactInstance;
                   },
                   mBGExecutor)
@@ -1342,7 +1350,34 @@ public class ReactHostImpl implements ReactHost {
                     log(method, "Move ReactHost to onHostDestroy()");
                     mReactLifecycleStateManager.moveToOnHostDestroy(reactContext);
 
-                    // Step 3: De-register the memory pressure listener
+                    return task;
+                  },
+                  mUIExecutor)
+              .continueWithTask(
+                  task -> {
+                    final ReactInstance reactInstance = task.getResult();
+                    if (reactInstance == null) {
+                      return task;
+                    }
+
+                    // Step 3: Stop all React Native surfaces
+                    stopAttachedSurfaces(method, reactInstance);
+
+                    // TODO(T161461674): Should we clear mAttachedSurfaces?
+                    // Not clearing mAttachedSurfaces could lead to a memory leak.
+
+                    return task;
+                  },
+                  mBGExecutor)
+              .continueWithTask(
+                  task -> {
+                    final ReactContext reactContext = mBridgelessReactContextRef.getNullable();
+
+                    if (reactContext == null) {
+                      raiseSoftException(method, "ReactContext is null. Destroy reason: " + reason);
+                    }
+
+                    // Step 4: De-register the memory pressure listener
                     log(method, "Destroying MemoryPressureRouter");
                     mMemoryPressureRouter.destroy(mContext);