Skip to content

Commit f10cded

Browse files
Ilya Kovalyovmeta-codesync[bot]
authored andcommitted
Fix activity context leak in LogBoxDialogSurfaceDelegate (facebook#56232)
Summary: Pull Request resolved: facebook#56232 ## Problem LeakCanary detected a memory leak in React Native Android apps using LogBox in bridgeless mode where a destroyed Activity is retained through the LogBox surface delegate chain: ``` GC Root: Global variable in native code │ ├─ LogBoxModule instance │ ↓ LogBoxModule.surfaceDelegate ├─ LogBoxDialogSurfaceDelegate instance │ ↓ LogBoxDialogSurfaceDelegate.reactRootView ├─ ReactSurfaceView instance │ View.mContext references a destroyed activity │ ↓ View.mContext ╰→ Activity instance (mDestroyed = true) ``` ## Root Cause The leak has two contributing factors: 1. **`hide()` doesn't clean up the root view** — `LogBoxDialogSurfaceDelegate.hide()` dismisses the dialog but keeps `reactRootView` alive, retaining a reference to the destroyed Activity via `View.mContext`. 2. **`destroyRootView()` is a no-op in bridgeless mode** — `ReactHostImplDevHelper.destroyRootView()` did nothing, so the ReactSurface was never detached from `ReactHostImpl.attachedSurfaces`. Factor 2 made Factor 1 unfixable: calling `destroyContentView()` in `hide()` would null `reactRootView`, but the surface remained registered as attached. On the next `show()` call, `createRootView("LogBox")` would return null (because `isSurfaceWithModuleNameAttached("LogBox")` was still true), breaking LogBox reopen entirely. ## Fix ### 1. Implement `destroyRootView()` in `ReactHostImplDevHelper` The bridgeless `destroyRootView()` was a no-op. The fix casts the root view to `ReactSurfaceView` to access the underlying `ReactSurfaceImpl`, then properly tears it down: - `surface.stop()` — synchronously removes from `attachedSurfaces` - `surface.detach()` — nulls host back-reference - `surface.clear()` — removes child views To enable this, `ReactSurfaceView.surface` visibility is changed from `private` to `internal` (both classes are in the same `com.facebook.react.runtime` package). ### 2. Add `destroyContentView()` to `hide()` in `LogBoxDialogSurfaceDelegate` Now that `destroyRootView()` properly detaches the surface, `hide()` safely destroys the content view, breaking the reference chain that retained the destroyed Activity. ### 3. Make `show()` self-sufficient `show()` now recreates the content view when it's missing (e.g., after `hide()` destroyed it), instead of returning early. ## Why this is safe 1. **LogBox reopen works.** After `hide()` detaches the surface, `createRootView("LogBox")` succeeds because `isSurfaceWithModuleNameAttached("LogBox")` returns false. 2. **`destroyContentView()` is idempotent.** Null-checks `reactRootView` before cleanup. 3. **`destroyRootView()` is safe for non-ReactSurfaceView.** The `as?` cast returns null for bridge-mode `ReactRootView` instances — bridge mode already has its own implementation. 4. **Dev-only impact.** LogBox is excluded from release builds. 5. **`internal` visibility is appropriate.** Both `ReactSurfaceView` and `ReactHostImplDevHelper` are in the same package. Changelog: [Android][Fixed] - Fixed activity context memory leak in LogBoxDialogSurfaceDelegate when using bridgeless mode Reviewed By: javache Differential Revision: D97825193 fbshipit-source-id: d059e40f846be72c296c8617b21b7d7082e78183
1 parent 7b3277f commit f10cded

File tree

3 files changed

+19
-3
lines changed

3 files changed

+19
-3
lines changed

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/LogBoxDialogSurfaceDelegate.kt

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ internal class LogBoxDialogSurfaceDelegate(private val devSupportManager: DevSup
4545
}
4646

4747
override fun show() {
48-
if (isShowing() || !isContentViewReady()) {
48+
if (isShowing()) {
4949
return
5050
}
5151
val context = devSupportManager.currentActivity
@@ -56,6 +56,14 @@ internal class LogBoxDialogSurfaceDelegate(private val devSupportManager: DevSup
5656
)
5757
return
5858
}
59+
60+
if (!isContentViewReady()) {
61+
createContentView("LogBox")
62+
}
63+
if (!isContentViewReady()) {
64+
return
65+
}
66+
5967
dialog = LogBoxDialog(context, reactRootView)
6068
dialog?.let { dialog ->
6169
dialog.setCancelable(false)
@@ -69,6 +77,7 @@ internal class LogBoxDialogSurfaceDelegate(private val devSupportManager: DevSup
6977
}
7078
(reactRootView?.parent as ViewGroup?)?.removeView(reactRootView)
7179
dialog = null
80+
destroyContentView()
7281
}
7382

7483
override fun isShowing(): Boolean = dialog?.isShowing ?: false

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImplDevHelper.kt

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,14 @@ internal class ReactHostImplDevHelper(private val delegate: ReactHostImpl) :
7070
}
7171

7272
override fun destroyRootView(rootView: View) {
73-
// Not implemented, only referenced by BridgeDevSupportManager
73+
val surface = (rootView as? ReactSurfaceView)?.surface ?: return
74+
// stop() synchronously removes the surface from ReactHostImpl.attachedSurfaces via
75+
// detachSurface(), then asynchronously tears down the React component tree.
76+
// detach() severs the surface's back-reference to the host.
77+
// clear() removes child views from the ReactSurfaceView.
78+
surface.stop()
79+
surface.detach()
80+
surface.clear()
7481
}
7582

7683
override fun reload(reason: String) {

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactSurfaceView.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ import kotlin.math.max
3636
* rendering a React component.
3737
*/
3838
@OptIn(FrameworkAPI::class, UnstableReactNativeAPI::class)
39-
public class ReactSurfaceView(context: Context?, private val surface: ReactSurfaceImpl) :
39+
public class ReactSurfaceView(context: Context?, internal val surface: ReactSurfaceImpl) :
4040
ReactRootView(context) {
4141
private val jsTouchDispatcher: JSTouchDispatcher = JSTouchDispatcher(this)
4242
private var jsPointerDispatcher: JSPointerDispatcher? = null

0 commit comments

Comments
 (0)