Repository navigation
fix(runtime): pin the isolate gate at the remaining foreign-thread entries - #506
Draft
edusperoni wants to merge 2 commits into
Draft
edusperoni wants to merge 2 commits into
edusperoni wants to merge 2 commits into
Conversation
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.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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. Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows #501. That PR added the per-isolate lifetime gate (
IsolateGates,IsolateWrapper::Pin()) and adopted it inArgConverter::MethodCallbackand 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 checkedIsValid()before waiting on the Locker.The problem
~Runtimeinvalidates the isolate without holding the Locker. It then takes the Locker, removes the isolate's Caches, and releases it. Suppose a foreign thread passesIsValid(), then queues on the Locker behind the teardown. When it finally gets the Locker,Caches::Get()returns an empty stand-in, andGetContext()dereferences a null context. If that thread is still waiting whenIsolate::Disposeruns, 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-checkIsValid(), then do the work.ArrayAdaptercount/objectAtIndex:0/nilDictionaryAdaptercount/objectForKey:/keyEnumerator0/nilnextObject/allObjectsnil-deallocofArrayAdapter,DictionaryAdapter,NSDataAdapterInstancesand 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.+initialize(ClassBuilder IMP)retain/release(ClassBuilder IMPs)Runtime, which the teardown frees. It posts throughNativeScriptPlatform::LookupEventLoopand compares against a runloop captured at__extendstime. The protect/unprotect closures take the Locker before readingInstances. If the isolate is gone, the call only forwards to the native retain/release.ClassBuilderextended classes,MetadataBuilderJS accessors on native classes)PropertyCallbackContextnow carries anIsolateWrapper; that is a heap struct, not a block capture.IsolateWrapperstays trivially copyable, and no ObjC block captures ashared_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(TriggerAsyncdrain,EmitClose,Drain): posted to the port's home loop.EventLoop::RunEntry,Runtimeembedder entry points,DrainRejectionsObserver,AsyncGraphOnFetchCompleted,WorkerWrapper/Worker.mmposts,Helpers.mmLockers: all run on their own loop or thread.Repro
IsolateTeardownCallbackTests.jsandcollectionAdapterQueryWorker.jsuse a new fixture,+[TNSTestNativeCallbacks query:fromThreads:forMilliseconds:]: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
The two extra tests are the new specs.