diff --git a/.github/workflows/test-and-publish.yaml b/.github/workflows/test-and-publish.yaml index 9927ebdb..fa998c6a 100644 --- a/.github/workflows/test-and-publish.yaml +++ b/.github/workflows/test-and-publish.yaml @@ -100,10 +100,10 @@ jobs: apt-get install -y lsb-release wget software-properties-common gnupg wget https://apt.llvm.org/llvm.sh chmod +x llvm.sh - ./llvm.sh 18 # install LLVM version 18 - update-alternatives --install /usr/bin/llvm-config llvm-config /usr/bin/llvm-config-18 18 - update-alternatives --install /usr/bin/clang clang /usr/bin/clang-18 18 - update-alternatives --install /usr/bin/clang++ clang++ /usr/bin/clang++-18 18 + ./llvm.sh 19 # SpiderMonkey at the current mozcentral.version pin requires clang/llvm >= 19 (confirmed via its own configure error) + update-alternatives --install /usr/bin/llvm-config llvm-config /usr/bin/llvm-config-19 19 + update-alternatives --install /usr/bin/clang clang /usr/bin/clang-19 19 + update-alternatives --install /usr/bin/clang++ clang++ /usr/bin/clang++-19 19 clang --version clang++ --version - name: Setup Python @@ -238,9 +238,30 @@ jobs: if [[ "$OSTYPE" == "linux-gnu"* ]]; then # Linux sudo apt-get update -y sudo apt-get install -y cmake llvm + # SpiderMonkey's headers now require clang/llvm >= 19 and + # libstdc++ >= 10 to compile against (same requirement as the + # build-spidermonkey job) -- this job's default toolchain + # (ubuntu:20.04's stock gcc-9) doesn't meet it. + sudo apt-get install -y lsb-release wget software-properties-common gnupg + wget https://apt.llvm.org/llvm.sh + chmod +x llvm.sh + sudo ./llvm.sh 19 + sudo update-alternatives --install /usr/bin/llvm-config llvm-config /usr/bin/llvm-config-19 19 + sudo update-alternatives --install /usr/bin/clang clang /usr/bin/clang-19 19 + sudo update-alternatives --install /usr/bin/clang++ clang++ /usr/bin/clang++-19 19 + sudo apt-get install -y libstdc++-10-dev + echo "CC=clang" >> $GITHUB_ENV + echo "CXX=clang++" >> $GITHUB_ENV elif [[ "$OSTYPE" == "darwin"* ]]; then # macOS brew update || true # allow failure brew install cmake pkg-config wget unzip coreutils # `coreutils` installs the `realpath` command + # Xcode's bundled clang is older than SpiderMonkey's own + # minimum (>=19) -- same fix as setup.sh's macOS branch. Pinned + # to llvm@19: the unversioned `llvm` formula (currently 23.x) + # has no bottle for Intel macOS or macOS 14, so it silently + # falls back to a multi-hour from-source build here. + brew install llvm@19 + echo "PATH=$(brew --prefix llvm@19)/bin:$PATH" >> $GITHUB_ENV fi echo "Installing python deps" poetry self add "poetry-dynamic-versioning[plugin]" @@ -255,10 +276,10 @@ jobs: run: | sudo apt-get install -y graphviz # the newest version in Ubuntu 20.04 repository is 1.8.17, but we need Doxygen 1.9 series - wget -c -q https://www.doxygen.nl/files/doxygen-1.9.7.linux.bin.tar.gz - tar xf doxygen-1.9.7.linux.bin.tar.gz - cd doxygen-1.9.7 && sudo make install && cd - - rm -rf doxygen-1.9.7 doxygen-1.9.7.linux.bin.tar.gz + wget -c -q https://www.doxygen.nl/files/doxygen-1.15.0.linux.bin.tar.gz + tar xf doxygen-1.15.0.linux.bin.tar.gz + cd doxygen-1.15.0 && sudo make install && cd - + rm -rf doxygen-1.15.0 doxygen-1.15.0.linux.bin.tar.gz BUILD_DOCS=1 BUILD_TYPE=None poetry install - name: Upload Doxygen-generated docs as CI artifacts if: ${{ matrix.os == 'ubuntu-22.04' && matrix.python_version == '3.11' }} diff --git a/CMakeLists.txt b/CMakeLists.txt index 1577c299..654c008a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -30,7 +30,11 @@ if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME) include(FetchContent) if (WIN32) - SET(COMPILE_FLAGS "/GR- /W0") + # This build doesn't go through Mozilla's moz.build system, which + # normally defines XP_WIN on Windows -- without it, SpiderMonkey headers + # that branch on it (e.g. PlatformMutex.h) fall through to a POSIX path + # that doesn't exist here. + SET(COMPILE_FLAGS "/GR- /W0 /DXP_WIN") SET(OPTIMIZED "/O2") SET(UNOPTIMIZED "/Od") @@ -39,7 +43,13 @@ if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME) SET(PROFILE "/PROFILE") SET(ADDRESS_SANITIZE "/fsanitize=address /Oy-") else() - SET(COMPILE_FLAGS "-fno-rtti -Wno-invalid-offsetof") + # Same reasoning as the WIN32 branch's -DXP_WIN above: this build + # doesn't go through moz.build, which normally defines XP_UNIX on + # Linux/macOS -- without it, headers that branch on it (e.g. + # mfbt/UniquePtrExtensions.h's FileHandleType) hit their "Unsupported + # OS?" #error instead. Only surfaced now because CI's build-and-test + # job never got past an unrelated compiler-version failure before. + SET(COMPILE_FLAGS "-fno-rtti -Wno-invalid-offsetof -DXP_UNIX") SET(OPTIMIZED "-Ofast -DNDEBUG") SET(UNOPTIMIZED "-O0") diff --git a/SPIDERMONKEY_VERSION_BUMP.md b/SPIDERMONKEY_VERSION_BUMP.md new file mode 100644 index 00000000..9998e476 --- /dev/null +++ b/SPIDERMONKEY_VERSION_BUMP.md @@ -0,0 +1,982 @@ +# pythonmonkey: moving off the nightly-alpha SpiderMonkey build — handover notes + +**Status: BUILD SUCCEEDS, CORE FUNCTIONALITY VERIFIED.** This document tracks +every change made while rebuilding pythonmonkey against a current +mozilla-central snapshot instead of the ~19-month-old nightly alpha it +previously shipped (`mozjs-136a1.dll`). Written for handover to the +pythonmonkey/dcp team — read this before trusting or shipping the resulting +build. **See the Testing section near the end for exactly what was and +wasn't verified before you rely on this.** + +New engine: `mozjs-157a1.dll`, built from mozilla-central commit +`1704651e7d6c706fcb753adab577e0954d61cee0` (current trunk as of this work, +2026-09-14) — see the "Which commit to target" section below for why this +specific commit and not a numbered Firefox release. + +--- + +## Why + +A security review of what `pip install dcp` actually puts on a machine found +that pythonmonkey embeds `mozjs-136a1.dll` — not a small standalone library, +but a build of SpiderMonkey (Firefox's JS engine) out of a full +`firefox-source` checkout. The `136a1` is Mozilla's own suffix for **Nightly +Alpha 1**, an unstable, pre-release development snapshot, explicitly "not for +general users." That build predates Firefox 136's eventual stable release — +including an emergency out-of-band patch (136.0.4, shipped 2025-03-27) for a +sandbox-escape vulnerability that was being **actively exploited in the wild** +(CVE-2025-2857), and the broader memory-safety fixes in that release cycle +(MFSA 2025-14). + +Checked `pythonmonkey`'s own upstream `main` branch (`git fetch origin`, +compared `mozcentral.version`): it is **also** still pinned to the exact same +`6bca861985ba51920c1cacc21986af01c51bd690` alpha commit as this local clone, +unchanged since at least their last commit. This is not a stale-local-clone +problem — it's what pythonmonkey ships to everyone today. + +## What target was chosen, and why + +This GitHub mirror (`mozilla-firefox/firefox`, what `setup.sh` actually +downloads from) only tracks mozilla-central **trunk** — it has no per-release +tags, only a rolling `last-mozilla-central` tag. There is no way to pin an +exact "Firefox 136.0.4" commit from this specific source. The practical +equivalent is a trunk commit safely after the point those fixes landed (Mozilla +lands security fixes in trunk before or alongside backporting them to release +branches). + +Initially picked a conservative target (~3.5 weeks after 136.0.4's ship date, +April 2025) to minimize source drift from the currently-working alpha. On +reflection (correctly challenged mid-task): that doesn't actually solve the +underlying problem — an 18-months-old-by-now snapshot would *also* read as +"stale, unpatched" to a future reviewer. Since this mirror only ever offers a +trunk snapshot regardless of which commit is picked, there's no "stable +channel" to fall back to either way — so the right choice is the **freshest** +trunk commit, not a stale one. + +**Final target: `1704651e7d6c706fcb753adab577e0954d61cee0`**, dated +2026-09-06 (~1 week before this work started, deliberately not the literal +tip-of-trunk at the moment of picking, to sidestep any short-lived transient +build breakage mozilla-central occasionally has). + +Old pin preserved at `mozcentral.version.orig-backup-136a1` for rollback. + +--- + +## Changes made, in order + +### 1. `mozcentral.version` + +```diff +- 6bca861985ba51920c1cacc21986af01c51bd690 ++ 1704651e7d6c706fcb753adab577e0954d61cee0 +``` + +### 2. `setup.sh` — several fixes, all confirmed necessary by real build failures (not speculative) + +**a. Rust install step made idempotent.** Was unconditional on every run. +Re-running `rustup-init.sh` when Rust is already installed downloads a fresh +installer exe and executes it, which on this Windows machine gets blocked by +Windows Defender ("Permission denied" — see finding 3 below for the pattern). +Skips the whole block if the 1.85 toolchain is already present, matching the +existing pattern right below it (the Poetry-install skip): + +```diff ++ if command -v rustup >/dev/null && rustup toolchain list 2>/dev/null | grep -q '^1\.85'; then ++ echo "Rust 1.85 toolchain already installed, skipping rustup-init" ++ else + echo "Installing rust compiler" + ... + curl ... | sh -s -- -y ... --default-toolchain 1.85 ++ fi + CARGO_BIN="$HOME/.cargo/bin/cargo" +- $CARGO_BIN install cbindgen ++ command -v cbindgen >/dev/null || $CARGO_BIN install cbindgen +``` + +**b. `wget` replaced with `curl` for the Firefox source download.** +`wget.exe` (confirmed both the MSYS2 copy and — implicitly — any copy) is +blocked outright by a **Windows Defender Application Control (WDAC) policy** +on this machine: `An Application Control policy has blocked this file`, +confirmed directly via a native PowerShell invocation, not a PATH/permissions +issue. This is new since the original 136a1 build session — nothing in that +session's own log mentions any AppLocker/WDAC block. **This machine's security +posture has tightened since the last build**, plausibly related to DWAN-prep +work happening in parallel — worth flagging to whoever manages this machine's +policy. `curl` (both Windows' own and MSYS2's) is unaffected; `unzip` is also +unaffected. Swapped just the `wget` call: + +```diff +- wget -c -q -O firefox-source-${MOZCENTRAL_VERSION}.zip https://... ++ curl -fsSL -o firefox-source-${MOZCENTRAL_VERSION}.zip https://... +``` + +**c. Removed `--disable-explicit-resource-management` configure flag.** +Worked around Bugzilla 1940342 (a header/lib enum mismatch from when the +`using` JS syntax was newly landing in nightly, circa early 2025). On the new +snapshot this is now an *unrecognized* configure option (`InvalidOptionError: +Unknown option`) — the feature has evidently shipped/stabilized since, taking +the flag (and presumably the underlying bug) with it. Removed rather than +guessing a replacement. + +**d. `MOZILLABUILD` `KeyError` fix reapplied.** This is the *same* fix already +made once before, ad-hoc, during the earlier one-line SharedArrayBuffer/Atomics +build session against the old `136a1` engine (not part of this PR) — but that +fix was applied directly to a file *inside* the ephemeral `firefox-source` +checkout, not to this persistent `setup.sh`, so it was lost when +`firefox-source` was deleted and re-fetched for the new commit. Re-applied, +and this time added as a proper `sed` patch in `setup.sh` itself (matching the +existing pattern of the other ~10 patches) so it survives future re-extracts: + +```diff ++ sed -i'' -e 's/os\.environ\["MOZILLABUILD"\]/os.environ.get("MOZILLABUILD", "")/g' ./python/mozbuild/mozbuild/backend/visualstudio.py # LOCAL PATCH: ... +``` + +**e. Poetry install made idempotent, and kept — not dropped.** An earlier +version of this patch skipped installing Poetry altogether, reasoning that it +was only consumed later in this same script's `.git/hooks/pre-commit` +dev-tooling branch and thus "irrelevant to actually building +SpiderMonkey/pythonmonkey." That reasoning was wrong — it broke that branch's +`$POETRY_BIN run pip install autopep8` line for anyone whose clone does take +it, by deleting `POETRY_BIN`'s own definition along with the install step. +Caught during review/retesting, not by a build failure. Fixed the *actual* +problem instead (this machine has no `python3` on `PATH`, only `python`, so +the real installer's `python3 - --version ...` invocation failed outright) +and made the install idempotent, matching the Rust fix in (a): + +```diff ++ if command -v "$POETRY_BIN" >/dev/null || [ -x "$POETRY_BIN" ]; then ++ echo "Poetry already installed, skipping" ++ else + echo "Installing poetry" +- curl -sSL https://install.python-poetry.org | python3 - --version "1.7.1" ++ PYTHON_FOR_POETRY=$(command -v python3 || command -v python) ++ curl -sSL https://install.python-poetry.org | "$PYTHON_FOR_POETRY" - --version "1.7.1" + ... ++ "$POETRY_BIN" self add 'poetry-dynamic-versioning[plugin]' ++ fi +``` + +### 3. Rust toolchain pin: 1.85 → 1.90.0 — was only worked around locally, not actually fixed, until CI caught it + +The new mozilla-central snapshot's own `configure` now hard-requires +`rustc >= 1.90.0` (`ERROR: Rust compiler 1.85.1 is too old`) — pythonmonkey's +own 1.85 pin is unrelated to this; it's Mozilla's minimum that moved. + +**This was originally worked around with a local, per-directory `rustup +override set stable` on the development machine only** — it never touched +`setup.sh`'s own `--default-toolchain 1.85`, so it was invisible to CI and to +anyone doing a fresh clone. That gap is exactly what caused all five CI +platforms to fail once this PR was actually pushed (`Rust compiler 1.85.1 is +too old` on Windows; the equivalent clang-side minimum, also newer than what +CI installs, on the other four — see the CI section below). Caught and fixed +by an autonomous pass that noticed the PR's CI had been red since the last +push and diagnosed it from the actual failure logs, not assumed. + +**Actual fix, in `setup.sh` itself**: bumped the hardcoded default toolchain +from `1.85` to `1.90.0` (the exact minimum `configure` reported), so a fresh +install — CI included — gets a toolchain that actually satisfies this +snapshot's requirement, rather than relying on whatever happens to be +overridden locally on one machine. + +### 3b. CI toolchain versions were also stale — same root cause, different files + +Confirmed via the actual GitHub Actions logs for this PR (all 5 platforms +failed, all for a version of this same reason): + +- **macOS (macos-14, macos-15-intel)**: no explicit LLVM install existed at + all — the build was relying on Xcode's bundled clang (16.0.0 and 17.0.6 on + the current runner images respectively), both below the >=19 requirement. + **Fixed, committed**: added `brew install llvm` plus putting its bin dir + first on `PATH` in `setup.sh`'s own macOS branch (homebrew's llvm keg + isn't symlinked onto PATH by default). +- **Windows**: covered by the rustc 1.90.0 bump above. **Fixed, committed.** +- **ubuntu (x64 and arm)**: `.github/workflows/test-and-publish.yaml`'s + "Setup LLVM" step explicitly installed LLVM 18 (`./llvm.sh 18`), which + needed bumping to 19 to match `configure`'s own `Only clang/llvm 19.0 or + newer is supported` error. **Fixed and applied via the GitHub UI** (this + session's push credentials don't have the `workflow` OAuth scope needed + to push workflow-file changes directly). This got ubuntu past the clang + check, but exposed a second issue right behind it — see 3c below. + +None of this had been verified against real CI before this pass — the +"Testing" section below was checked against local builds and a real network +job, not a green CI run. See that section for the corrected status. + +**Update**: the ubuntu workflow-file fix above was applied by hand via the +GitHub UI (workflow-file pushes need `workflow` OAuth scope this session +didn't have). The clang bump alone got ubuntu past the clang-version check, +but exposed a second, different issue right behind it: + +### 3c. Ubuntu's build container also needs a newer libstdc++, not just a newer clang + +`ERROR: The libstdc++ in use is not new enough.` CI's ubuntu jobs +deliberately build inside an `ubuntu:20.04` container, not the +`ubuntu-22.04` runner OS itself (`.github/workflows/test-and-publish.yaml` +line 75: *"Use the Ubuntu 20.04 container inside Ubuntu 22.04 runner to +build"*) — a real, deliberate choice, presumably so the resulting wheel +links against an older glibc/libstdc++ and runs on more end-user systems. +20.04's *default* toolchain is gcc-9. + +Checked the actual requirement rather than guessing: SpiderMonkey's own +`build/moz.configure/toolchain.configure` (`minimum_gcc_version()`) requires +libstdc++ from **gcc 10.1.0** specifically (`_GLIBCXX_RELEASE >= 10`) — not +some bleeding-edge version. gcc-9's libstdc++ is `_GLIBCXX_RELEASE == 9`, +one short. + +**Fixed, and pushed without needing workflow scope**: added +`apt-get install --yes libstdc++-10-dev` to `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. libstdc++'s ABI has been stable and symbol-versioned since long +before gcc 10 (released 2020), so targeting gcc-10-level symbols shouldn't +meaningfully narrow which end-user systems the wheel still works on -- +reasoned through, not independently verified against an actual old-system +runtime. + +### 4. `CMakeLists.txt` — `XP_WIN` now defined globally for the Windows build + +```diff + if (WIN32) +- SET(COMPILE_FLAGS "/GR- /W0") ++ SET(COMPILE_FLAGS "/GR- /W0 /DXP_WIN") +``` + +pythonmonkey's own `.cc` files (and the SpiderMonkey public headers they pull +in) are compiled directly by this CMake/clang-cl build, not through Mozilla's +own `moz.build` system — which normally defines `XP_WIN` (Mozilla's standard +"building for Windows" macro) for every object file it compiles itself. +Without it, any SpiderMonkey header that branches on `defined(XP_WIN)` +(assuming, reasonably, that Mozilla's own build always defines it on Windows) +silently takes its POSIX/pthread branch instead — confirmed via a real build +failure: `mozilla/PlatformMutex.h` trying to `#include ` (doesn't +exist for an MSVC/clang-cl target), which cascaded into missing-type errors in +`mozilla/UniquePtrExtensions.h`. One instance of this exact class of bug was +already patched, per-file, in the original build (`BaseProfilerUtils.h`, +`defined(XP_WIN)` → `defined(_WIN32)`, still present as a `setup.sh` sed +patch) — since the newer Mozilla snapshot has apparently grown *more* files +with this pattern, fixing it globally here is more robust than continuing to +patch individual headers as they're discovered. + +### 5. `src/BufferType.cc` — SpiderMonkey internal API adaptation — **NEEDS TEAM REVIEW** + +`JS_GetArrayBufferViewFixedData` no longer exists in the new SpiderMonkey — +not renamed, redesigned. This is the **first non-mechanical fix** in this +whole effort: a real SpiderMonkey-internal-C++-API break requiring judgment +about GC/memory safety, not just an environment/build-tooling issue. + +```diff + bool isSharedMemory; + if (!JS_GetArrayBufferViewBuffer(cx, typedArray, &isSharedMemory)) return nullptr; + +- uint8_t __destBuf[0] = {}; +- uint8_t *data = JS_GetArrayBufferViewFixedData(typedArray, __destBuf, 0); +- if (data == nullptr) { // shared memory or still having inline data ++ if (isSharedMemory) { + PyErr_SetString(PyExc_TypeError, "PythonMonkey cannot coerce TypedArrays backed by shared memory."); + return nullptr; + } ++ ++ JS::AutoAssertNoGC nogc(cx); // see below re: why AutoAssertNoGC, not the base AutoRequireNoGC ++ bool isSharedMemory2; ++ uint8_t *data = static_cast(JS_GetArrayBufferViewData(typedArray, &isSharedMemory2, nogc)); ++ if (data == nullptr) { ++ PyErr_SetString(PyExc_TypeError, "PythonMonkey cannot coerce TypedArrays backed by shared memory."); ++ return nullptr; ++ } +``` + +**The reasoning, and exactly what hasn't been verified:** + +The old function's safety contract was: return `nullptr` if the TypedArray's +data is still stored inline (i.e. GC-movable, because it lives inside the +TypedArray object shell rather than a separately-allocated ArrayBuffer). The +new function (`JS_GetArrayBufferViewData`) drops that runtime check entirely +and instead requires a `JS::AutoRequireNoGC` token from the caller. + +**`AutoRequireNoGC` (`js/GCAPI.h`) itself has protected constructor/destructor +— it's a base marker type, not directly instantiable** (confirmed by a real +build error when first tried). Used `JS::AutoAssertNoGC` instead, a public +subclass that — better than a pure marker — actually performs a runtime +assertion in diagnostic builds that no GC occurs while it's alive (a no-op in +release builds, same as the base class would have been). This gives genuine +runtime verification of the safety property in debug/diagnostic builds, not +just a compile-time formality. Even so, this change does **not** mechanically +preserve the *original* function's safety guarantee (which rejected movable +data outright rather than asserting on it) — it relies on reasoning as well +as this assertion: + +The existing `JS_GetArrayBufferViewBuffer()` call, immediately before this, +already exists specifically (per its own original comment, unchanged) to force +any inline/movable TypedArray data to be promoted to a real, stably-allocated +ArrayBuffer first. If that reasoning is correct, the pointer +`JS_GetArrayBufferViewData` returns immediately afterward should already be +backed by stable (non-inline) storage — meaning the specific hazard the old +function's runtime check guarded against should already be closed off before +this new call ever runs, and the `AutoRequireNoGC` token is satisfiable +truthfully rather than just suppressing a compiler complaint. + +**This has NOT been independently verified against SpiderMonkey's actual GC +behavior.** pythonmonkey hands this pointer to Python as a `Py_buffer`, which +Python code can hold onto indefinitely — well past the scope of the local +`nogc` guard. If the reasoning above is wrong in some edge case (e.g. a +TypedArray configuration where `JS_GetArrayBufferViewBuffer`'s promotion +doesn't fully eliminate movability), this would be a real, silent +use-after-free / data-corruption bug, worse than the build simply failing. + +**Recommended before trusting this build for anything beyond +experimentation**: stress-test the TypedArray-to-Python-buffer path +specifically under a compacting/moving GC configuration +(`--enable-gczeal` or equivalent SpiderMonkey debug-build GC-stress mode), +and/or get this specific diff reviewed by someone with real SpiderMonkey GC +internals expertise. Do not ship this change based on this document's +reasoning alone. + +### 6. `include/JobQueue.hh` / `src/JobQueue.cc` — SpiderMonkey API addition, mechanical fix (low risk) + +`JS::JobQueue::getHostDefinedData` (the base class pythonmonkey's `JobQueue` +overrides) gained a second out-parameter, `incumbentGlobal` — previously that +concept was only supplied as an *input* to the separate `enqueuePromiseJob` +method, which pythonmonkey's implementation already ignores entirely (no +incumbent-global tracking at all; jobs are just forwarded to Python's asyncio +event loop). Unlike the `BufferType.cc` fix, **this one is not a judgment +call**: pythonmonkey's existing `getHostDefinedData` already took the "we +don't need this" stance for the original `data` out-param +(`data.set(nullptr); return true;`), so the new `incumbentGlobal` param gets +exactly the same treatment, consistent with the file's own established +pattern rather than inventing new behavior: + +```diff +- bool JobQueue::getHostDefinedData(JSContext *cx, JS::MutableHandle data) const { ++ bool JobQueue::getHostDefinedData(JSContext *cx, JS::MutableHandle incumbentGlobal, JS::MutableHandle data) const { ++ incumbentGlobal.set(nullptr); // We don't need the incumbent global + data.set(nullptr); // We don't need the host defined data + return true; + } +``` + +(Header declaration in `JobQueue.hh` updated to match.) + +--- + +### 7. `include/JobQueue.hh` / `src/JobQueue.cc` / `src/modules/pythonmonkey/pythonmonkey.cc` — SpiderMonkey JobQueue redesign (architecture change — NEEDS REVIEW) + +This is qualitatively different from every fix above it: not a renamed +parameter or a missing macro, but a real redesign of how SpiderMonkey expects +an embedder to receive promise/microtask jobs. Flagged to the team explicitly +before proceeding; the decision (from the project owner) was to keep going and +document it thoroughly rather than stop here. + +**What changed, and how this was confirmed (not guessed):** the build failed +with `error: only virtual member functions can be marked 'override'` on +pythonmonkey's `enqueuePromiseJob` and `empty()` overrides. Reading the new +`JS::JobQueue` base class in full (`js/public/Promise.h`) confirmed both +methods are gone from the interface entirely — not renamed, removed. Two new +pure-virtual methods were added instead: `getHostDefinedGlobal` and (already +present, unrelated) `saveJobQueue`. To understand *why*, and find the +replacement mechanism, traced every call site of `jobQueue->` across all of +`js/src` (7 total, none enqueue-shaped), read Gecko's own real embedding +(`xpcom/base/CycleCollectedJSContext.h/.cpp`) and SpiderMonkey's own reference +embedding (`js/src/vm/JSContext.h`'s `InternalJobQueue`), and finally read +`js/public/friend/MicroTask.h`, which turned out to document the whole new +design inline (see its `[SMDOC]` comment block). + +**The new design, in short:** SpiderMonkey no longer calls out to the embedder +for every job as it's created. Instead it queues jobs itself, internally +(`cx->microTaskQueues`, via `EnqueueJob()` in `js/src/builtin/Promise.cpp`). +The embedder is expected to *pull* jobs from that queue itself, inside its +`JobQueue::runJobs()` override, whenever it wants a "microtask checkpoint" to +happen. That pull is triggered by the embedder calling the free function +`js::RunJobs(cx)` (declared in `jsfriendapi.h`) — note this is **not** the +same thing as the `runJobs()` *method* pythonmonkey overrides, despite the +identical name: `js::RunJobs(cx)` is the public entry point, and its entire +body is `cx->jobQueue->runJobs(cx)` — i.e. it's what *calls* our override. + +Previously, pythonmonkey never needed to call `js::RunJobs(cx)` anywhere, +because `enqueuePromiseJob` forwarded each job to Python's asyncio event loop +the instant SpiderMonkey created it — there was no engine-side queue to drain. +Under the new design, if nothing ever calls `js::RunJobs(cx)`, jobs pile up in +`cx->microTaskQueues` forever and **no promise ever resolves**. So this fix +has two parts: + +**(a) `JobQueue::runJobs()` now does real work** (`src/JobQueue.cc`), instead +of being a no-op. It loops while `JS::HasAnyMicroTasks(cx)`, dequeues each job +via `JS::DequeueNextMicroTask` + `JS::ToMaybeWrappedJSMicroTask`, and forwards +it to the Python event loop — recreating what `enqueuePromiseJob` used to do +per-job, just pull-based now instead of push-based: + +```diff +- void JobQueue::runJobs(JSContext *cx) { +- // Do nothing +- } ++ void JobQueue::runJobs(JSContext *cx) { ++ while (JS::HasAnyMicroTasks(cx)) { ++ JS::RootedValue entry(cx, JS::DequeueNextMicroTask(cx)); ++ if (entry.isNull()) break; ++ JS::Rooted job(cx, JS::ToMaybeWrappedJSMicroTask(entry)); ++ if (!job) continue; ++ auto *rootedJob = new JS::PersistentRooted(cx, job); ++ // ... pack (cx, rootedJob) into a PyCFunction closure, enqueue it on ++ // the running Python event-loop (see runMicroTaskCallback), same as ++ // enqueuePromiseJob's loop.enqueue(callback) did before. ++ } ++ } +``` + +One real difference from the old `enqueuePromiseJob`: `job` there was a +`JS::HandleObject` documented as an ECMA-262 Job (i.e. a plain callable +function with no arguments), which pythonmonkey converted straight to a +Python callable via `pyTypeFactory(cx, jobv)` and handed to Python. The new +`JS::JSMicroTask*` is **not** a generically-callable function — it's an opaque +engine-internal representation that must be executed specifically via +`JS::RunJSMicroTask(cx, job)`, inside `AutoRealm`d to +`JS::GetExecutionGlobalFromJSMicroTask(job)` (this exact usage pattern is +documented in the `[SMDOC]` block at the top of `js/public/friend/MicroTask.h` +— not improvised). So instead of reusing `pyTypeFactory` to wrap `job` +itself, a new small native PyCFunction (`runMicroTaskCallback`) was added, +modelled directly on the existing `dispatchToEventLoop`/`callDispatchFunc` +pattern already in this same file (which smuggles a `(JSContext*, +JS::Dispatchable*)` pair through a Python closure the same way) — packs +`(cx, rootedJob)` as a 2-tuple, and when Python's loop finally calls it, calls +`JS::RunJSMicroTask` and reports failure via the existing +`setSpiderMonkeyException(cx)` helper (same one used throughout +`pythonmonkey.cc`). + +`JS::JSMicroTask` is a type alias for plain `JSObject` (confirmed directly in +`MicroTask.h`: `using JSMicroTask = JSObject;`), so it can be kept alive +across the gap between "dequeued here" and "Python's event loop calls back, +possibly much later" the same way pythonmonkey already keeps +FinalizationRegistry callbacks alive elsewhere in this file: a heap-allocated +`JS::PersistentRooted`, freed once the callback actually runs. + +**(b) `pythonmonkey.cc` now calls `js::RunJobs(GLOBAL_CX)`** once, immediately +after every top-level `JS_ExecuteScript()` call — the natural equivalent of +the HTML spec's "clean up after running script" microtask checkpoint, and (as +far as could be found) the only place in pythonmonkey's own source that a +checkpoint like this was ever implicitly happening before (via the old +immediate-forwarding design). + +**`getHostDefinedGlobal`** (the other new pure-virtual method) was given the +same "we don't track this" stance pythonmonkey already takes for +`getHostDefinedData`'s params — `out.set(nullptr); return true;` — which +matches SpiderMonkey's own reference embedding +(`InternalJobQueue::getHostDefinedGlobal` in `js/src/vm/JSContext.cpp`) +exactly, so this part is low-risk / pattern-consistent rather than a guess. + +**NEEDS REVIEW — this is the least-verified change in this entire document, +more so than the `BufferType.cc` GC fix:** +- Whether draining exactly once per `JS_ExecuteScript()` call is the *right* + cadence for this embedding (vs., say, needing a checkpoint after every + re-entry into JS, or after every Python-side `await` of a JS promise) has + not been verified against real async/await interop test cases — only + against "does it compile and does the basic shape make sense." +- GC-safety of holding a `JS::PersistentRooted` across an + arbitrary, unbounded real-world delay (Python's event loop may not run the + callback for a while) is modelled on the pre-existing, working + `finalizationRegistryCallbacks` pattern in this same file, but has not been + independently confirmed for `JSMicroTask` objects specifically. +- The ordering/interleaving semantics (does a JS promise chain still resolve + in the same relative order it used to, now that jobs are batch-pulled per + checkpoint instead of pushed one at a time?) has not been tested. +- **Before trusting this for anything beyond experimentation**: write and run + a test that chains multiple `await`s across the Python/JS boundary + (`pm.eval` returning a Promise that resolves another Promise, etc.) and + confirms both completion and ordering, not just successful compilation. + +**UPDATE — this was tested, and the concern above was real.** Once the build +first succeeded (fix #10 below), a smoke test awaiting even a single, +already-resolved JS Promise from Python hung indefinitely. Root cause: the +one `js::RunJobs(GLOBAL_CX)` call added above (after `JS_ExecuteScript`) +only checkpoints the *first* batch of jobs created during top-level script +execution. It does not cover the other two places jobs get freshly enqueued +into `cx->microTaskQueues`, both entirely outside of any `JS_ExecuteScript` +call: + +1. **`PromiseType::getPyObject`** (`src/PromiseType.cc`) — called when Python + code `await`s a JS Promise. `JS::AddPromiseReactions` attaches a reaction + callback; if the promise is already settled (the common case for a + same-tick resolution), this immediately enqueues a job that nothing was + draining. +2. **`futureOnDoneCallback`** (`src/PromiseType.cc`) — called from a Python + `asyncio.Future`'s done-callback (i.e. from Python's event loop, not from + JS at all) to resolve/reject a JS Promise that JS was awaiting on a Python + awaitable. `JS::ResolvePromise`/`JS::RejectPromise` here can trigger that + promise's own already-attached reactions, again with nothing draining them. +3. **`runMicroTaskCallback`** (`src/JobQueue.cc`, part of this same fix #7) — + running one microtask (e.g. one `await` in a chain) can enqueue the next + one; the callback returned without re-checkpointing, so a promise chain + with more than one `await` stalled after the first hop even once (1) and + (2) were fixed. + +Fixed by adding `js::RunJobs(cx)` at all three points — mechanical once the +pattern was identified (same call used above), but finding *where* it was +missing required actually running async code, not just getting a clean +compile. **This is the concrete confirmation that "compiles" and "works" are +different claims for this whole JobQueue rewrite** — treat any other +not-yet-exercised code path in this rewrite (the debug queue, +`saveJobQueue`/`SavedJobQueue` used by the Debugger API, `isDrainingStopped`) +with the same suspicion until it's actually been run. + +Retested after this fix: a single `await` of an already-resolved Promise, a +two-hop `await` chain inside an async function (verifying both completion +*and* ordering), and a `setTimeout`-based Promise (exercising the unrelated, +pre-existing `PyEventLoop::enqueueWithDelay` timer path) — see the Testing +section near the end of this document for exact results. + +### 8. `src/JobQueue.cc` — `mozilla::Unused` / `mozilla/Unused.h` removed upstream, mechanical fix (low risk) + +Next build error after fix #7: `fatal error: 'mozilla/Unused.h' file not found`. +Confirmed this isn't a path/environment issue — the header (and the +`mozilla::Unused` helper it declared) is genuinely gone from the current +mozilla-central snapshot's `mfbt/` directory, not just moved (checked: absent +from `mfbt/`, and grepping `dom/`, `xpcom/base/`, `js/src/vm/` for +`mozilla::Unused` turns up zero uses anywhere in current upstream code, +confirming it's been fully purged, not merely renamed). The old header +(preserved at +`_spidermonkey_install.orig-136a1-backup/include/mozjs-136a1/mozilla/Unused.h` +from the previous build) shows `Unused << expr` was only ever a thin +"suppress unused-nodiscard-return-value warning" helper +(`template void operator<<(const T&) const {}`) — functionally identical +to a plain `(void)expr;` cast. Two use sites in `JobQueue.cc` (the only file +in this codebase that used it) were switched to that, and the now-dead +`#include ` removed: + +```diff +- mozilla::Unused << finalizationRegistryCallbacks->append(callback); ++ (void)finalizationRegistryCallbacks->append(callback); +... +- mozilla::Unused << JS_CallFunction(cx, NULL, func, JS::HandleValueArray::empty(), &unused_rval); ++ (void)JS_CallFunction(cx, NULL, func, JS::HandleValueArray::empty(), &unused_rval); +``` + +### 9. `include/JobQueue.hh` / `src/JobQueue.cc` — off-thread dispatch API redesign (moderate risk) + +Next build errors after fix #8, all in the same area: `JS::InitDispatchToEventLoop` +no longer exists ("did you mean 'dispatchToEventLoop'?"); a 3-argument call +where only 2 are now expected; and `'run' is a protected member of +'JS::Dispatchable'`. Read the current `js/public/Promise.h` (lines 622-817) in +full to understand the new shape rather than guessing from the error text +alone. + +**What changed:** +- `JS::InitDispatchToEventLoop(cx, callback, closure)` → replaced by + `JS::InitAsyncTaskCallbacks(cx, dispatchCallback, delayedDispatchCallback, + asyncTaskStartedCallback, asyncTaskFinishedCallback, closure)`. The first + two callbacks are now both mandatory (previously only one existed at all); + the last two are optional (`nullptr` accepted). +- `DispatchToEventLoopCallback`'s signature changed from taking a raw + `JS::Dispatchable*` to taking ownership via `js::UniquePtr&&`. +- `Dispatchable::run()` is now `protected`. The new public entry point is the + static `Dispatchable::Run(JSContext*, js::UniquePtr&&, + MaybeShuttingDown)`, which takes ownership and is responsible for both + calling `run()` and cleaning up. +- A brand new, previously-nonexistent-for-this-embedding + `DelayedDispatchToEventLoopCallback` is now mandatory too. + +**Fix, in `src/JobQueue.cc`:** + +```diff +- JS::InitDispatchToEventLoop(cx, dispatchToEventLoop, cx); ++ JS::InitAsyncTaskCallbacks(cx, dispatchToEventLoop, delayedDispatchToEventLoop, nullptr, nullptr, cx); +``` + +`dispatchToEventLoop` itself: the raw `Dispatchable*` this used to smuggle +through a Python closure (packed as a `PyLong` pointer, same trick used +elsewhere in this file for the microtask fix in #7) is now obtained via +`dispatchable.release()` before packing, and reconstructed with +`js::UniquePtr(dispatchable)` on the other side, then run +via `JS::Dispatchable::Run(cx, ..., JS::Dispatchable::NotShuttingDown)` +instead of the old direct `dispatchable->run(cx, ...)` call — mechanical +translation of the ownership-transfer model, not a judgment call. + +**`delayedDispatchToEventLoop` — NEEDS REVIEW, the one genuine judgment call +in this fix:** this embedding has no existing mechanism for scheduling a +callback *safely from an arbitrary SpiderMonkey helper thread* with a delay. +`PyEventLoop::enqueueWithDelay` exists and is used elsewhere (JS +`setTimeout`), but it calls `asyncio.loop.call_later`, which — unlike +`call_soon_threadsafe` (used by `PyEventLoop::enqueue`, and safe from any +thread) — is not documented as callable from a thread other than the one +running the loop. Since `DelayedDispatchToEventLoopCallback` is explicitly +documented as needing to be safe from any thread, reusing `enqueueWithDelay` +directly would be a plausible new thread-safety bug, not a fix. + +Instead, this implementation always returns `false`, which +`js/public/Promise.h` explicitly sanctions: *"If a timeout manager is not +available for given context, it should return false."* + +**Correction made during this fix, left visible because the first instinct +was wrong and it's a useful lesson for reviewers:** the first attempt had +this call `dispatchable.release()` then `task->transferToRuntime()` directly, +based on a doc comment on `Dispatchable::transferToRuntime()` showing that +exact usage pattern. That failed to compile — `transferToRuntime()` is +`protected`, so an embedder callback has no access to it (the doc comment +describes SpiderMonkey's *own* internal usage, not the embedder-facing API). +The actually-correct, embedder-facing call was found by reading real +production code instead of inferring from a header comment: Gecko's own +`dom/workers/RuntimeService.cpp` (`JSDispatchableRunnable::PostDispatch`) +handles exactly this "took ownership, failed/declined to dispatch" case with +the public static `JS::Dispatchable::ReleaseFailedTask(std::move(task))`, +which is what this fix now uses. + +**Risk assessment**: this should only affect internal SpiderMonkey features +that specifically need an off-thread *delayed* dispatch (the header mentions +`Atomics.waitAsync` timeouts as an example). Ordinary JS `setTimeout` / +`setInterval` go through a separate, unaffected, already-working path +(pythonmonkey's own JS-exposed timer functions calling +`PyEventLoop::enqueueWithDelay` directly, on the main thread). **Not verified +against a real `Atomics.waitAsync`-with-timeout test case** — if this +embedding's use cases ever depend on that specific feature, this will need +a real timeout-manager implementation instead of the `false` stub. + +### 10. `src/modules/pythonmonkey/pythonmonkey.cc` — asm.js support removed, mechanical fix (low risk) + +Next build error after fix #9 (and the first one outside `JobQueue.cc`/`.hh`): +`error: no member named 'setAsmJS' in 'JS::ContextOptions'`. Confirmed via +`js/public/ContextOptions.h` that no asm.js-related member exists on +`ContextOptions` anymore at all — not renamed, removed. asm.js was a +pre-WebAssembly, Firefox-specific JS-subset compilation target; WebAssembly +(enabled separately via the still-present `.setWasm(true)`, unaffected by +this) has long since superseded it upstream. Simply deleted the +`.setAsmJS(true)` call in the `ContextOptionsRef` chaining call during +context setup — nothing to replace it with, since the feature itself is gone, +not relocated. + +### 11. `src/JSFunctionProxy.cc` / `src/JSMethodProxy.cc` — a 4th missing JobQueue checkpoint, found by independent retesting (moderate risk, now fixed) + +**This section exists because the "All passed, repeatably" claim under point 3 +of the Testing section below was wrong when first written.** Independent +retesting (by Claude, at the requester's request, specifically to audit this +PR before review) redeployed the actual built `pythonmonkey.pyd` + +`mozjs-157a1.dll` — the previously-installed copy in `site-packages` was +stale, still linked against the old `136a1` engine, so earlier manual smoke +tests after the JobQueue rewrite had not actually been exercising this build +— and found that awaiting a JS Promise resolved via `setTimeout` hangs +indefinitely, and so does the real `dcp_local_job_test.py` end-to-end job +(it gets through bootstrap and identity loading, then never fires a single +`readystatechange` event). + +**Root cause**: fix #7's checkpoint list (`JobQueue::runJobs`, +`PromiseType::getPyObject`, `futureOnDoneCallback` — three places new jobs +get enqueued into `cx->microTaskQueues` outside of a top-level +`JS_ExecuteScript()` call) missed a fourth: `JSFunctionProxy_call` +(`src/JSFunctionProxy.cc`) and `JSMethodProxy_call` (`src/JSMethodProxy.cc`) +are the generic entry points Python uses to call back into *any* JS function +or bound method it was handed — this is what fires a `setTimeout` callback +dispatched from `PyEventLoop`, or a JS event listener invoked directly from +Python code. Both call `JS_CallFunctionValue` and return without ever +draining the job queue afterward. If the JS function just called +resolved/rejected a Promise with already-attached reactions (the common case: +`resolve(...)` inside a `setTimeout` callback), that enqueues a job nothing +was scheduled to drain. + +**Fix**, identical in both files — add the same checkpoint used everywhere +else in this rewrite, immediately after the call succeeds: + +```diff + if (!JS_CallFunctionValue(cx, thisObj, jsFunc, jsArgs, &jsReturnVal)) { + setSpiderMonkeyException(cx); + return NULL; + } + ++ js::RunJobs(cx); ++ + if (PyErr_Occurred()) { + return NULL; + } +``` + +(`#include ` added to both files for the declaration, matching +`JobQueue.cc`'s existing include.) + +**Retested after this fix** — all of Testing point 3 below plus an added +sequential-delayed-promises case, and all of points 4 and 5 (the full +`localExec()` suite and the real `exec()` test) were rerun end-to-end against +this exact rebuilt binary. All passed; see the corrected Testing section +below. This is the second time in this same JobQueue rewrite that "compiles +and a few manual checks look right" turned out not to mean "actually works" +— treat that as a standing warning for any *other* not-yet-exercised path in +this rewrite (the Debugger-API paths flagged in "Not yet done" below), not +just the two paths that have now each independently failed once. + +### 12. `src/modules/pythonmonkey/pythonmonkey.cc` — no `ModuleLoadHook` registered, `import(...)` hung forever (found running the *actual* full test suite for the first time, not a hand-picked subset) + +**How this was found**: the user reported CI hangs on `(ubuntu-22.04, 3.8)` +and `(ubuntu-22.04, 3.10)` with no further output after `test_dicts.py`. +Reproducing locally with `pytest tests/python/test_event_loop.py::test_promises` +alone did not hang (fast pass), but running the *actual full suite* +(`pytest tests/python`, matching CI's own invocation) reproduced it -- and, on +this machine, sometimes as a hang and sometimes as a genuine Windows access +violation later in the same run (see fix #13; two independent bugs were +stacked behind each other, and the first one being a hang meant the second +one was never reached before). Bisected `test_promises` line-by-line with a +`faulthandler.dump_traceback_later` watchdog down to one exact statement: + +```python +with pytest.raises(pm.SpiderMonkeyError, + match="\nError: Dynamic module import is disabled or not supported in this context"): + await pm.eval("import('some_module')") +``` + +**Root cause**: pythonmonkey never calls `JS::SetModuleLoadHook`. Reading +`js::HostLoadImportedModule` (`js/src/vm/Modules.cpp`) directly: when +`cx->runtime()->moduleLoadHook` is null, it calls `JS_ReportErrorASCII(cx, +"Module load hook not set")` and returns `false` immediately -- it does +**not** fall through to `FinishLoadingImportedModuleFailedWithPendingException` +the way it does when a hook *is* registered but itself returns `false`. Its +caller, `TryStartDynamicModuleImport`, discards that return value +(`(void)HostLoadImportedModule(...)`) and unconditionally returns `true`, so +`StartDynamicModuleImport`'s own fallback (`RejectPromiseWithPendingError`) is +never reached either. Net effect: the promise returned by `import(...)` is +created and returned to script, but nothing ever resolves or rejects it -- +confirmed empirically (`import('some_module').then(onResolve, onReject)` +called neither callback, ever) -- and the pending "Module load hook not set" +exception is left dangling on `cx`, uncleared. `await`ing that promise from +Python hangs forever, since `PromiseType::getPyObject`'s own `js::RunJobs(cx)` +checkpoint has nothing to drain: no reaction job is ever enqueued for a +promise that never settles. + +This is not a regression introduced by anything else in this document -- this +codepath has presumably always been broken the same way, just never +exercised end-to-end before. It surfaced now because this was the first time +the full suite was actually run to completion against a real rebuilt binary +in one continuous session, one test after another with no gaps. + +**Fix**: register a `JS::ModuleLoadHook` at context-init time +(`PyInit_pythonmonkey`, right after `JOB_QUEUE->init`) that immediately fails +every load with `JS_ReportErrorASCII` and returns `false` -- making +pythonmonkey the thing responsible for finishing the promise, exactly as the +`JS::ModuleLoadHook` doc comment (`js/public/Modules.h`) requires of any +embedder that permits dynamic-import syntax to be parsed at all: + +```diff ++ JS::SetModuleLoadHook(JS_GetRuntime(GLOBAL_CX), pythonmonkeyModuleLoadHook); +``` + +Since a hook is now registered, `HostLoadImportedModule` takes the +already-correct `if (!ok) { ... FinishLoadingImportedModuleFailedWithPendingException(...) }` +path instead of the no-hook early-return, and the promise rejects properly +with an `Error: Dynamic module import is disabled or not supported in this +context` -- which happens to be exactly the message the pre-existing test +already expected, strongly suggesting this is what the test always assumed +would happen and never got to verify. + +**Retested**: the exact `pytest.raises(...)` block above now passes; the +full `test_promises` test passes; the isolated minimal repro +(`await pm.eval("import('some_module')")`) resolves (rejects) immediately +instead of hanging, across 3 repeated runs. + +### 13. `src/JobQueue.cc` -- a 5th missing JobQueue checkpoint, in the off-thread dispatch path (`callDispatchFunc`) + +**This is the "fifth Python-to-JS callback path" scenario fix #11 flagged as a +reason to stop patching call sites one-by-one.** Found immediately after +fixing #12 above, once the full suite could progress far enough to reach it: +`test_webassembly` (off-thread `WebAssembly.instantiate`) hung in isolation, +and crashed with a genuine Windows access violation when run as part of the +full suite (same bug, two different symptoms depending on unrelated heap +state at the time -- this is almost certainly also the explanation for why +the CI hang and this session's local access-violation crash looked different +from each other despite sharing a root cause upstream of this fix). + +**Root cause**: `JobQueue::dispatchToEventLoop` (the `JS::InitAsyncTaskCallbacks` +callback added in fix #9, invoked by SpiderMonkey from a helper thread when +off-thread work like WebAssembly compilation finishes) hands the JS +`Dispatchable` off to `callDispatchFunc`, which runs it via +`JS::Dispatchable::Run(cx, ...)`. Confirmed by instrumenting every step with +`fprintf(stderr, ...)` and rebuilding: the dispatch machinery itself works +correctly end-to-end (helper thread -> spawned thread -> main loop -> +`callDispatchFunc` all ran and returned normally, twice, matching +WebAssembly's compile-then-instantiate two-phase off-thread completion) -- but +`callDispatchFunc` never called `js::RunJobs(cx)` afterward. Running the +dispatchable resumes JS execution that settles the WebAssembly promise and +enqueues its reaction job, and -- same story as fixes #7 and #11 -- nothing +was draining it. + +**Fix**, same pattern as every other checkpoint in this rewrite: + +```diff + JS::Dispatchable::Run(cx, js::UniquePtr(dispatchable), JS::Dispatchable::NotShuttingDown); ++ ++ js::RunJobs(cx); ++ + Py_RETURN_NONE; +``` + +**Retested**: the isolated `WebAssembly.instantiate(...).then(...)` repro +resolves correctly across 3 repeated runs (previously hung every time); the +full `test_webassembly` test passes. + +**This makes five independent missing-checkpoint sites found across fixes #7, +#11, and #13 (`JobQueue::runJobs`'s own callback, `PromiseType::getPyObject`, +`futureOnDoneCallback`, `JSFunctionProxy_call`/`JSMethodProxy_call`, and now +`callDispatchFunc`).** Per fix #11's own stated threshold, this is the signal +to stop finding these one at a time: **someone should systematically audit +every place in this codebase that resumes JS execution from outside a +top-level `JS_ExecuteScript()` call** (grep for `JS_Call*`, `JS_Invoke`, +`Dispatchable::Run`, and any other JS-entry point) and confirm each one +either already has a `js::RunJobs(cx)` checkpoint or is proven not to need +one, rather than continuing to wait for the next one to surface as a hang or +a crash in someone's test run. + +**Full suite re-run after both fix #12 and #13**: `pytest tests/python` (all +files, not a subset) -- **676 passed in 59.8s, zero hangs, zero crashes** -- +confirmed on this machine (Windows, Python 3.14.7) in one continuous run +immediately after applying both fixes and rebuilding. + +--- + +## Testing — what was actually run, and what it showed + +**Important caveat added later**: everything in this section was run +against a local build on the original development machine, using the local +Rust `stable` override described in section 3 above — **not against real +CI**. When this PR's actual CI ran, all 5 platforms failed on toolchain +version mismatches invisible to that local setup (section 3/3b). Those were +fixed, followed by two more real runtime bugs found only by running the full +test suite for real (sections 12-13), and finally two CI-infrastructure-only +issues unrelated to any code in this PR (the docs step's `doxygen.nl` link +having gone 404, and `brew install llvm` silently falling back to a +multi-hour from-source build on Intel/macOS-14 runners because the +unversioned formula has no bottle there — fixed by pinning `llvm@19`). + +**CI is now fully green**: every `build-spidermonkey-*` job and all 34 +`build-and-test` OS/Python combinations pass +(https://github.com/Distributive-Network/PythonMonkey/actions/runs/35745019095), +confirmed 2026-09-22. The PR is mergeable and awaiting review. + +Ten build errors were fixed in total (sections 1-10 above), each one a real +SpiderMonkey-internal API break between the old `136a1` nightly-alpha build +and current mozilla-central — none were environment/tooling issues by this +point (those were resolved earlier, before section 1). The build finally +succeeded (`pythonmonkey.pyd` + `mozjs-157a1.dll`, `BUILD_EXIT_CODE=0`). + +After that, four rounds of runtime verification were run (not just "it +compiles"): + +1. **Basic eval**: `pm.eval('1 + 2')`, `pm.eval('JSON.stringify(...)')` — + passed. +2. **`SharedArrayBuffer`/`Atomics`** — the original, one-line motivating fix + for this entire rebuild (a completely separate, older issue from + everything in this document). Verified still working: + `Atomics.store`/`Atomics.load` round-trip through a `SharedArrayBuffer` + returned the correct value. +3. **Async/Promise interop across the Python/JS boundary** — this is where + real bugs actually turned up (see the "UPDATE" note under fix #7 above for + the full story: a single `await` of a JS Promise hung indefinitely on the + first attempt, root-caused to `js::RunJobs(cx)` only being called from one + of the three places new jobs actually get enqueued). After fixing all + three call sites, verified: a single `await` of an already-resolved JS + Promise; a two-`await` chain inside a JS async function, checking both + completion *and* correct ordering (`[1, 3, 5]`, not e.g. `[1, 5, 3]`); and + a `setTimeout`-based Promise. **This third case was reported as passing + here, but that was wrong** — see fix #11 above: the actual built binary + deployed to `site-packages` was stale at the time (still the old `136a1` + engine), so this hadn't really been exercised against this rewrite. Once + retested against the real binary, the `setTimeout` case hung, was + root-caused to a 4th missing checkpoint (fix #11), and after that fix, all + of the above — plus an added sequential-back-to-back-delayed-promises + case — passed, repeatably, confirmed against the actual rebuilt + `pythonmonkey.pyd`. +4. **The real `localExec()` test suite**, run end-to-end against the new + engine, exactly as originally planned. **Like point 3, this was also + re-verified after fix #11** — `dcp_local_job_test.py` specifically hangs + after identity loading without that fix (identity loading itself is an + `async` JS function call, which goes through `PromiseType::getPyObject` + and was already covered; the job's own event/timer-driven machinery is + what hit the missing 4th checkpoint): + - `dcp_local_job_test.py` — a real job (`dcp.compute_for` over 8 letters, + uppercasing work function), through the full `localExec()` pipeline + (readystate transitions, identity loading from a real `id.keystore`, + job deployment, slice completion, result collection). **Passed** — + correct output `YELLING!`. + - `pycomod_localexec_test.py` — a much heavier stress test: real + filesystem shipping (`job.fs.add`) of a local Python package into the + sandbox, extra declared Pyodide modules (`pandas` on top of the usual + `numpy`/`cloudpickle`), extra work-function arguments flowing through + `job.jobArguments`, and — importantly — non-primitive slice results + (nested dicts of numpy arrays), which exercises cloudpickle's real + serialization round-trip rather than the primitive fast path. **Passed** + — all 5 slices completed with correct structure and correct numeric + values (spot-checked a sample series: `values[:5] = [25. 25. 25. 25. + 25.]`, `dtype=float32`, as expected for this model). Takes a few minutes + (real Pyodide package loading: `pandas`/`numpy`/`cloudpickle`/etc.) — + don't mistake the lack of output during that window for a hang. + +5. **Real `job.exec()` (not `localExec()`) — genuine network dispatch, + verified separately** (`dcp_real_exec_test.py`, modelled directly on + `dcp_sample_job.py`'s pattern but loading identity from `id.keystore` + the same safe way `dcp_local_job_test.py` does, rather than an inline + private key). This is a materially different code path from everything + above: `localExec()` never leaves the process, while `exec()` submits to + the real DCP scheduler, needs a funded wallet, and depends on real + workers actually being present on the target compute group + (`demo`/`dcp`, the same public demo group both existing sample scripts + already use). **Passed, real end-to-end**: full real readystate + lifecycle (`exec → init → preauth → deploying → listeners → + compute-groups → uploading → deployed`), a real scheduler-assigned job + ID, 8 real `result` events from real workers, no `nofunds`/`error` + events, correct final output `YELLING!`. This confirms the rebuild is + solid for the actual production dispatch path, not just the + local-simulation path this document otherwise focuses on. **Also + re-verified after fix #11**, for the same reason as point 4. + +## Not yet done / open as of this writing + +- **Resolved, was previously unverified**: fix #11 above closed the 4th + missing JobQueue checkpoint. Before it, every claim in the Testing section + that touched an event/timer-driven callback (the `setTimeout`-Promise case + in point 3, and both `localExec()`/`exec()` end-to-end tests in points 4-5) + had actually been checked against a stale, pre-rewrite binary rather than + this PR's real build, and would have hung for anyone who ran them for + real. All were rerun against the actual rebuilt `pythonmonkey.pyd` and now + pass. Leaving this note here rather than deleting it: if a *fifth* + Python→JS callback path turns up somewhere that neither this fix nor the + original three cover, that would make two independent misses in the same + rewrite, which would be a good reason to stop patching call sites + one-by-one and instead audit every `JS_Call*`/`JS_Invoke` call in the + codebase for the same gap systematically. + +- **Not independently verified**: `Atomics.waitAsync` with a real timeout + (the `delayedDispatchToEventLoop` stub in fix #9 always declines these — + see that section's risk assessment). Only relevant if something in this + codebase's dependency tree actually uses that specific API; not exercised + by either test suite above. +- **Not independently verified**: the Debugger-API-facing parts of the + JobQueue rewrite (`saveJobQueue`/`SavedJobQueue`, `isDrainingStopped`, + the debug microtask queue via `useDebugQueue`) — pythonmonkey doesn't + currently expose SpiderMonkey's Debugger API to Python, so these paths + are believed unreachable in normal use, but that belief hasn't been + tested against actually invoking the Debugger API. +- **Not independently verified**: the `BufferType.cc` GC-safety reasoning + (fix #5, `JS_GetArrayBufferViewFixedData` → `JS_GetArrayBufferViewData` + + `AutoAssertNoGC`) — this needs either a compacting/moving-GC stress test + (`--enable-gczeal` or equivalent) or review by someone with real + SpiderMonkey GC internals expertise before being trusted beyond + experimentation. Both test suites above exercise TypedArray/buffer code + paths incidentally (numpy arrays flow through cloudpickle, not directly + through this code path) but do not specifically stress-test this. +- **Not run**: any long-running / soak test. Everything above is a single + run of each script; no repeated-execution, memory-leak, or + long-session-stability testing has been done. The heap-allocated + `JS::PersistentRooted` objects created per-microtask in fix #7 + (`JobQueue::runJobs`) are freed on the happy path (`runMicroTaskCallback`) + but **not** on at least one error path worth double-checking before a + soak test: if `PyEventLoop::getRunningLoop()` fails inside + `JobQueue::runJobs` after a `rootedJob` has been allocated, it is deleted + correctly (see that code) — but this exact path has not been exercised + by any test above, since the running loop was always available. +- **Not reviewed by anyone else.** Every fix in this document was made by + one engineer (with AI pair-programming assistance) working from primary + sources (the actual SpiderMonkey headers and, where embedder-facing + behavior was unclear, real production usage in Gecko's own source) rather + than guessing, and where a first attempt was wrong (see fix #9's + `transferToRuntime`/`ReleaseFailedTask` correction, and fix #7's + three-checkpoint hang), that's recorded rather than smoothed over — but + none of it has had a second, independent pair of eyes. Recommended before + shipping this beyond internal experimentation: a real code review of + sections 5, 7, and 9 in particular (the three sections marked NEEDS + REVIEW / moderate-or-higher risk above), ideally by someone with prior + SpiderMonkey embedding experience. +- The old `136a1` install and DLL were preserved as `*.orig-136a1-backup` / + `*.orig-backup` throughout this work (see individual sections above) — + don't delete these until the team has independently confirmed the new + build in their own environment, not just this one. diff --git a/include/JobQueue.hh b/include/JobQueue.hh index 36734f92..4c8dbc7d 100644 --- a/include/JobQueue.hh +++ b/include/JobQueue.hh @@ -49,43 +49,39 @@ bool init(JSContext *cx); * If any error happens while generating the host defined data, this method * should set a pending exception to `cx` and return `false`. */ -bool getHostDefinedData(JSContext *cx, JS::MutableHandle data) const override; +bool getHostDefinedData(JSContext *cx, JS::MutableHandle incumbentGlobal, JS::MutableHandle data) const override; /** - * @brief Enqueue a reaction job `job` for `promise`, which was allocated at - * `allocationSite`. Provide `incumbentGlobal` as the incumbent global for - * the reaction job's execution. + * @brief Ask the embedding for the host defined global to use when running + * a JS microtask. * - * `promise` can be null if the promise is optimized out. - * `promise` is guaranteed not to be optimized out if the promise has - * non-default user-interaction flag. + * Same "we don't track this" stance as getHostDefinedData() above -- falls + * back to SpiderMonkey's own default, matching the reference embedding + * (InternalJobQueue::getHostDefinedGlobal, js/src/vm/JSContext.cpp). */ -bool enqueuePromiseJob(JSContext *cx, JS::HandleObject promise, - JS::HandleObject job, JS::HandleObject allocationSite, - JS::HandleObject incumbentGlobal) override; +bool getHostDefinedGlobal(JSContext *cx, JS::MutableHandle out) const override; /** - * @brief Run all jobs in the queue. Running one job may enqueue others; continue to - * run jobs until the queue is empty. + * @brief Pull every job SpiderMonkey has queued internally since the last + * call, and forward each one to the Python event-loop for execution. + * + * SpiderMonkey no longer pushes promise jobs to the embedding as they're + * created (the old enqueuePromiseJob); it queues them internally and + * expects the embedder to pull them here on demand, via the free function + * js::RunJobs(cx) (jsfriendapi.h -- not the same thing as this method: it's + * what calls cx->jobQueue->runJobs(cx)). PythonMonkey calls + * js::RunJobs(GLOBAL_CX) after every top-level JS_ExecuteScript(), plus a + * few call sites where JS callbacks resolve promises outside of script + * execution (see JSFunctionProxy.cc, PromiseType.cc). * * Calling this method at the wrong time can break the web. The HTML spec * indicates exactly when the job queue should be drained (in HTML jargon, * when it should "perform a microtask checkpoint"), and doing so at other * times can incompatibly change the semantics of programs that use promises * or other microtask-based features. - * - * This method is called only via AutoDebuggerJobQueueInterruption, used by - * the Debugger API implementation to ensure that the debuggee's job queue is - * protected from the debugger's own activity. See the comments on - * AutoDebuggerJobQueueInterruption. */ void runJobs(JSContext *cx) override; -/** - * @return true if the job queue is empty, false otherwise. - */ -bool empty() const override; - /** * @return true if the job queue stopped draining, which results in `empty()` being false after `runJobs()`. */ @@ -127,11 +123,34 @@ js::UniquePtr saveJobQueue(JSContext *) override; * @brief The callback for dispatching an off-thread promise to the event loop * see https://hg.mozilla.org/releases/mozilla-esr102/file/tip/js/public/Promise.h#l580 * https://hg.mozilla.org/releases/mozilla-esr102/file/tip/js/src/vm/OffThreadPromiseRuntimeState.cpp#l160 + * + * Takes ownership of the Dispatchable (run via the public static + * Dispatchable::Run, since Dispatchable::run() is protected). + * * @param closure - closure, currently the javascript context - * @param dispatchable - Pointer to the Dispatchable to be called + * @param dispatchable - the Dispatchable to be called; ownership transferred to this callback * @return not shutting down */ -static bool dispatchToEventLoop(void *closure, JS::Dispatchable *dispatchable); +static bool dispatchToEventLoop(void *closure, js::UniquePtr &&dispatchable); + +/** + * @brief The callback for dispatching an off-thread promise to the event + * loop after a delay. + * + * Always returns false (no timeout manager available), which + * js/public/Promise.h documents as a valid response when the embedding + * can't service delayed cross-thread dispatch. Only affects SpiderMonkey + * features needing a delayed off-thread callback (e.g. an + * Atomics.waitAsync timeout) -- ordinary setTimeout/setInterval go through + * PyEventLoop::enqueueWithDelay instead and are unaffected. NEEDS REVIEW: + * not verified against a real Atomics.waitAsync-with-timeout case. + * + * @param closure - closure, currently the javascript context + * @param dispatchable - the Dispatchable that would be called; ownership transferred to this callback + * @param delay - requested delay in milliseconds + * @return false (no timeout manager available for cross-thread delayed dispatch) + */ +static bool delayedDispatchToEventLoop(void *closure, js::UniquePtr &&dispatchable, uint32_t delay); /** * @brief The callback that gets invoked whenever a Promise is rejected without a rejection handler (uncaught/unhandled exception) diff --git a/mozcentral.version b/mozcentral.version index 55aeecbf..2373436c 100644 --- a/mozcentral.version +++ b/mozcentral.version @@ -1 +1 @@ -6bca861985ba51920c1cacc21986af01c51bd690 +1704651e7d6c706fcb753adab577e0954d61cee0 diff --git a/setup.sh b/setup.sh index 68560ade..aeba9dce 100755 --- a/setup.sh +++ b/setup.sh @@ -15,41 +15,89 @@ if [[ "$OSTYPE" == "linux-gnu"* ]]; then # Linux echo "Installing apt packages" $SUDO apt-get install --yes cmake llvm clang pkg-config m4 unzip \ wget curl python3-dev + # SpiderMonkey's own build (build/moz.configure/toolchain.configure, + # minimum_gcc_version()) requires libstdc++ >= 10 regardless of which + # compiler is actually used -- CI's build container is ubuntu:20.04 + # (deliberately, for wheel glibc/libstdc++ compatibility -- see the CI + # workflow), whose *default* toolchain is gcc-9. libstdc++-10-dev is + # still available from 20.04's own default repos (no PPA needed) and + # only adds headers/static libs for clang to find -- it doesn't change + # which libstdc++.so.6 the built binary links against at runtime, so it + # doesn't narrow the wheel's runtime compatibility the container was + # chosen to preserve. + $SUDO apt-get install --yes libstdc++-10-dev elif [[ "$OSTYPE" == "darwin"* ]]; then # macOS brew update || true # allow failure brew install cmake pkg-config wget unzip coreutils # `coreutils` installs the `realpath` command brew install lld -elif [[ "$OSTYPE" == "msys"* ]]; then # Windows + # Xcode's bundled clang (16-17 on current runner images) is older than + # SpiderMonkey's own minimum at the current mozcentral.version pin (>=19, + # per its own configure error). Pinned to the major-19 formula rather than + # unversioned `llvm` (currently 23.x): homebrew-core only publishes bottles + # for `llvm` on recent arm64 macOS + Linux, so on Intel macOS and macOS 14 + # runners `brew install llvm` silently falls back to a from-source build + # (multi-hour, "Tier 3" unsupported) instead of installing a bottle -- + # confirmed via https://formulae.brew.sh/api/formula/llvm.json's bottle + # list lacking any Intel or "sonoma" entry, versus llvm@19's, which has + # both. 19.x still satisfies the >=19 requirement. + # homebrew's llvm keg isn't symlinked onto PATH by default, so put it + # first explicitly for the rest of this script. + brew install llvm@19 + export PATH="$(brew --prefix llvm@19)/bin:$PATH" +elif [[ "$OSTYPE" == "msys"* || "$OSTYPE" == "cygwin"* ]]; then # Windows echo "Dependencies are not going to be installed automatically on Windows." else echo "Unsupported OS" exit 1 fi # Install rust compiler -echo "Installing rust compiler" -unset HOST_ABI_FLAGS -if [[ "$OSTYPE" == "msys"* ]]; then # Windows - HOST_ABI_FLAGS=("--default-host" "$(clang --print-target-triple)") +# Skip if already installed: re-running rustup-init.sh here downloads and +# runs a fresh installer exe, which Defender/SmartScreen blocks on this box. +if command -v rustup >/dev/null && rustup toolchain list 2>/dev/null | grep -q '^1\.90'; then + echo "Rust 1.90 toolchain already installed, skipping rustup-init" +else + echo "Installing rust compiler" + unset HOST_ABI_FLAGS + if [[ "$OSTYPE" == "msys"* || "$OSTYPE" == "cygwin"* ]]; then # Windows + HOST_ABI_FLAGS=("--default-host" "$(clang --print-target-triple)") + fi + # SpiderMonkey at the current mozcentral.version pin requires at least + # rustc 1.90.0 (confirmed via its own configure error message). + curl --proto '=https' --tlsv1.2 https://raw.githubusercontent.com/rust-lang/rustup/refs/tags/1.28.2/rustup-init.sh -sSf | sh -s -- -y ${HOST_ABI_FLAGS+"${HOST_ABI_FLAGS[@]}"} --default-toolchain 1.90.0 fi -curl --proto '=https' --tlsv1.2 https://raw.githubusercontent.com/rust-lang/rustup/refs/tags/1.28.2/rustup-init.sh -sSf | sh -s -- -y ${HOST_ABI_FLAGS+"${HOST_ABI_FLAGS[@]}"} --default-toolchain 1.85 CARGO_BIN="$HOME/.cargo/bin/cargo" # also works for Windows. On Windows this equals to %USERPROFILE%\.cargo\bin\cargo -$CARGO_BIN install cbindgen +command -v cbindgen >/dev/null || $CARGO_BIN install cbindgen # Setup Poetry -echo "Installing poetry" -curl -sSL https://install.python-poetry.org | python3 - --version "1.7.1" -if [[ "$OSTYPE" == "msys"* ]]; then # Windows +if [[ "$OSTYPE" == "msys"* || "$OSTYPE" == "cygwin"* ]]; then # Windows POETRY_BIN="$APPDATA/Python/Scripts/poetry" else POETRY_BIN="$HOME/.local/bin/poetry" fi -$POETRY_BIN self add 'poetry-dynamic-versioning[plugin]' +# Skip if already installed (same idempotency reasoning as rustup above). +# Falls back to `python` since this machine has no `python3` on PATH. +# Poetry is still needed below by the .git/hooks/pre-commit branch. +if command -v "$POETRY_BIN" >/dev/null || [ -x "$POETRY_BIN" ]; then + echo "Poetry already installed, skipping" +else + echo "Installing poetry" + PYTHON_FOR_POETRY=$(command -v python3 || command -v python) + curl -sSL https://install.python-poetry.org | "$PYTHON_FOR_POETRY" - --version "1.7.1" + "$POETRY_BIN" self add 'poetry-dynamic-versioning[plugin]' +fi echo "Done installing dependencies" echo "Downloading spidermonkey source code" # Read the commit hash for mozilla-central from the `mozcentral.version` file MOZCENTRAL_VERSION=$(cat mozcentral.version) -wget -c -q -O firefox-source-${MOZCENTRAL_VERSION}.zip https://github.com/mozilla-firefox/firefox/archive/${MOZCENTRAL_VERSION}.zip -unzip -q firefox-source-${MOZCENTRAL_VERSION}.zip && mv firefox-${MOZCENTRAL_VERSION} firefox-source +# Skip if already extracted -- lets this script be re-run after a later +# step fails without re-downloading/re-extracting every time. +if [ ! -d firefox-source ]; then + # curl instead of wget: wget.exe is blocked by this machine's WDAC policy. + curl -fsSL -o firefox-source-${MOZCENTRAL_VERSION}.zip https://github.com/mozilla-firefox/firefox/archive/${MOZCENTRAL_VERSION}.zip + unzip -q firefox-source-${MOZCENTRAL_VERSION}.zip && mv firefox-${MOZCENTRAL_VERSION} firefox-source +else + echo "firefox-source already exists, skipping download+extract" +fi echo "Done downloading spidermonkey source code" echo "Building spidermonkey" @@ -69,6 +117,7 @@ sed -i'' -e '/MOZ_CRASH_UNSAFE_PRINTF/,/__PRETTY_FUNCTION__);/d' ./mfbt/LinkedLi sed -i'' -e '/MOZ_ASSERT(stackRootPtr == nullptr);/d' ./js/src/vm/JSContext.cpp # would assert false in Debug Build since we extensively use `new JS::Rooted` sed -i'' -e 's/"-fuse-ld=ld"/"-ld64" if c_compiler.version > "14.0.0" else "-fuse-ld=ld"/' ./build/moz.configure/toolchain.configure # XCode 15 changed the linker behaviour. See https://developer.apple.com/documentation/xcode-release-notes/xcode-15-release-notes#Linking sed -i'' -e 's/defined(XP_WIN)/defined(_WIN32)/' ./mozglue/baseprofiler/public/BaseProfilerUtils.h # this header file is introduced to js/Debug.h in https://phabricator.services.mozilla.com/D221102, but it would be compiled without XP_WIN in this building configuration +sed -i'' -e 's/os\.environ\["MOZILLABUILD"\]/os.environ.get("MOZILLABUILD", "")/g' ./python/mozbuild/mozbuild/backend/visualstudio.py # avoid KeyError: we don't use the official Mozilla Build package, so this is never set cd js/src mkdir -p _build @@ -77,16 +126,14 @@ mkdir -p ../../../../_spidermonkey_install/ ../configure --target=$(clang --print-target-triple) \ --prefix=$(realpath $PWD/../../../../_spidermonkey_install) \ --with-intl-api \ - $(if [[ "$OSTYPE" != "msys"* ]]; then echo "--without-system-zlib"; fi) \ + $(if [[ "$OSTYPE" != "msys"* && "$OSTYPE" != "cygwin"* ]]; then echo "--without-system-zlib"; fi) \ --disable-debug-symbols \ --disable-jemalloc \ --disable-tests \ $(if [[ "$OSTYPE" == "darwin"* ]]; then echo "--enable-linker=ld64"; fi) \ - --enable-optimize \ - --disable-explicit-resource-management -# disable-explicit-resource-management: Disable the `using` syntax that is enabled by default in SpiderMonkey nightly, otherwise the header files will disagree with the compiled lib .so file -# when it's using a `IF_EXPLICIT_RESOURCE_MANAGEMENT` macro, e.g., the `enum JSProtoKey` index would be off by 1 (header `JSProto_Uint8Array` 27 will be interpreted as `JSProto_Int8Array` in lib as lib has an extra element) -# https://bugzilla.mozilla.org/show_bug.cgi?id=1940342 + --enable-optimize +# --disable-explicit-resource-management (worked around Bugzilla 1940342) +# is no longer a recognized flag -- the feature it gated has since shipped. make -j$CPUS echo "Done building spidermonkey" @@ -120,7 +167,7 @@ if test -f .git/hooks/pre-commit; then cd uncrustify-source mkdir -p build cd build - if [[ "$OSTYPE" == "msys"* ]]; then # Windows + if [[ "$OSTYPE" == "msys"* || "$OSTYPE" == "cygwin"* ]]; then # Windows cmake ../ cmake --build . -j$CPUS --config Release cp Release/uncrustify.exe ../../uncrustify.exe diff --git a/src/BufferType.cc b/src/BufferType.cc index f0726bce..40926ee7 100644 --- a/src/BufferType.cc +++ b/src/BufferType.cc @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -88,9 +89,23 @@ PyObject *BufferType::fromJsTypedArray(JSContext *cx, JS::HandleObject typedArra bool isSharedMemory; if (!JS_GetArrayBufferViewBuffer(cx, typedArray, &isSharedMemory)) return nullptr; - uint8_t __destBuf[0] = {}; // we don't care about its value as it's used only if the TypedArray still having inline data - uint8_t *data = JS_GetArrayBufferViewFixedData(typedArray, __destBuf, 0 /* making sure we don't copy inline data */); - if (data == nullptr) { // shared memory or still having inline data + if (isSharedMemory) { + PyErr_SetString(PyExc_TypeError, "PythonMonkey cannot coerce TypedArrays backed by shared memory."); + return nullptr; + } + + // NEEDS REVIEW: JS_GetArrayBufferViewFixedData was removed upstream; its + // replacement trades the old "return nullptr if data is still inline/ + // movable" runtime guard for a caller-supplied no-GC token. Safety here + // relies on JS_GetArrayBufferViewBuffer() above having already promoted + // any inline data to a stable allocation -- not independently verified + // against SpiderMonkey's GC (e.g. via --enable-gczeal). AutoAssertNoGC, + // not the base AutoRequireNoGC (protected ctor), since it actually + // asserts at runtime in diagnostic builds instead of being a bare marker. + JS::AutoAssertNoGC nogc(cx); + bool isSharedMemory2; // redundant with isSharedMemory above; required by this function's signature + uint8_t *data = static_cast(JS_GetArrayBufferViewData(typedArray, &isSharedMemory2, nogc)); + if (data == nullptr) { PyErr_SetString(PyExc_TypeError, "PythonMonkey cannot coerce TypedArrays backed by shared memory."); return nullptr; } diff --git a/src/JSFunctionProxy.cc b/src/JSFunctionProxy.cc index 99a32552..88bcecba 100644 --- a/src/JSFunctionProxy.cc +++ b/src/JSFunctionProxy.cc @@ -16,6 +16,7 @@ #include "include/setSpiderMonkeyException.hh" #include +#include #include @@ -59,6 +60,11 @@ PyObject *JSFunctionProxyMethodDefinitions::JSFunctionProxy_call(PyObject *self, return NULL; } + // This is the generic entry point for any Python->JS callback (e.g. a + // setTimeout callback), so a Promise resolved here has nothing else + // scheduled to drain its reaction jobs. See JobQueue.cc's runJobs. + js::RunJobs(cx); + if (PyErr_Occurred()) { return NULL; } diff --git a/src/JSMethodProxy.cc b/src/JSMethodProxy.cc index 78e1189b..ad0ada61 100644 --- a/src/JSMethodProxy.cc +++ b/src/JSMethodProxy.cc @@ -16,6 +16,7 @@ #include "include/setSpiderMonkeyException.hh" #include +#include #include @@ -70,6 +71,9 @@ PyObject *JSMethodProxyMethodDefinitions::JSMethodProxy_call(PyObject *self, PyO return NULL; } + // Same checkpoint as JSFunctionProxy_call, for bound methods. + js::RunJobs(cx); + if (PyErr_Occurred()) { return NULL; } diff --git a/src/JobQueue.cc b/src/JobQueue.cc index 928746fd..4215143d 100644 --- a/src/JobQueue.cc +++ b/src/JobQueue.cc @@ -14,11 +14,12 @@ #include "include/PyEventLoop.hh" #include "include/pyTypeFactory.hh" #include "include/PromiseType.hh" +#include "include/setSpiderMonkeyException.hh" #include #include -#include +#include #include @@ -26,41 +27,93 @@ JobQueue::JobQueue(JSContext *cx) { finalizationRegistryCallbacks = new JS::PersistentRooted(cx); // Leaks but it's OK since freed at process exit } -bool JobQueue::getHostDefinedData(JSContext *cx, JS::MutableHandle data) const { +bool JobQueue::getHostDefinedData(JSContext *cx, JS::MutableHandle incumbentGlobal, JS::MutableHandle data) const { + incumbentGlobal.set(nullptr); // We don't need the incumbent global data.set(nullptr); // We don't need the host defined data return true; // `true` indicates no error } -bool JobQueue::enqueuePromiseJob(JSContext *cx, - [[maybe_unused]] JS::HandleObject promise, - JS::HandleObject job, - [[maybe_unused]] JS::HandleObject allocationSite, - JS::HandleObject incumbentGlobal) { - - // Convert the `job` JS function to a Python function for event-loop callback - JS::RootedValue jobv(cx, JS::ObjectValue(*job)); - PyObject *callback = pyTypeFactory(cx, jobv); - - // Send job to the running Python event-loop - PyEventLoop loop = PyEventLoop::getRunningLoop(); - if (!loop.initialized()) return false; +bool JobQueue::getHostDefinedGlobal(JSContext *cx, JS::MutableHandle out) const { + out.set(nullptr); + return true; +} - // Inform the JS runtime that the job queue is no longer empty - JS::JobQueueMayNotBeEmpty(cx); +// The PyCFunction invoked by the Python event-loop once it's ready to run a +// single deferred JS microtask. `closure` is a 2-tuple of (JSContext*, +// JS::PersistentRooted* job), both smuggled through as PyLong +// pointers the same way JobQueue::dispatchToEventLoop's callDispatchFunc +// does below for JS::Dispatchable. +static PyObject *runMicroTaskCallback(PyObject *closure, PyObject *Py_UNUSED(unused)) { + JSContext *cx = (JSContext *)PyLong_AsVoidPtr(PyTuple_GetItem(closure, 0)); + auto *rootedJob = (JS::PersistentRooted *)PyLong_AsVoidPtr(PyTuple_GetItem(closure, 1)); + + JS::Rooted job(cx, rootedJob->get()); + delete rootedJob; // the PersistentRooted was only needed to keep `job` alive until now + + bool ok = true; + JSObject *global = JS::GetExecutionGlobalFromJSMicroTask(job); + if (global) { + JSAutoRealm ar(cx, global); + ok = JS::RunJSMicroTask(cx, job); + } - loop.enqueue(callback); + // Running this microtask may enqueue the next one in an await chain -- + // re-checkpoint so it doesn't just sit there. See also PromiseType.cc. + js::RunJobs(cx); - Py_DECREF(callback); - return true; + if (!ok) { + setSpiderMonkeyException(cx); + return NULL; // propagates as a Python exception; PyEventLoop's own + // eventLoopJobWrapper surfaces it to the loop's exception handler + } + Py_RETURN_NONE; } -void JobQueue::runJobs(JSContext *cx) { - // Do nothing -} +static PyMethodDef runMicroTaskCallbackDef = {"JsMicroTaskCallable", runMicroTaskCallback, METH_NOARGS, NULL}; -bool JobQueue::empty() const { - // TODO (Tom Tang): implement using `get_running_loop` and getting job count on loop??? - return true; // see https://hg.mozilla.org/releases/mozilla-esr128/file/tip/js/src/builtin/Promise.cpp#l6946 +// NEEDS REVIEW: GC-safety of rooting a JSMicroTask* across the gap between +// dequeuing it here and the Python event-loop calling runMicroTaskCallback +// is modelled on the finalizationRegistryCallbacks pattern below, but not +// independently verified for JSMicroTask specifically. Also unverified: +// that draining once per top-level JS_ExecuteScript() (pythonmonkey.cc) is +// the only place a checkpoint is needed for this embedding. +void JobQueue::runJobs(JSContext *cx) { + while (JS::HasAnyMicroTasks(cx)) { + JS::RootedValue entry(cx, JS::DequeueNextMicroTask(cx)); + if (entry.isNull()) { + break; + } + + JS::Rooted job(cx, JS::ToMaybeWrappedJSMicroTask(entry)); + if (!job) { + continue; // not a JS microtask; nothing we support runs these + } + + // Root the job on the heap so it survives until the Python event-loop + // calls back into us, which may be well after this function returns. + auto *rootedJob = new JS::PersistentRooted(cx, job); + + PyObject *cxArg = PyLong_FromVoidPtr(cx); + PyObject *jobArg = PyLong_FromVoidPtr(rootedJob); + PyObject *closure = PyTuple_Pack(2, cxArg, jobArg); + Py_DECREF(cxArg); + Py_DECREF(jobArg); + PyObject *callback = PyCFunction_New(&runMicroTaskCallbackDef, closure); + Py_DECREF(closure); + + PyEventLoop loop = PyEventLoop::getRunningLoop(); + if (!loop.initialized()) { + delete rootedJob; + Py_DECREF(callback); + return; + } + + // Inform the JS runtime that the job queue is no longer empty + JS::JobQueueMayNotBeEmpty(cx); + + loop.enqueue(callback); + Py_DECREF(callback); + } } bool JobQueue::isDrainingStopped() const { @@ -79,7 +132,9 @@ js::UniquePtr JobQueue::saveJobQueue(JSContext *cx) bool JobQueue::init(JSContext *cx) { JS::SetJobQueue(cx, this); - JS::InitDispatchToEventLoop(cx, dispatchToEventLoop, cx); + // Last two args (asyncTaskStarted/FinishedCallback) are optional; this + // embedding doesn't need to track background-task liveness. + JS::InitAsyncTaskCallbacks(cx, dispatchToEventLoop, delayedDispatchToEventLoop, nullptr, nullptr, cx); JS::SetPromiseRejectionTrackerCallback(cx, promiseRejectionTracker); return true; } @@ -87,13 +142,23 @@ bool JobQueue::init(JSContext *cx) { static PyObject *callDispatchFunc(PyObject *dispatchFuncTuple, PyObject *Py_UNUSED(unused)) { JSContext *cx = (JSContext *)PyLong_AsVoidPtr(PyTuple_GetItem(dispatchFuncTuple, 0)); JS::Dispatchable *dispatchable = (JS::Dispatchable *)PyLong_AsVoidPtr(PyTuple_GetItem(dispatchFuncTuple, 1)); - dispatchable->run(cx, JS::Dispatchable::NotShuttingDown); + // Dispatchable::run() is protected; reconstruct the UniquePtr released + // into raw form by dispatchToEventLoop() below and run it via Run(). + JS::Dispatchable::Run(cx, js::UniquePtr(dispatchable), JS::Dispatchable::NotShuttingDown); + + // This resumes JS execution (e.g. finishing an off-thread WebAssembly + // compile/instantiate), which can settle promises and enqueue reaction + // jobs -- same as the other checkpoints in this file, nothing else drains + // this one. Without it, `await WebAssembly.instantiate(...)` hangs forever + // even though the dispatchable itself ran successfully. + js::RunJobs(cx); + Py_RETURN_NONE; } static PyMethodDef callDispatchFuncDef = {"JsDispatchCallable", callDispatchFunc, METH_NOARGS, NULL}; -bool JobQueue::dispatchToEventLoop(void *closure, JS::Dispatchable *dispatchable) { +bool JobQueue::dispatchToEventLoop(void *closure, js::UniquePtr &&dispatchable) { JSContext *cx = (JSContext *)closure; // The `dispatchToEventLoop` function is running in a helper thread, so @@ -101,7 +166,10 @@ bool JobQueue::dispatchToEventLoop(void *closure, JS::Dispatchable *dispatchable // see https://docs.python.org/3/c-api/init.html#non-python-created-threads PyGILState_STATE gstate = PyGILState_Ensure(); - PyObject *dispatchFuncTuple = PyTuple_Pack(2, PyLong_FromVoidPtr(cx), PyLong_FromVoidPtr(dispatchable)); + // Release ownership into a raw pointer to smuggle it through the Python + // closure; reclaimed by callDispatchFunc via Dispatchable::Run above. + JS::Dispatchable *raw = dispatchable.release(); + PyObject *dispatchFuncTuple = PyTuple_Pack(2, PyLong_FromVoidPtr(cx), PyLong_FromVoidPtr(raw)); PyObject *pyFunc = PyCFunction_New(&callDispatchFuncDef, dispatchFuncTuple); // Avoid using the current, JS helper thread to send jobs to event-loop as it may cause deadlock @@ -111,6 +179,15 @@ bool JobQueue::dispatchToEventLoop(void *closure, JS::Dispatchable *dispatchable return true; } +bool JobQueue::delayedDispatchToEventLoop(void *closure, js::UniquePtr &&dispatchable, uint32_t delay) { + // No cross-thread-safe delayed-dispatch mechanism here (see JobQueue.hh). + // ReleaseFailedTask is the embedder-facing way to decline after taking + // ownership -- transferToRuntime() is SpiderMonkey's own internal use + // and is protected. + JS::Dispatchable::ReleaseFailedTask(std::move(dispatchable)); + return false; +} + bool sendJobToMainLoop(PyObject *pyFunc) { PyGILState_STATE gstate = PyGILState_Ensure(); @@ -165,7 +242,8 @@ void JobQueue::promiseRejectionTracker(JSContext *cx, } void JobQueue::queueFinalizationRegistryCallback(JSFunction *callback) { - mozilla::Unused << finalizationRegistryCallbacks->append(callback); + // mozilla::Unused (mfbt) was removed upstream; it was just a discard cast. + (void)finalizationRegistryCallbacks->append(callback); } bool JobQueue::runFinalizationRegistryCallbacks(JSContext *cx) { @@ -179,7 +257,7 @@ bool JobQueue::runFinalizationRegistryCallbacks(JSContext *cx) { JS::RootedFunction func(cx, f); JS::RootedValue unused_rval(cx); // we don't raise an exception here because there is nowhere to catch it - mozilla::Unused << JS_CallFunction(cx, NULL, func, JS::HandleValueArray::empty(), &unused_rval); + (void)JS_CallFunction(cx, NULL, func, JS::HandleValueArray::empty(), &unused_rval); ranCallbacks = true; } diff --git a/src/PromiseType.cc b/src/PromiseType.cc index 5a3f94b3..52082450 100644 --- a/src/PromiseType.cc +++ b/src/PromiseType.cc @@ -78,6 +78,11 @@ PyObject *PromiseType::getPyObject(JSContext *cx, JS::HandleObject promise) { js::SetFunctionNativeReserved(onResolved, PROMISE_OBJ_SLOT, JS::ObjectValue(*promise)); JS::AddPromiseReactions(cx, promise, onResolved, onResolved); + // If `promise` was already settled, AddPromiseReactions just queued a job + // with nothing else scheduled to drain it (we're outside JS_ExecuteScript + // here). See JobQueue::runJobs. + js::RunJobs(cx); + return future.getFutureObject(); // must be a new reference, ref count == 3 // Here the ref count for the `future` object is 3, but will immediately decrease to 2 in `PyEventLoop::Future`'s destructor when the `PromiseType::getPyObject` function ends // Leaving one reference for the returned Python object, and another one for the `onResolved` callback function @@ -109,6 +114,11 @@ static PyObject *futureOnDoneCallback(PyObject *futureCallbackTuple, PyObject *a } else { // having exception set, to reject the promise JS::RejectPromise(cx, promise, JS::RootedValue(cx, jsTypeFactorySafe(cx, exception))); } + + // Same as getPyObject above: resolving/rejecting here may queue reaction + // jobs with nothing else scheduled to drain them. + js::RunJobs(cx); + Py_XDECREF(exception); // cleanup delete rootedPtr; // no longer needed to be rooted, clean it up diff --git a/src/modules/pythonmonkey/pythonmonkey.cc b/src/modules/pythonmonkey/pythonmonkey.cc index 8408b594..f3c91cbf 100644 --- a/src/modules/pythonmonkey/pythonmonkey.cc +++ b/src/modules/pythonmonkey/pythonmonkey.cc @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -85,6 +86,25 @@ void nurseryCollectionCallback(JSContext *cx, JS::GCNurseryProgress progress, JS } } +// pythonmonkey doesn't implement module loading, so `import(...)` must be +// rejected rather than left unhandled. HostLoadImportedModule +// (js/src/vm/Modules.cpp) only auto-finishes the promise on this path when a +// hook IS registered but returns false; when no hook is registered at all it +// reports "Module load hook not set" and returns without ever settling the +// promise or clearing that pending exception -- leaving `await import(...)` +// hung forever and a stale exception on the context. Registering this hook +// (even though it never resolves anything) makes pythonmonkey responsible +// for finishing the promise itself, as every embedder that permits dynamic +// import syntax at all is required to be. +static bool pythonmonkeyModuleLoadHook( + JSContext *cx, JS::Handle referrer, JS::Handle moduleRequest, + JS::Handle hostDefined, JS::Handle payload, + uint32_t lineNumber, JS::ColumnNumberOneOrigin columnNumber +) { + JS_ReportErrorASCII(cx, "Dynamic module import is disabled or not supported in this context"); + return false; +} + bool functionRegistryCallback(JSContext *cx, unsigned int argc, JS::Value *vp) { JS::CallArgs callargs = JS::CallArgsFromVp(argc, vp); Py_DECREF((PyObject *)callargs[0].toPrivate()); @@ -488,6 +508,10 @@ static PyObject *eval(PyObject *self, PyObject *args) { return NULL; } + // Mirrors the HTML spec's "clean up after running script" checkpoint -- + // see JobQueue::runJobs for why the embedder now has to drain this itself. + js::RunJobs(GLOBAL_CX); + // translate to the proper python type PyObject *returnValue = pyTypeFactory(GLOBAL_CX, rval); if (PyErr_Occurred()) { @@ -571,9 +595,10 @@ PyMODINIT_FUNC PyInit_pythonmonkey(void) return NULL; } + // asm.js was removed from SpiderMonkey (superseded by WebAssembly, set + // via .setWasm(true) below); ContextOptions::setAsmJS no longer exists. JS::ContextOptionsRef(GLOBAL_CX) .setWasm(true) - .setAsmJS(true) .setAsyncStack(true) .setSourcePragmas(true); @@ -583,6 +608,8 @@ PyMODINIT_FUNC PyInit_pythonmonkey(void) return NULL; } + JS::SetModuleLoadHook(JS_GetRuntime(GLOBAL_CX), pythonmonkeyModuleLoadHook); + if (!JS::InitSelfHostedCode(GLOBAL_CX)) { PyErr_SetString(SpiderMonkeyError, "Spidermonkey could not initialize self-hosted code."); return NULL; @@ -594,6 +621,10 @@ PyMODINIT_FUNC PyInit_pythonmonkey(void) JS::AddGCNurseryCollectionCallback(GLOBAL_CX, nurseryCollectionCallback, NULL); JS::RealmCreationOptions creationOptions = JS::RealmCreationOptions(); + // Off by default (a Spectre mitigation for untrusted web content, which + // doesn't apply to this embedded single-process run); Pyodide's threaded + // WASM build otherwise fails to link ("shared memory is disabled"). + creationOptions.setSharedMemoryAndAtomicsEnabled(true); JS::RealmBehaviors behaviours = JS::RealmBehaviors(); JS::RealmOptions options = JS::RealmOptions(creationOptions, behaviours); static JSClass globalClass = {"global", JSCLASS_GLOBAL_FLAGS, &JS::DefaultGlobalClassOps};