Skip to content

fix(runtime): pin the isolate gate at the remaining foreign-thread entries - #506

Draft
edusperoni wants to merge 2 commits into
mainfrom
fix/isolate-pin-callbacks
Draft

edusperoni wants to merge 2 commits into
mainfrom
fix/isolate-pin-callbacks

Conversation

@edusperoni

Copy link
Copy Markdown
Collaborator

Follows #501. That PR added the per-isolate lifetime gate (IsolateGates, IsolateWrapper::Pin()) and adopted it in ArgConverter::MethodCallback and the JS block dispose. This PR applies the same pattern to the other entry points that can run on a thread other than the isolate's own and that checked IsValid() before waiting on the Locker.

The problem

~Runtime invalidates the isolate without holding the Locker. It then takes the Locker, removes the isolate's Caches, and releases it. Suppose a foreign thread passes IsValid(), then queues on the Locker behind the teardown. When it finally gets the Locker, Caches::Get() returns an empty stand-in, and GetContext() dereferences a null context. If that thread is still waiting when Isolate::Dispose runs, it locks a freed isolate instead.

Converted sites

Every converted site now follows the same steps: pin, bail if the pin is refused or IsValid() is false, take the Locker, re-check IsValid(), then do the work.

Site Behaviour once the gate is closed or the isolate is invalid
ArrayAdapter count / objectAtIndex: returns 0 / nil
DictionaryAdapter count / objectForKey: / keyEnumerator returns 0 / nil
Map and object key enumerators nextObject / allObjects returns nil
-dealloc of ArrayAdapter, DictionaryAdapter, NSDataAdapter Works like the JS block dispose. While the gate is open, the dealloc detaches its claim, erases Instances and resets the persistent, all under the Locker. This happens even after invalidation, because the teardown walk can still reach the claim through the JS object. Once the gate is closed, or the pin is refused, it frees the claim without taking the Locker.
Extended class +initialize (ClassBuilder IMP) skipped
Extended class retain / release (ClassBuilder IMPs) Only pins at the retain counts that toggle GC protection. The foreign-thread path no longer reads Runtime, which the teardown frees. It posts through NativeScriptPlatform::LookupEventLoop and compares against a runloop captured at __extends time. The protect/unprotect closures take the Locker before reading Instances. If the isolate is gone, the call only forwards to the native retain/release.
Property getter/setter FFI callbacks (ClassBuilder extended classes, MetadataBuilder JS accessors on native classes) getter returns a zeroed value, setter does nothing. PropertyCallbackContext now carries an IsolateWrapper; that is a heap struct, not a block capture.

IsolateWrapper stays trivially copyable, and no ObjC block captures a shared_ptr.

Left alone

These sites only run on the isolate's own thread:

  • Timers.cpp/.hpp: timers fire from the EventLoop's ordered-lane token drain on the isolate's thread. Their state lives in a Caches state slot that is destroyed under ~Runtime's Locker.
  • AnimationFrame.mm: the CADisplayLink is added to the creating JS thread's run loop.
  • Messaging.cpp (TriggerAsync drain, EmitClose, Drain): posted to the port's home loop.
  • EventLoop::RunEntry, Runtime embedder entry points, DrainRejectionsObserver, AsyncGraphOnFetchCompleted, WorkerWrapper/Worker.mm posts, Helpers.mm Lockers: all run on their own loop or thread.

Repro

IsolateTeardownCallbackTests.js and collectionAdapterQueryWorker.js use a new fixture, +[TNSTestNativeCallbacks query:fromThreads:forMilliseconds:]:

  1. A worker hands a JS array or object to native code.
  2. Native code reads it in a tight loop from 4 background threads.
  3. The worker is terminated 20 ms later, 3 rounds per collection kind.

On main under ASan, the run crashed with a SEGV at address 0 in Caches::GetContext() inside -[ArrayAdapter objectAtIndex:] on a background queue. That crash comes from a read that queued on the Locker behind ~Runtime. The repro is committed on its own, before the fix.

Results

Suite main (baseline) this branch
plain 1754 / 0 failures / 11 skipped 1756 / 0 failures / 11 skipped
ASan 1754 / 0 failures / 16 skipped 1756 / 0 failures / 16 skipped

The two extra tests are the new specs.

A worker hands a JS array or object to native code that keeps reading
it from four background threads while the worker is terminated. A read
that passed IsolateWrapper::IsValid() and then queued on the isolate's
Locker behind ~Runtime runs after the teardown removed the isolate's
Caches: Caches::Get() returns an empty stand-in, and GetContext()
dereferences its null context (SEGV in -[ArrayAdapter objectAtIndex:]
under ASan). The same wait can also outlast Isolate::Dispose, which
~Runtime does not defer for a thread blocked on the Locker.
…tries

The collection adapters, the extended classes' synthesized methods and
the JS property accessors installed on native classes all checked
IsolateWrapper::IsValid() and then waited for the isolate's Locker. A
thread that passed the check could get the Locker only after ~Runtime
had removed the isolate's Caches, or after Isolate::Dispose, and then
read a null context or a freed isolate.

These entries now follow ArgConverter::MethodCallback: pin the gate,
bail when the pin is refused or the isolate is invalid, take the
Locker, and check validity again under it.

- ArrayAdapter, DictionaryAdapter and its key enumerators: count,
  objectAtIndex:, objectForKey:, keyEnumerator, nextObject and
  allObjects return 0/nil once the isolate is gone.
- Adapter -dealloc (Array, Dictionary, NSData): like the JS block
  dispose, it detaches its claim under the Locker until the gate
  closes, even after the isolate is invalidated, because the teardown
  can still reach the claim through the JS object. Once the gate is
  closed it frees the claim without the Locker.
- Extended class +initialize: skipped once the isolate is gone.
- Extended class retain/release: pin only on the retain counts that
  toggle GC protection. The foreign-thread path posts through
  NativeScriptPlatform::LookupEventLoop and a runloop captured at
  __extends time instead of reading the Runtime, which the teardown
  frees. The protect/unprotect closures take the Locker before they
  read Instances.
- PropertyCallbackContext carries an IsolateWrapper, and the property
  getter/setter callbacks of ClassBuilder and MetadataBuilder pin like
  MethodCallback: a getter returns a zeroed value and a setter does
  nothing once the isolate is gone.
@coderabbitai

coderabbitai Bot commented Oct 9, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • 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

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

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.

1 participant