Skip to content

fix(jsc): release suspension lock between optional trap retries - #794

Open
steipete wants to merge 1 commit into
oven-sh:mainfrom
steipete:upstream/foreign-stack-trap-suspension
Open

steipete wants to merge 1 commit into
oven-sh:mainfrom
steipete:upstream/foreign-stack-trap-suspension

Conversation

@steipete

Copy link
Copy Markdown

What does this PR do?

Avoid a Linux deadlock when a Worker is terminated while a native call is running on a foreign stack and waiting for another VM. The asynchronous trap sender currently calls blocking Thread::suspend() under the process-wide suspension lock. A stack pointer outside the registered thread stack causes that call to retry indefinitely; another VM cannot collect, so it cannot run the callback that lets the native call return.

Give optional VM-trap sampling a single suspension attempt. A foreign-stack refusal leaves the trap pending and lets the existing work-queue retry release the suspension lock first. Mandatory GC suspension still waits for a valid register snapshot. Failed attempts neither invoke the inspection callback nor change suspension nesting, and platform errors are not reported as thread exit. Mach suspension retains its existing operation.

The native regression enters a VM-owned host function on a guarded custom stack, requests termination, waits for the trap suspension signal, and requires parent full GC to finish before releasing the native call. It also checks protected-root survival, canonical termination delivery, temporary refusal, later delivery, and nested suspension/resume.

Related: #235, #741, #742, and WebKit/WebKit#65562 (bug 315453) address alternate signal-stack register selection. They do not cover a thread whose interrupted stack pointer is genuinely outside its registered stack. General same-VM foreign-stack GC scanning and the sampling profiler's blocking sampling path are outside this fix.

Adapted from my fixes in openclaw/WebKit#25 and #30.

How did you verify your code works?

Fresh Linux x86-64 builds based on main at 5d2ceba2921d29312827883b1fafa5c7cd5cba7e, using LLVM 23.1.2, Release -O2 -g1, static JSC, native LLInt, Bun additions/event loop, FTL, and external mimalloc. Both arms used identical engine configuration; the candidate applied only this PR's six production-file changes.

cmake --build build --target testForeignStackSuspension jsc -j24
build/bin/testForeignStackSuspension

For the unchanged-main control, the same regression was compiled with TEST_FOREIGN_STACK_BLOCKING_CONTROL=1. This omits only the new trySendMessage API checks, which cannot compile against main; the termination/GC test body is identical. A single run under GDB reached its existing 30-second alarm. Its all-thread capture showed the trap sender in Thread::suspend() → sendMessageScoped() → SignalSender::work(), the other VM's full GC waiting in MachineThreads::tryCopyOtherThreadStacks() on the suspension lock, and the native function still on its foreign stack. The debugger confirmed SIGALRM (14) and returned 124 for that expected negative control.

With this patch and the control define removed, the regression exited 0:

PASS temporary refusal, later delivery, nested suspension and resume
PASS parent full GC before native release, rooted object retained, pending termination delivered

The baseline and candidate shared test/build harness additions; no other production fix was present in either arm. This is native Linux x86-64 proof, not a new sanitizer or cross-platform run. WebKit style checks pass for all eight changed files, and git diff --check is clean.

Debugger setup for the unchanged-main control

Run gdb --batch --nx -x baseline.gdb --args build/bin/testForeignStackSuspension, with baseline.gdb containing:

set pagination off
set confirm off
set print thread-events off
set disable-randomization off
handle SIGPWR nostop noprint pass
handle SIGUSR1 nostop noprint pass
handle SIGUSR2 nostop noprint pass
handle SIGALRM stop print nopass
run
thread apply all bt full
python
import gdb
try:
    signal_number = int(gdb.parse_and_eval("$_siginfo.si_signo"))
except gdb.error:
    signal_number = 0
print("FOREIGN_STACK_BASELINE_STOP_SIGNAL=" + str(signal_number))
gdb.execute("quit " + ("124" if signal_number == 14 else "1"))
end

Use a single suspension attempt for asynchronous VM trap sampling while preserving blocking suspension for mandatory GC snapshots. Cover foreign-stack refusal, nested suspension, parent collection progress, and pending termination delivery in a native regression.

@claude claude Bot left a comment

Copy link
Copy Markdown

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 Oct 12, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 11f8136d-2aa2-4ffd-93fc-d3a88bae4d52

📥 Commits

Reviewing files that changed from the base of the PR and between 5d2ceba and 294c676.


📒 Files selected for processing (8)
  • Source/JavaScriptCore/API/tests/testForeignStackSuspension.cpp
  • Source/JavaScriptCore/runtime/VMTraps.cpp
  • Source/JavaScriptCore/shell/CMakeLists.txt
  • Source/WTF/wtf/ThreadMessage.cpp
  • Source/WTF/wtf/ThreadMessage.h
  • Source/WTF/wtf/Threading.cpp
  • Source/WTF/wtf/Threading.h
  • Source/WTF/wtf/posix/ThreadingPOSIX.cpp

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.



Walkthrough

The change adds thread suspension results for temporarily unavailable threads, adds message APIs that report suspension outcomes, and updates VM trap delivery to use them. It also adds a foreign-stack regression test and conditional Linux build configuration.

Changes

Foreign-stack suspension handling

Layer / File(s) Summary
Thread suspension results
Source/WTF/wtf/Threading.h, Source/WTF/wtf/Threading.cpp, Source/WTF/wtf/posix/ThreadingPOSIX.cpp
Thread::trySuspend reports whether suspension succeeded or the thread is temporarily unavailable. POSIX Thread::suspend retries temporary unavailability.
Optional thread messages
Source/WTF/wtf/ThreadMessage.h, Source/WTF/wtf/ThreadMessage.cpp, Source/JavaScriptCore/runtime/VMTraps.cpp
The message API reports suspension failures and temporary unavailability. VM trap delivery uses trySendMessage for the owner-thread callback.
Foreign-stack regression coverage
Source/JavaScriptCore/API/tests/testForeignStackSuspension.cpp, Source/JavaScriptCore/shell/CMakeLists.txt
The test checks message refusal while a worker uses a foreign stack, then checks termination and GC behavior. Linux CMake conditionally configures and builds the test.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 294c6

The change lets asynchronous VM trap delivery avoid holding the process-wide suspension lock while a thread is on a foreign stack. This removes a reported hang between VMs. No concrete merge-blocking risk was identified; it was validated on Linux x86-64 only.

Pre-merge checks | Passed 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description check Passed The description is detailed and on topic. It includes the bug association, explanation, changed behavior, verification steps, results, and limitations. It does not use the exact template headings for …
Title check Passed The title is concise and accurately identifies the main change: releasing the suspension lock between optional trap retries.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use path_filters to narrow the review scope.


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

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