[NathanWalker]

Important

Squash merge this PR. The first commit (1e1f35b) describes the original, larger change. Review cut it down in the later commits, so its message describes code that is not in the final diff. Use this as the squash commit message (title + body), replacing GitHub's default list of commit messages:

fix(android): re-attach NativeScript after a JavaScript reload

After a JS reload, install() returned early on a process-wide flag, so the
new jsi::Runtime had none of NativeScript's globals, and the old
com.tns.Runtime stayed subscribed to GC notifications.

Track the install per NativeScriptNativeApiModule instance and detach in
invalidate(), reusing the worker teardown: detachWorkerRuntime and
DisposeWorkerRuntime are renamed detachRuntime and DisposeRuntime. ~Runtime
resets the main-runtime state so the next attach becomes the main runtime,
and the metadata tree is built once per process.

Split out of #77. Reworked after review to the smallest change that fixes the reload. The leak fixes are now #92, and the GC-delivery race is #91.

Summary

After a JS reload, install() returned early because of a process-wide flag, so the new jsi::Runtime had none of NativeScript's globals. The old com.tns.Runtime also stayed subscribed to GC notifications.

A reload is now handled like disposing a worker:

 NativeScriptNativeApiModule (one per JS runtime)
   install()
-    if g_attached (process-wide): return
+    if attachedRuntime != null (this instance): return
     attachedRuntime = Runtime.attachToHostRuntime(...)
+  invalidate()          // RN: JS thread stopped, runtime still alive
+    runtime.detach()
+      Runtime.detachRuntime(id)      // the worker path: runtimeCache.remove + GcListener.unsubscribe
+      Runtime::DisposeRuntime(...)   // the worker path: DestroyRuntime in an engine scope, delete

• Renames. A reload now uses the worker teardown, so it gets generic names, with bodies unchanged:

  • Java detachWorkerRuntime → detachRuntime;
  • native DisposeWorkerRuntime → DisposeRuntime, in both the jsi and napi trees.

  DisposeRuntime is now also built in the host-runtime flavour.

• Teardown order. In RN 0.86, ReactInstance.destroy() stops and joins the JS thread, then invalidates TurboModules, then destroys the runtime. invalidate() therefore detaches while the runtime is alive and idle.

• Main runtime. The attached runtime is the main runtime, which nothing expected to go away. ~Runtime now resets the main-runtime statics, so the next attach becomes the main runtime. BuildMetadata builds the tree only once, because MetadataNode's static caches point into it.

Standalone apps: the only changes are the renames.

Evidence

Tested on an emulator (Pixel 9, API 36) with the ng-native-with-ns testbed, using synced sources.

• Before: after a reload, global.java / global.android were undefined.
  After: the globals are present after every reload, across 126 reloads in the same process, with one detach per reload.
• Forced GC: no crash with a timer creating Java objects and forcing GC every 100 ms while reloading. GC batches were delivered throughout.
• Delayed Runnable: one that fires in its own runtime runs normally. One posted just before a reload throws NativeScriptException: Cannot find runtime for instance=…, the same error as a callback into a disposed worker.
• The napi and jsi trees both compile for arm64.

Not verified:

• A physical device.
• The legacy-bridge invalidate() path (RN 0.79–0.81 with the old architecture).
• The standalone runtime and worker test suites with the renames; those were compiled only.

Merge Danger

Door: two-way

Blast Radius: Android reload in React Native. Worker teardown is renamed but otherwise unchanged.

Reviewer call: a Java callback still queued from the old runtime throws Cannot find runtime for instance=… when it fires after a reload. On the UI thread that crashes the app, exactly as a callback into a disposed worker does today. This also applies to expo-updates reloadAsync() in production.

Known, unchanged, and shared with workers: the GC thread can deliver a batch while a runtime is being disposed. #91 adds a per-runtime lock for it.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 578ec9c2-fa4e-4677-b0b6-4fe9a76150cf

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[ammarahm-ed]

Tracking the install per module instance instead of with a process-wide flag is exactly the fix for the reload bug, and detaching in invalidate() is the right place to do it.

But the PR does a lot more than the bug needs, and some of it changes behaviour for every NativeScript app, not just React Native.

A reload is the same as disposing a worker

We already create and tear down runtimes all the time for workers, and that path works. The new native Detach is the same code as the existing worker dispose, and the Java detach() repeats most of what the worker detach already does. Please reuse those paths instead of adding parallel ones. The worker dispose isn't really worker-specific, so it could just be renamed.

The one thing that's genuinely new for React Native is that the attached runtime is the "main" runtime, and nothing expected the main runtime to go away. Resetting that state when it's deleted, and only building the metadata once, is needed. That part makes sense.

Changes that affect all apps

• A failed post to the JS thread now throws. This is shared runtime code. In a regular NativeScript app, a Java callback aimed at a terminated worker used to be ignored. Now it throws, and on the UI thread that crashes the app. That shouldn't come in as part of a React Native reload fix.
• GC delivery now holds one lock shared by all runtimes, while calling into the runtime. One runtime's GC can now wait behind another runtime's busy JS, and so can starting or stopping workers. The race it closes is real, but it already exists for workers, so it deserves its own fix, ideally with a lock per runtime.

Not needed for this bug

• The orphaned-objects map. Once the old runtime is removed from the caches, calls into it already fail with an error. The map only improves the message, and it adds process-wide state that every reload fills with every object.
• The leak fixes (method callback data, LRU cache, global ref). They're real leaks and help workers too, so they're worth fixing, but in a separate PR. The method callback data change in particular needs a QuickJS run, since registering extra pointers for deletion risks a double free.

Suggested shape

1. Per-instance install state and detach on invalidate(), as you have it.
2. Detach reuses the existing worker teardown, on both the Java and native sides.
3. Reset the main-runtime state when that runtime is deleted, and only build the metadata once.

That should be a much smaller PR with no behaviour change for standalone apps. The leak fixes and the GC race can follow as their own small PRs.

[NathanWalker]

Thanks, agreed with all of it. I've cut this down to the shape you suggested (900ee4c, 8d6e841):

1. Install state and detach. Install state is tracked per module instance, and invalidate() detaches.
2. Detach is the worker teardown. The Java detachWorkerRuntime became detachRuntime. The native DisposeWorkerRuntime became DisposeRuntime in both the jsi and napi trees. Their bodies are unchanged, and the parallel Detach is gone.
3. Main runtime. ~Runtime resets the main-runtime statics, and the metadata is built once.

I removed these from this PR:

• the throw on a failed post;
• the GcListener lock and the notifyGc change;
• the orphaned-objects map;
• the leak fixes.

Standalone apps only see the renames. A callback that fires into a reloaded runtime now fails the same way a callback into a disposed worker does (Cannot find runtime for instance=…).

The split-out parts are their own PRs:

• fix(android): free runtime-owned JNI refs and method callback data on dispose #92, leak fixes: LRU cache weak refs, the ~Runtime global ref, and MethodCallbackData. It ran on QuickJS (napi 522/522, jsi 450/450) and on Hermes, with a worker-churn stress test. The PR includes the double-free ownership audit.
• fix(android): deliver GC notifications under a per-runtime lock #91, GC delivery: a lock per runtime instead of one shared lock. The stress test crashed the old code and doesn't crash with the fix.

[NathanWalker]

Please squash merge this one. The first commit (1e1f35b) still describes the original, larger change that review cut down. The description now starts with the squash commit message to use.