Skip to content

Finish React Native workers before publishing promise results - #33204

Open
Gopalakrishnan Nallasamy (GopalakrishnanN) wants to merge 1 commit into
mainfrom
GopalakrishnanN/fix-rn-worker-completion
Open

Gopalakrishnan Nallasamy (GopalakrishnanN) wants to merge 1 commit into
mainfrom
GopalakrishnanN/fix-rn-worker-completion

Conversation

@GopalakrishnanN

Copy link
Copy Markdown
Contributor

Description

Fixes native AsyncWorker completion ordering around promise publication.

The background worker previously dispatched resolve/reject and only then invoked onFinished() through its raw this pointer. A fast JS completion can release the promise's last owner, leaving that virtual call after destruction. Completion now happens before publication; terminal dispatch locks the weak owner and keeps it alive through resolve/reject/error handling. Execution still uses the existing destructor abort/join contract, so this does not keep abandoned work alive for its whole execution.

Adds a native-device completion check covering 64 resolve/reject publications, positive and negative component fixtures, and updates the Detox aggregate from 14 to 15 checks. The negative fixture rejects a promise whose worker was not finished at settlement. Updates developer-check documentation.

Motivation and Context

Fixes #33185. Two iOS E2E jobs crashed during EPContext data reads with AsyncWorker::toPromise's worker-lambda stack at the virtual completion call. The same native sources also had passing controls, consistent with a lifetime/order race rather than a deterministic model-validation failure.

Validation

In an isolated Windows checkout, using the locked React Native 0.73 headers/dependencies:

  • npm run test:unit -- --runTestsByPath unit-test\ep-context-data-read.test.js — 5 passed, including positive resolve/reject completion and the premature-settlement negative control.
  • Repository-root eslint react_native\e2e\src\EPContextDataReadTest.tsx — passed. The repository intentionally ignores the test JS files; their syntax/runtime validation is supplied by Jest.
  • Repository-root Prettier check on the three modified TS/JS files — passed.
  • clang-format --dry-run --Werror js\react_native\cpp\AsyncWorker.h — passed.
  • MSVC cl /std:c++17 /EHsc /W4 /WX /Zs on a concrete worker including actual AsyncWorker.h, public ORT headers, and locked real React Native JSI/CallInvoker headers — passed.
  • git diff --check — passed.

The local Jest tests mock the native bridge and are not a claim of native iOS execution. The actual 15-check device suite is wired into the existing Android/iOS CI; fresh device results remain required. No crash is retried away and no test is disabled.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@GopalakrishnanN

Copy link
Copy Markdown
Contributor Author

Native JSI review requested: Yulong Wang (@fs-eire), GitHub does not permit a formal review request for your account on this repository (422 collaborator restriction), so routing the formal request to Edward Chen (@edgchen1) and noting the domain review here. The repair is also applied to the two failing heads #33062/#29563; their fresh native-device CI must validate15/15, not merely local mocked component tests.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The concurrency fix appears sound, but native Android and iOS device validation remains necessary for this lifetime-sensitive change.

0 open findings

What changed in this PR

Fixes a React Native AsyncWorker lifetime race by marking work complete before publishing promise results.

Changes:

  • Reorders native completion and safely retains the worker during dispatch.
  • Adds resolve/reject completion checks and updates the Detox total.
  • Documents the completion-ordering invariant.
File Description
js/​react_native/​cpp/​AsyncWorker.h Fixes worker completion and publication ordering.
js/​react_native/​e2e/​src/​EPContextDataReadTest.tsx Adds the native completion check.
js/​react_native/​e2e/​unit-test/​ep-context-data-read.test.js Tests correct and premature settlement.
js/​react_native/​e2e/​test/​OnnxruntimeModuleExample.test.js Updates the aggregate check count.
js/​react_native/​README.md Documents device coverage and ordering.

🧠 Review effort: Balanced


💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@GopalakrishnanN

Copy link
Copy Markdown
Contributor Author

Fresh native Android CI passed: https://github.com/microsoft/onnxruntime/actions/runs/37810913856/job/113436032133 . The Detox log explicitly asserts the real device summary "15/15 checks passed" and marks the EPContext data-read test passed. This includes the 64 native resolve/reject completion publications, not just the five mocked component fixtures (which also passed). iOS compilation passed; the iOS native E2E gate is still running, so #33185 remains open pending that original platform verification.

This branch has not been deployed

No deployments
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.

[Mobile] Intermittent iOS EPContext E2E SIGSEGV in AsyncWorker::toPromise

2 participants