Skip to content

test(runtime): make the JS block dispose race specs prove the interleaving - #505

Draft
edusperoni wants to merge 2 commits into
mainfrom
test/jsblock-race-test-hardening
Draft

edusperoni wants to merge 2 commits into
mainfrom
test/jsblock-race-test-hardening

Conversation

@edusperoni

Copy link
Copy Markdown
Collaborator

Follows up #500 with its review findings.

Specs that could pass without exercising the race

The specs for a JS block whose dispose waits on the isolate released the block 1 ms after marshalling, inside a 30 ms sleep. When that release slipped past the turn (loaded or ASan simulator), the block disposed normally. handleof then threw and the re-marshal built a fresh block anyway, so the specs passed vacuously.

  • The fixture now keeps blocks until the test releases them (keepBlockUntilReleased:, releaseKeptBlocks).
  • releaseKeptBlocksAwaitingDispose: drops the last reference on a global queue. It then polls libclosure's deallocating flag in the block header until it is set, and returns NO on a timeout.
    • The flag is set by the releasing _Block_release immediately before it calls the dispose helper, so this is a deterministic "dispose has started" signal with no production hook and no sleep.
    • The caller holds the isolate's Locker, so the dispose cannot finish and the header stays readable.
    • strandDispose asserts that it returned true.
  • The worker spec no longer relies on a 300 ms hold. The worker keeps the block, and the parent releases it from its nsworkerended listener. That event is posted after the worker's runtime has been deleted, so the release lands after ~Runtime, once the isolate is gone.

Leftover state

The handleof-while-held spec held its block for 1 s, so the release fired during a later spec. It now releases the block synchronously before it ends and expects one release. The unused keepBlock:forMilliseconds: and sleepMilliseconds: fixtures are removed.

Comment nits

  • InteropTypes.mm, handleof: the block is kept until the current autorelease pool drains, not for the rest of the turn.
  • Interop.mm, JSBlock dispose: states the ownership rule. The JSBlock is the wrapper's only owner (ObjectManager::DisposeValue leaves it alone), whether or not the isolate is alive.
  • Interop.h: TryRetainJSBlock documents only its contract. The Block_copy rationale stays next to its definition in Interop.mm.
  • ObjectManager.mm, DisposeValue Block case: no bug narration. The comment is now just "Block_release, not CFRelease: the block may have been a stack block promoted by Block_copy."

Discrimination check

TryRetainJSBlock was temporarily made to Block_copy unconditionally and return true, which matches #500 being reverted at both the cache hit and handleof. The hammer specs were disabled for this check. The revert was not committed.

Under ASan:

  • "is not handed out by interop.handleof" fails: Expected function to throw an exception. The stranding assertion itself passed, so the dispose had started.
  • "is replaced by a fresh block when the function is marshalled again" crashes: heap-use-after-free in IsolateWrapper::Isolate() from ArgConverter::MethodCallback, invoked from NSBlockOperation. The revived block ran after its dispose freed the callback wrapper.

Suite

  • Plain: 1754 specs, 0 failures, 11 skipped.
  • ASan: 1754 specs, 0 failures, 16 skipped.

Both match main.

The specs for a JS block whose dispose waits on the isolate relied on a
1 ms release landing inside a 30 ms sleep. When the release slipped past
the turn, the block disposed normally and the specs passed without
exercising the race.

The fixture now keeps blocks until the test releases them. The
stranding helper drops the last reference on a background queue and
waits for libclosure's deallocating flag on the block, so the spec
fails instead of passing vacuously when the dispose never started.

The handleof-while-held spec releases its block before it ends, and the
worker spec releases the block only after the worker's nsworkerended
event, once its runtime has been deleted.
State the JSBlock dispose's ownership rule, keep the Block_copy
rationale next to TryRetainJSBlock's definition only, describe the
handleof reference as lasting until the autorelease pool drains, and
drop the bug narration from DisposeValue's Block case.
@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