Skip to content

worker: unregister ScriptExecutionContext eagerly on teardown - #30349

Closed
magJ wants to merge 1 commit into
oven-sh:mainfrom
magJ:workers_segfault_fix
Closed

magJ wants to merge 1 commit into
oven-sh:mainfrom
magJ:workers_segfault_fix

Conversation

@magJ

@magJ magJ commented May 7, 2026

Copy link
Copy Markdown

The GC collect inside teardownJSCVM is best-effort: any JSC::Strong, conservative stack root, or DOM object with hasPendingActivity (e.g. an entangled MessagePort with a listener) keeps the global marked. When ~GlobalObject doesn't run before shutdown() reaches vm.deinit() / arena_.deinit(), the context stays registered in allScriptExecutionContextsMap with a dangling pointer. Any concurrent postTaskTo — such as the main thread still sending postMessage to a worker that threw on its first message — would find the stale entry and write into freed memory, crashing in mi_malloc.

Fix: remove the context from the map explicitly in WebWorker__teardownJSCVM before gcUnprotect, while the globalObject is still alive. This makes postTaskTo() return false deterministically once teardown begins.

Make removeFromContextsMap() idempotent via an m_isInContextsMap flag so ~GlobalObject's later call (whenever GC eventually collects the global) is a no-op rather than an assertion failure.

Fixes: #30348

The GC collect inside teardownJSCVM is best-effort: any JSC::Strong,
conservative stack root, or DOM object with hasPendingActivity (e.g. an
entangled MessagePort with a listener) keeps the global marked. When
~GlobalObject doesn't run before shutdown() reaches vm.deinit() /
arena_.deinit(), the context stays registered in allScriptExecutionContextsMap
with a dangling pointer. Any concurrent postTaskTo — such as the main thread
still sending postMessage to a worker that threw on its first message — would
find the stale entry and write into freed memory, crashing in mi_malloc.

Fix: remove the context from the map explicitly in WebWorker__teardownJSCVM
before gcUnprotect, while the globalObject is still alive. This makes
postTaskTo() return false deterministically once teardown begins.

Make removeFromContextsMap() idempotent via an m_isInContextsMap flag so
~GlobalObject's later call (whenever GC eventually collects the global) is a
no-op rather than an assertion failure.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

@claude claude Bot 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.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented May 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b585e713-3352-4a01-8c3a-73332785e21e

📥 Commits

Reviewing files that changed from the base of the PR and between c5a2f8f and 23d7523.

📒 Files selected for processing (3)
  • src/jsc/bindings/ScriptExecutionContext.cpp
  • src/jsc/bindings/ScriptExecutionContext.h
  • src/jsc/bindings/webcore/Worker.cpp

Walkthrough

This PR prevents a segfault in worker teardown by making ScriptExecutionContext removal from the global contexts map idempotent. A new boolean flag tracks map membership; explicit removal during worker VM teardown prevents races where other tasks observe the context still in the map, and duplicate removals during later garbage collection are safely skipped.

Changes

Worker Teardown Context Map Safety

Layer / File(s) Summary
Context Map Membership Tracking
src/jsc/bindings/ScriptExecutionContext.h
Adds m_isInContextsMap boolean member to track whether the context identifier is currently in the global contexts map.
Map Lifecycle Methods
src/jsc/bindings/ScriptExecutionContext.cpp
addToContextsMap() sets the flag after insertion; removeFromContextsMap() checks the flag and returns early if already removed, preventing duplicate removals and redundant assertions.
Worker VM Teardown Integration
src/jsc/bindings/webcore/Worker.cpp
Explicitly calls removeFromContextsMap() during WebWorker__teardownJSCVM before garbage collection, ensuring timely cleanup before subsequent gcUnprotect and forced collection.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: making ScriptExecutionContext registration unregister eagerly during worker teardown.
Description check ✅ Passed The description provides the issue context, root cause analysis, and solution approach, but lacks explicit verification details per template's 'How did you verify your code works?' section.
Linked Issues check ✅ Passed The PR directly addresses the segfault from #30348 by removing ScriptExecutionContext from the global map during worker teardown to prevent stale pointer access and making removeFromContextsMap() idempotent.
Out of Scope Changes check ✅ Passed All changes are narrowly scoped to fixing the worker teardown segfault: adding idempotency tracking, explicit unregistration during teardown, and related header changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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 and usage tips.

@magJ

magJ commented May 21, 2026

Copy link
Copy Markdown
Author

seems to have been fixed independently with #30875

@magJ magJ closed this May 21, 2026
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.

segfault with throwing workers

1 participant