Skip to content

Commit d74e5ee

Browse files
fix: two more missing JobQueue checkpoints found via full-suite CI repro
CI has been hanging on (ubuntu-22.04, 3.8)/(3.10) with no further output after test_dicts.py. Reproduced locally by running the actual full suite (not a hand-picked subset) and bisecting with a faulthandler watchdog: 1. pythonmonkey never registers a JS::ModuleLoadHook, so `await pm.eval("import(...)")` hangs forever -- js::HostLoadImportedModule discards the "no hook" failure via TryStartDynamicModuleImport's `(void)` cast instead of ever settling the promise. Fixed by registering a hook that immediately fails every load and properly finishes the promise. 2. JobQueue.cc's callDispatchFunc (the off-thread dispatch path used by WebAssembly.instantiate) never called js::RunJobs(cx) after running the dispatchable, so any promise it resolved was never drained -- the same missing-checkpoint bug as fix #7/#11, a 5th instance of it. Both confirmed via isolated repros (hung every time before, resolve cleanly after, 3/3 runs) and a full `pytest tests/python` run: 676 passed in 59.8s, zero hangs, zero crashes, where before it would hang or crash partway through depending on unrelated heap timing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent f6714c7 commit d74e5ee

3 files changed

Lines changed: 154 additions & 0 deletions

File tree

‎SPIDERMONKEY_VERSION_BUMP.md‎

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -698,6 +698,130 @@ and a few manual checks look right" turned out not to mean "actually works"
698698
this rewrite (the Debugger-API paths flagged in "Not yet done" below), not
699699
just the two paths that have now each independently failed once.
700700

701+
### 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)
702+
703+
**How this was found**: the user reported CI hangs on `(ubuntu-22.04, 3.8)`
704+
and `(ubuntu-22.04, 3.10)` with no further output after `test_dicts.py`.
705+
Reproducing locally with `pytest tests/python/test_event_loop.py::test_promises`
706+
alone did not hang (fast pass), but running the *actual full suite*
707+
(`pytest tests/python`, matching CI's own invocation) reproduced it -- and, on
708+
this machine, sometimes as a hang and sometimes as a genuine Windows access
709+
violation later in the same run (see fix #13; two independent bugs were
710+
stacked behind each other, and the first one being a hang meant the second
711+
one was never reached before). Bisected `test_promises` line-by-line with a
712+
`faulthandler.dump_traceback_later` watchdog down to one exact statement:
713+
714+
```python
715+
with pytest.raises(pm.SpiderMonkeyError,
716+
match="\nError: Dynamic module import is disabled or not supported in this context"):
717+
await pm.eval("import('some_module')")
718+
```
719+
720+
**Root cause**: pythonmonkey never calls `JS::SetModuleLoadHook`. Reading
721+
`js::HostLoadImportedModule` (`js/src/vm/Modules.cpp`) directly: when
722+
`cx->runtime()->moduleLoadHook` is null, it calls `JS_ReportErrorASCII(cx,
723+
"Module load hook not set")` and returns `false` immediately -- it does
724+
**not** fall through to `FinishLoadingImportedModuleFailedWithPendingException`
725+
the way it does when a hook *is* registered but itself returns `false`. Its
726+
caller, `TryStartDynamicModuleImport`, discards that return value
727+
(`(void)HostLoadImportedModule(...)`) and unconditionally returns `true`, so
728+
`StartDynamicModuleImport`'s own fallback (`RejectPromiseWithPendingError`) is
729+
never reached either. Net effect: the promise returned by `import(...)` is
730+
created and returned to script, but nothing ever resolves or rejects it --
731+
confirmed empirically (`import('some_module').then(onResolve, onReject)`
732+
called neither callback, ever) -- and the pending "Module load hook not set"
733+
exception is left dangling on `cx`, uncleared. `await`ing that promise from
734+
Python hangs forever, since `PromiseType::getPyObject`'s own `js::RunJobs(cx)`
735+
checkpoint has nothing to drain: no reaction job is ever enqueued for a
736+
promise that never settles.
737+
738+
This is not a regression introduced by anything else in this document -- this
739+
codepath has presumably always been broken the same way, just never
740+
exercised end-to-end before. It surfaced now because this was the first time
741+
the full suite was actually run to completion against a real rebuilt binary
742+
in one continuous session, one test after another with no gaps.
743+
744+
**Fix**: register a `JS::ModuleLoadHook` at context-init time
745+
(`PyInit_pythonmonkey`, right after `JOB_QUEUE->init`) that immediately fails
746+
every load with `JS_ReportErrorASCII` and returns `false` -- making
747+
pythonmonkey the thing responsible for finishing the promise, exactly as the
748+
`JS::ModuleLoadHook` doc comment (`js/public/Modules.h`) requires of any
749+
embedder that permits dynamic-import syntax to be parsed at all:
750+
751+
```diff
752+
+ JS::SetModuleLoadHook(JS_GetRuntime(GLOBAL_CX), pythonmonkeyModuleLoadHook);
753+
```
754+
755+
Since a hook is now registered, `HostLoadImportedModule` takes the
756+
already-correct `if (!ok) { ... FinishLoadingImportedModuleFailedWithPendingException(...) }`
757+
path instead of the no-hook early-return, and the promise rejects properly
758+
with an `Error: Dynamic module import is disabled or not supported in this
759+
context` -- which happens to be exactly the message the pre-existing test
760+
already expected, strongly suggesting this is what the test always assumed
761+
would happen and never got to verify.
762+
763+
**Retested**: the exact `pytest.raises(...)` block above now passes; the
764+
full `test_promises` test passes; the isolated minimal repro
765+
(`await pm.eval("import('some_module')")`) resolves (rejects) immediately
766+
instead of hanging, across 3 repeated runs.
767+
768+
### 13. `src/JobQueue.cc` -- a 5th missing JobQueue checkpoint, in the off-thread dispatch path (`callDispatchFunc`)
769+
770+
**This is the "fifth Python-to-JS callback path" scenario fix #11 flagged as a
771+
reason to stop patching call sites one-by-one.** Found immediately after
772+
fixing #12 above, once the full suite could progress far enough to reach it:
773+
`test_webassembly` (off-thread `WebAssembly.instantiate`) hung in isolation,
774+
and crashed with a genuine Windows access violation when run as part of the
775+
full suite (same bug, two different symptoms depending on unrelated heap
776+
state at the time -- this is almost certainly also the explanation for why
777+
the CI hang and this session's local access-violation crash looked different
778+
from each other despite sharing a root cause upstream of this fix).
779+
780+
**Root cause**: `JobQueue::dispatchToEventLoop` (the `JS::InitAsyncTaskCallbacks`
781+
callback added in fix #9, invoked by SpiderMonkey from a helper thread when
782+
off-thread work like WebAssembly compilation finishes) hands the JS
783+
`Dispatchable` off to `callDispatchFunc`, which runs it via
784+
`JS::Dispatchable::Run(cx, ...)`. Confirmed by instrumenting every step with
785+
`fprintf(stderr, ...)` and rebuilding: the dispatch machinery itself works
786+
correctly end-to-end (helper thread -> spawned thread -> main loop ->
787+
`callDispatchFunc` all ran and returned normally, twice, matching
788+
WebAssembly's compile-then-instantiate two-phase off-thread completion) -- but
789+
`callDispatchFunc` never called `js::RunJobs(cx)` afterward. Running the
790+
dispatchable resumes JS execution that settles the WebAssembly promise and
791+
enqueues its reaction job, and -- same story as fixes #7 and #11 -- nothing
792+
was draining it.
793+
794+
**Fix**, same pattern as every other checkpoint in this rewrite:
795+
796+
```diff
797+
JS::Dispatchable::Run(cx, js::UniquePtr<JS::Dispatchable>(dispatchable), JS::Dispatchable::NotShuttingDown);
798+
+
799+
+ js::RunJobs(cx);
800+
+
801+
Py_RETURN_NONE;
802+
```
803+
804+
**Retested**: the isolated `WebAssembly.instantiate(...).then(...)` repro
805+
resolves correctly across 3 repeated runs (previously hung every time); the
806+
full `test_webassembly` test passes.
807+
808+
**This makes five independent missing-checkpoint sites found across fixes #7,
809+
#11, and #13 (`JobQueue::runJobs`'s own callback, `PromiseType::getPyObject`,
810+
`futureOnDoneCallback`, `JSFunctionProxy_call`/`JSMethodProxy_call`, and now
811+
`callDispatchFunc`).** Per fix #11's own stated threshold, this is the signal
812+
to stop finding these one at a time: **someone should systematically audit
813+
every place in this codebase that resumes JS execution from outside a
814+
top-level `JS_ExecuteScript()` call** (grep for `JS_Call*`, `JS_Invoke`,
815+
`Dispatchable::Run`, and any other JS-entry point) and confirm each one
816+
either already has a `js::RunJobs(cx)` checkpoint or is proven not to need
817+
one, rather than continuing to wait for the next one to surface as a hang or
818+
a crash in someone's test run.
819+
820+
**Full suite re-run after both fix #12 and #13**: `pytest tests/python` (all
821+
files, not a subset) -- **676 passed in 59.8s, zero hangs, zero crashes** --
822+
confirmed on this machine (Windows, Python 3.14.7) in one continuous run
823+
immediately after applying both fixes and rebuilding.
824+
701825
---
702826

703827
## Testing — what was actually run, and what it showed

‎src/JobQueue.cc‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,14 @@ static PyObject *callDispatchFunc(PyObject *dispatchFuncTuple, PyObject *Py_UNUS
145145
// Dispatchable::run() is protected; reconstruct the UniquePtr released
146146
// into raw form by dispatchToEventLoop() below and run it via Run().
147147
JS::Dispatchable::Run(cx, js::UniquePtr<JS::Dispatchable>(dispatchable), JS::Dispatchable::NotShuttingDown);
148+
149+
// This resumes JS execution (e.g. finishing an off-thread WebAssembly
150+
// compile/instantiate), which can settle promises and enqueue reaction
151+
// jobs -- same as the other checkpoints in this file, nothing else drains
152+
// this one. Without it, `await WebAssembly.instantiate(...)` hangs forever
153+
// even though the dispatchable itself ran successfully.
154+
js::RunJobs(cx);
155+
148156
Py_RETURN_NONE;
149157
}
150158

‎src/modules/pythonmonkey/pythonmonkey.cc‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
#include <js/Class.h>
3535
#include <js/Date.h>
3636
#include <js/Initialization.h>
37+
#include <js/Modules.h>
3738
#include <js/Object.h>
3839
#include <js/Proxy.h>
3940
#include <js/SourceText.h>
@@ -85,6 +86,25 @@ void nurseryCollectionCallback(JSContext *cx, JS::GCNurseryProgress progress, JS
8586
}
8687
}
8788

89+
// pythonmonkey doesn't implement module loading, so `import(...)` must be
90+
// rejected rather than left unhandled. HostLoadImportedModule
91+
// (js/src/vm/Modules.cpp) only auto-finishes the promise on this path when a
92+
// hook IS registered but returns false; when no hook is registered at all it
93+
// reports "Module load hook not set" and returns without ever settling the
94+
// promise or clearing that pending exception -- leaving `await import(...)`
95+
// hung forever and a stale exception on the context. Registering this hook
96+
// (even though it never resolves anything) makes pythonmonkey responsible
97+
// for finishing the promise itself, as every embedder that permits dynamic
98+
// import syntax at all is required to be.
99+
static bool pythonmonkeyModuleLoadHook(
100+
JSContext *cx, JS::Handle<JSScript *> referrer, JS::Handle<JSObject *> moduleRequest,
101+
JS::Handle<JS::Value> hostDefined, JS::Handle<JS::Value> payload,
102+
uint32_t lineNumber, JS::ColumnNumberOneOrigin columnNumber
103+
) {
104+
JS_ReportErrorASCII(cx, "Dynamic module import is disabled or not supported in this context");
105+
return false;
106+
}
107+
88108
bool functionRegistryCallback(JSContext *cx, unsigned int argc, JS::Value *vp) {
89109
JS::CallArgs callargs = JS::CallArgsFromVp(argc, vp);
90110
Py_DECREF((PyObject *)callargs[0].toPrivate());
@@ -588,6 +608,8 @@ PyMODINIT_FUNC PyInit_pythonmonkey(void)
588608
return NULL;
589609
}
590610

611+
JS::SetModuleLoadHook(JS_GetRuntime(GLOBAL_CX), pythonmonkeyModuleLoadHook);
612+
591613
if (!JS::InitSelfHostedCode(GLOBAL_CX)) {
592614
PyErr_SetString(SpiderMonkeyError, "Spidermonkey could not initialize self-hosted code.");
593615
return NULL;

0 commit comments

Comments
 (0)