Rebuild against current mozilla-central SpiderMonkey (157a1), enable SharedArrayBuffer/Atomics - #509
Open
dan-distributive wants to merge 10 commits into
Open
dan-distributive wants to merge 10 commits into
dan-distributive wants to merge 10 commits into
Conversation
…SharedArrayBuffer/Atomics
pythonmonkey previously embedded mozjs-136a1.dll, a ~19-month-old Firefox
Nightly Alpha build that predates Firefox 136's stable release and its
out-of-band security patch (136.0.4) for an actively-exploited sandbox-escape
CVE. This rebuilds against a current mozilla-central snapshot (commit
1704651e7d6c706fcb753adab577e0954d61cee0) instead.
Ten distinct SpiderMonkey embedder-API breaks between 136a1 and current
trunk are fixed, ranging from mechanical (asm.js removed, mozilla::Unused
removed) to a real architecture change: JS::JobQueue's per-job
enqueuePromiseJob push callback was removed entirely in favor of an
engine-internal queue the embedder must explicitly drain via js::RunJobs(),
which required adding checkpoints at three call sites (top-level script
execution, and the two places PromiseType.cc attaches/resolves promise
reactions across the Python/JS boundary) to avoid a real hang this surfaced
during testing.
Also enables SharedArrayBuffer/Atomics (JS::RealmCreationOptions::
setSharedMemoryAndAtomicsEnabled), off by default in this embedding but
required for Pyodide's threaded WASM build to link at all
("LinkError: shared memory is disabled" otherwise) - unrelated to the
version bump itself but bundled here since both were verified together.
See SPIDERMONKEY_VERSION_BUMP.md for the full change-by-change rationale,
explicit risk/review flags on the two changes needing real SpiderMonkey-
internals judgment (the JobQueue redesign and a GC-safety adaptation in
BufferType.cc), and what was and wasn't verified. Verified against basic
eval, SharedArrayBuffer/Atomics, async/await across the Python/JS boundary,
and both real localExec() test suites (dcp_local_job_test.py,
pycomod_localexec_test.py) end-to-end.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
localExec() never leaves the process; exec() submits to the real DCP scheduler and depends on a funded wallet and live workers on the target compute group. Verified separately and documented since it's a materially different code path from everything else already tested. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
Note on authorship: this PR was largely produced by Claude (Anthropic's Claude Code), working under my direction and review, not hand-written by me. The SpiderMonkey engine rebuild, the ten API-break fixes, and the testing described in Flagging this explicitly because none of this has had a second human reviewer yet, and reviewers should know to apply the same scrutiny here they would to any AI-assisted change — especially the two sections the doc itself marks NEEDS REVIEW (the |
The prior commit's Testing section claimed the setTimeout-Promise case and both end-to-end localExec()/exec() job tests passed, but that was checked against a stale site-packages install still linked against the old 136a1 engine, not the actual rebuilt binary. Redeploying the real build and retesting surfaced a genuine hang: JSFunctionProxy_call and JSMethodProxy_call (src/JSFunctionProxy.cc, src/JSMethodProxy.cc) are the generic entry points Python uses to call back into any JS function/method it holds - e.g. a setTimeout callback dispatched from PyEventLoop - and neither drained the job queue after invoking the callback. This is a 4th checkpoint site the previous commit's JobQueue rewrite missed, alongside the three it already found (JobQueue::runJobs, PromiseType::getPyObject, futureOnDoneCallback). Fixed the same way: js::RunJobs(cx) right after the JS_CallFunctionValue call in both files. Also: - setup.sh: restore POETRY_BIN (made idempotent, like the existing rustup check) instead of deleting it outright. The previous commit dropped it reasoning it was unused outside the build, but the .git/hooks/pre-commit dev-tooling branch further down still calls "$POETRY_BIN run pip install autopep8", which would have broken silently for any clone that takes that branch. Fixes the actual problem that motivated the removal instead (no python3 on PATH on this machine, only python). - Remove BUILD_LOG.md, added by the previous commit but describing unrelated sessions (an earlier one-line SharedArrayBuffer fix against the old engine, and separate WebSocket-module work) rather than this rebuild. - SPIDERMONKEY_VERSION_BUMP.md: document the above, correct the Testing section's now-inaccurate "all passed" claims, and note the fix and full retest results (all four end-to-end tests - async unit tests, dcp_local_job_test.py, pycomod_localexec_test.py, dcp_real_exec_test.py - now pass against the actual rebuilt pythonmonkey.pyd). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous two commits' inline comments read like a debugging journal (narrating what was tried, what failed, "confirmed via testing" for nearly every line) rather than code documentation. Rewrote them to state the non-obvious reasoning briefly and let the code show the what - readers can see the diff and the SpiderMonkey headers for themselves. No behavior change; rebuilt and reran the async/Promise regression tests to confirm. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
3 of 5 CI platforms (both macOS runners, Windows) have been failing since this branch's last push, undetected until now -- diagnosed from the actual GitHub Actions logs, not assumed. Root cause: the mozilla-central commit this PR now targets raised its own minimum toolchain requirements (clang/llvm >=19, rustc >=1.90.0) past what setup.sh installs. The Rust side of this was already found once, but only worked around with a local, per-directory `rustup override` on the original dev machine -- never committed to setup.sh itself, so it stayed invisible to CI and any fresh clone. That gap is exactly what surfaced here. - Default Rust toolchain 1.85 -> 1.90.0 (the exact minimum reported by configure). - Added `brew install llvm` + PATH override for macOS, whose Xcode-bundled clang (16-17 on current runner images) was never getting overridden the way Ubuntu's CI already does with its own explicit LLVM install step. - SPIDERMONKEY_VERSION_BUMP.md: corrected section 3 to describe the actual, committed fix instead of the local-only workaround, added 3b documenting the CI-side toolchain gaps (ubuntu's own fix is a separate commit -- it touches the workflow YAML, which needs different push permissions), and flagged in the Testing section that everything there was run locally, not against real CI, until now. Ubuntu (2 of 5 platforms) still needs .github/workflows/test-and-publish.yaml's explicit LLVM step bumped from 18 to 19 -- held as a separate, uncommitted change since pushing it requires `workflow` OAuth scope this session's credentials don't have. Not yet re-verified green against real CI after this fix (pushing to find out). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…scope) Follow-up to 4ef9df7 -- that commit's doc update described the ubuntu LLVM bump as already applied, but it was split into a separate, currently-unpushed change since it touches a workflow YAML file this session's push credentials aren't scoped for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
CI status update: all 5 platforms had been failing since the last push ( Pushed ( Still broken, fix written but not pushed: Ubuntu (x64/arm) needs the same LLVM bump (18 → 19) in Full details in 🤖 Generated with Claude Code |
spidermonkey latest requires llvm 19
Ubuntu's clang bump (18->19, applied via GitHub UI since it needed workflow OAuth scope) got past the clang-version check, but exposed a second issue right behind it: "The libstdc++ in use is not new enough." Checked the actual requirement in SpiderMonkey's own source rather than guessing (build/moz.configure/toolchain.configure, minimum_gcc_version() = 10.1.0): the real minimum is libstdc++ from gcc 10, not some bleeding-edge version. CI's ubuntu jobs deliberately build inside an ubuntu:20.04 container (not the runner OS) for wheel glibc/libstdc++ compatibility -- 20.04's default toolchain is gcc-9, one major version short. Fix: apt-get install libstdc++-10-dev in setup.sh's own Linux dependency list -- already available in 20.04's default repos, no PPA needed. This only adds headers/static libs for clang to compile against; it doesn't change which libstdc++.so.6 the built binary links against at runtime, so it shouldn't narrow the wheel's runtime compatibility the way installing a whole newer toolchain might. libstdc++'s ABI has been stable and symbol-versioned since well before gcc 10 (2020), so this is a low-risk, well-understood fix, not independently verified against an actual old-system runtime. Lives in setup.sh, not the workflow YAML, so no elevated push scope needed for this one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
needed the clang-19/libstdc++-10 toolchain bump
Once build-and-test's compiler was actually clang-19 (previous commit's fix), it got past the toolchain-version checks and reached a real compile error: mfbt/UniquePtrExtensions.h's #error "Unsupported OS?" -- it branches on XP_WIN or XP_UNIX to pick FileHandleType's definition, and this build doesn't go through Mozilla's moz.build system, which normally defines these automatically. This is the exact same class of bug already fixed for Windows (XP_WIN, CMakeLists.txt's WIN32 branch, documented in SPIDERMONKEY_VERSION_BUMP.md section 4) -- just never discovered for Linux/macOS before now, because CI's build-and-test job never compiled far enough to hit it (blocked on the gcc-9/clang-19 issue first). Fix: add -DXP_UNIX to the non-Windows COMPILE_FLAGS in CMakeLists.txt. Checked mfbt for other OS-macro branches first (XP_LINUX, XP_DARWIN) -- those are optional/additive (missing them skips an optimization or a feature branch, not a hard #error), so left alone rather than guessing ahead of an actual failure. Lives in CMakeLists.txt, a regular repo file, so no elevated push scope needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Summary
mozjs-157a1, commit1704651e7d6c706fcb753adab577e0954d61cee0), replacing the ~19-month-oldmozjs-136a1Nightly Alpha build it previously shipped — which predates Firefox 136's stable release and its out-of-band patch for an actively-exploited sandbox-escape CVE.136a1and current trunk, most mechanical (asm.jsremoved,mozilla::Unusedremoved, off-threadDispatchabledispatch API redesigned) but one a real architecture change:JS::JobQueue's per-jobenqueuePromiseJobpush callback was removed entirely in favor of an engine-internal queue the embedder must explicitly drain viajs::RunJobs().SharedArrayBuffer/Atomics(JS::RealmCreationOptions::setSharedMemoryAndAtomicsEnabled), off by default in this embedding but required for Pyodide's threaded WASM build to link at all.Full change-by-change rationale, diffs, and explicit risk/review flags are in
SPIDERMONKEY_VERSION_BUMP.md(added by this PR) — please read it before merging. It calls out two changes needing real SpiderMonkey-internals judgment rather than a mechanical port (a GC-safety adaptation inBufferType.cc, and theJobQueueredesign), and is explicit about what was and wasn't independently verified.Test plan
All of the following were run against the rebuilt engine and passed:
pm.eval()(arithmetic, JSON)SharedArrayBuffer/Atomicsround-trip (the original motivating fix)await, an ordered two-hopawaitchain inside a JS async function, and asetTimeout-based promisedcp_local_job_test.py— fulllocalExec()pipeline, correct outputpycomod_localexec_test.py— heavier stress test: filesystem shipping, extra Pyodide modules (pandas), cloudpickle round-trip of nested numpy arrays, correct numeric outputjob.exec()(notlocalExec()) against the live DCP network on thedemo/dcpcompute group — full real scheduler lifecycle, real worker-produced results, correct outputNot yet independently verified (see
SPIDERMONKEY_VERSION_BUMP.md's closing section for the full list):Atomics.waitAsyncwith a real timeout, Debugger-API-facing JobQueue paths, and theBufferType.ccGC-safety reasoning under a compacting/moving-GC stress configuration. None of this has had a second reviewer yet — recommend focused review on the sections flagged NEEDS REVIEW in the doc before merging.🤖 Generated with Claude Code