Skip to content

[js/rn] Fix iOS SIGSEGV on JS reload by releasing Env in -invalidate - #28367

Open
Alexey Shlaikov (shlaikov) wants to merge 1 commit into
microsoft:mainfrom
shlaikov:fix/rn-jsi-reload-crash
Open

Alexey Shlaikov (shlaikov) wants to merge 1 commit into
microsoft:mainfrom
shlaikov:fix/rn-jsi-reload-crash

Conversation

@shlaikov

Copy link
Copy Markdown

Description

The static env shared_ptr in OnnxruntimeModule holds an onnxruntimejsi::Env whose tensorConstructor_ is a std::shared_ptr<facebook::jsi::WeakObject> pointing into the JSI runtime.

On a JS reload (e.g. Cmd+R during development) the React Native bridge tears down the JSI runtime before the Obj-C OnnxruntimeModule instance is deallocated. When env.reset() runs from -dealloc, ~WeakObject calls runtime.releasePointerValue(...) on a runtime that no longer exists, producing a SIGSEGV in jsi::Pointer::~Pointer().

This change makes OnnxruntimeModule adopt RCTInvalidating and resets env from -invalidate, which React Native calls on the JS thread while the runtime is still alive. -dealloc keeps env.reset() as a no-op fallback for paths that may bypass -invalidate.

Files changed:

  • js/react_native/ios/OnnxruntimeModule.h — declare conformance to RCTInvalidating, import its header.
  • js/react_native/ios/OnnxruntimeModule.mm — implement -invalidate to reset the static env.

Motivation and Context

After the JSI migration (#25764) any iOS app that loads an InferenceSession and then triggers a JS reload (Cmd+R, fast refresh that escalates to a full reload, or programmatic DevSettings.reload()) crashes hard. This is reproducible 100% of the time on onnxruntime-react-native@1.24.x with React Native 0.85.

Repro

  1. In an RN iOS app, create an InferenceSession.
  2. Press Cmd+R in the simulator (or call DevSettings.reload()).
  3. App crashes with EXC_BAD_ACCESS (SIGSEGV) KERN_INVALID_ADDRESS at 0x0.

Crash signature (from a real .ips):

Thread: com.facebook.react.runtime.JavaScript
-[OnnxruntimeModule dealloc]
 -> std::shared_ptr<onnxruntimejsi::Env>::reset
 -> onnxruntimejsi::Env::~Env()
 -> std::shared_ptr<facebook::jsi::WeakObject>::~shared_ptr
 -> facebook::jsi::WeakObject::~WeakObject()
 -> facebook::jsi::Pointer::~Pointer()
 -> SIGSEGV at 0x0

RCTInvalidating is the standard React Native escape hatch for this exact lifecycle problem — it is invoked on the JS thread before the bridge invalidates modules and tears down the runtime, so any JSI handles can be released safely.

I have not touched the Android side. The Android JNI binding stores tensorConstructor differently and I have not seen the equivalent crash there; happy to follow up in a separate PR if maintainers want symmetric handling.

The static `env` shared_ptr in OnnxruntimeModule holds a
jsi::WeakObject (tensorConstructor) that points into the JSI runtime.
On JS reload (e.g. Cmd+R in dev), the bridge tears down the JSI
runtime before the OnnxruntimeModule instance is deallocated. When
`env.reset()` runs from -dealloc, ~WeakObject calls
runtime.releasePointerValue on a dead runtime, producing a SIGSEGV in
jsi::Pointer::~Pointer().

Adopt RCTInvalidating and release `env` from -invalidate, which RN
calls on the JS thread while the runtime is still alive. -dealloc
keeps env.reset() as a no-op fallback for paths that bypass invalidate.

Repro:
  1. Create an InferenceSession in a RN iOS app
  2. Press Cmd+R in the simulator (or otherwise reload the bridge)
  3. App crashes with EXC_BAD_ACCESS

Crash signature:
  Thread: com.facebook.react.runtime.JavaScript
  -[OnnxruntimeModule dealloc]
   -> shared_ptr<onnxruntimejsi::Env>::reset
   -> ~Env() -> ~WeakObject() -> ~Pointer()
   -> SIGSEGV at 0x0
@shlaikov

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@shlaikov

Copy link
Copy Markdown
Author

Could a maintainer re-run the failed Windows CPU job? It died on the setup-python cache step, unrelated to this diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants