Trim comments to be concise and focus on why, not what

The previous two commits' inline comments read like a debugging journal
(narrating what was tried, what failed, "confirmed via testing" for
nearly every line) rather than code documentation. Rewrote them to state
the non-obvious reasoning briefly and let the code show the what -
readers can see the diff and the SpiderMonkey headers for themselves.

No behavior change; rebuilt and reran the async/Promise regression tests
to confirm.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: dan-distributive

CMakeLists.txt

@@ -30,20 +30,10 @@ if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME)
   include(FetchContent)
 
   if (WIN32)
-    # LOCAL PATCH: pythonmonkey's own compile of its .cc files (and of
-    # SpiderMonkey's public headers pulled in by them) goes through this
-    # CMake/clang-cl build directly, not through Mozilla's own moz.build
-    # system -- which normally defines XP_WIN for every object file it
-    # compiles. Without it, SpiderMonkey headers that branch on
-    # `defined(XP_WIN)` (assuming it's always set on Windows, since
-    # that's Mozilla's own standard "we're building for Windows" macro)
-    # silently fall through to their POSIX/pthread branch instead,
-    # confirmed via a real build failure (PlatformMutex.h including the
-    # nonexistent <pthread.h>, UniquePtrExtensions.h missing Windows
-    # HANDLE-based types as a result). Defining it globally here fixes
-    # every such header at once, rather than patching each one
-    # individually as it's discovered (one already was, in
-    # BaseProfilerUtils.h, before this more general fix existed).
+    # 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")

include/JobQueue.hh

@@ -53,40 +53,26 @@ bool getHostDefinedData(JSContext *cx, JS::MutableHandle<JSObject *> incumbentGl
 
 /**
  * @brief Ask the embedding for the host defined global to use when running
- * a JS microtask (LOCAL PATCH: new pure-virtual method added alongside the
- * SpiderMonkey 157a1 JobQueue redesign -- see runJobs() below for context).
+ * a JS microtask.
  *
- * Mirrors the "we don't track this" stance already taken in
- * getHostDefinedData() above: we have no host defined global of our own, so
- * SpiderMonkey falls back to its own default (the microtask's execution
- * global, from GetExecutionGlobalFromJSMicroTask). Matches SpiderMonkey's
- * own reference embedding, InternalJobQueue::getHostDefinedGlobal, which
- * does exactly this (js/src/vm/JSContext.cpp).
+ * 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 getHostDefinedGlobal(JSContext *cx, JS::MutableHandle<JSObject *> out) const override;
 
 /**
  * @brief Pull every job SpiderMonkey has queued internally since the last
  * call, and forward each one to the Python event-loop for execution.
  *
- * LOCAL PATCH (SpiderMonkey 157a1 API change): `JobQueue::enqueuePromiseJob`
- * -- the old per-job push callback this class used to override -- was
- * removed from the base class entirely. SpiderMonkey now enqueues promise
- * reaction jobs into its own internal queue as it creates them (see
- * EnqueueJob() in js/src/builtin/Promise.cpp), without notifying the
- * embedding. The embedding is instead expected to pull queued jobs itself,
- * here, whenever it wants a "microtask checkpoint" to happen -- triggered
- * by the embedder calling the free function js::RunJobs(cx) (declared in
- * jsfriendapi.h; NOT the same thing as this method, despite the identical
- * name -- js::RunJobs(cx) is what calls cx->jobQueue->runJobs(cx), i.e.
- * this override). PythonMonkey calls js::RunJobs(GLOBAL_CX) once after each
- * top-level JS_ExecuteScript() call, in pythonmonkey.cc.
- *
- * This preserves the original behaviour -- JS promise reactions execute as
- * Python asyncio callbacks, not synchronously inline -- by draining
- * SpiderMonkey's internal queue and re-creating the same "hand this job to
- * Python's event loop" forwarding enqueuePromiseJob used to do per-job, just
- * done here in a pull/batch fashion instead.
+ * 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,
@@ -138,15 +124,8 @@ js::UniquePtr<JS::JobQueue::SavedJobQueue> saveJobQueue(JSContext *) override;
  *          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
  *
- * LOCAL PATCH (SpiderMonkey 157a1 API change): `JS::InitDispatchToEventLoop`
- * (2-callback init) was replaced by `JS::InitAsyncTaskCallbacks`, which now
- * mandates both a `DispatchToEventLoopCallback` AND a
- * `DelayedDispatchToEventLoopCallback` (see delayedDispatchToEventLoop()
- * below). The callback signature itself also changed: it now takes ownership
- * of the Dispatchable via `js::UniquePtr<Dispatchable>&&` instead of a raw
- * pointer, and `Dispatchable::run()` is now `protected` -- callers must go
- * through the new public static `Dispatchable::Run(cx, task, shuttingDown)`
- * instead of calling `->run()` directly.
+ * 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 - the Dispatchable to be called; ownership transferred to this callback
@@ -156,27 +135,15 @@ static bool dispatchToEventLoop(void *closure, js::UniquePtr<JS::Dispatchable> &
 
 /**
  * @brief The callback for dispatching an off-thread promise to the event
- * loop after a delay (LOCAL PATCH: newly mandatory as of the same API
- * change described on dispatchToEventLoop() above -- previously this
- * concept didn't need to exist as a separate callback for this embedding).
+ * loop after a delay.
  *
- * NEEDS REVIEW: this embedding has no cross-thread-safe delayed-dispatch
- * mechanism (PyEventLoop::enqueueWithDelay exists but calls
- * asyncio.loop.call_later, which -- unlike call_soon_threadsafe, used
- * elsewhere in this codebase -- is not documented as safe to call from a
- * thread other than the one running the loop; this callback, per its
- * declaration in js/public/Promise.h, must be safe to call from ANY
- * thread). Per that same header's documented contract ("If a timeout
- * manager is not available for given context, it should return false"),
- * this always returns false, i.e. this embedding declines to service
- * engine-level delayed dispatch. This should only affect internal
- * SpiderMonkey features that specifically need a delayed off-thread
- * callback (e.g. an Atomics.waitAsync timeout) -- ordinary JS
- * `setTimeout`/`setInterval` in pythonmonkey go through a separate,
- * already-working path (PyEventLoop::enqueueWithDelay called from JS-exposed
- * timer functions, not this SpiderMonkey-internal callback) and are
- * unaffected. Not verified against a real Atomics.waitAsync-with-timeout
- * test case.
+ * 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

setup.sh

@@ -26,14 +26,8 @@ else
   exit 1
 fi
 # Install rust compiler
-# LOCAL PATCH: like the Poetry skip below, this step was unconditional --
-# no check for whether rust/the 1.85 toolchain is already installed. On a
-# machine where it already is, re-running rustup-init.sh downloads a fresh
-# installer exe into a temp dir and executes it, which on this Windows
-# machine gets blocked ("Permission denied", almost certainly Defender/
-# SmartScreen refusing to run a newly-downloaded, unsigned exe straight out
-# of a temp directory) -- a real, reproducible failure, not a flake. Skip
-# the whole block if rustup + the 1.85 toolchain are already present.
+# 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\.85'; then
   echo "Rust 1.85 toolchain already installed, skipping rustup-init"
 else
@@ -52,15 +46,9 @@ if [[ "$OSTYPE" == "msys"* || "$OSTYPE" == "cygwin"* ]]; then # Windows
 else
   POETRY_BIN="$HOME/.local/bin/poetry"
 fi
-# LOCAL PATCH: like the rustup step above, made idempotent (skip if already
-# installed) rather than always re-running the installer. Also, this
-# machine has no `python3` on PATH (only `python`), which made the real
-# installer command (`python3 - --version ...`) fail outright -- confirmed
-# via a real failure, not speculative. Poetry itself is still needed: the
-# `.git/hooks/pre-commit` dev-tooling branch further down calls
-# `$POETRY_BIN run pip install autopep8`, so skipping this setup entirely
-# (an earlier version of this patch did) would silently break that branch
-# for anyone whose clone takes it.
+# 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
@@ -74,19 +62,10 @@ 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)
-# LOCAL PATCH: this download+extract is not idempotent as originally
-# written -- it always re-extracts and always re-`mv`s, which fails once
-# firefox-source already exists from a prior (possibly failed-later) run.
-# Since this script needs re-running whenever a later step fails (and we've
-# hit several unrelated Windows-environment issues after this point), skip
-# entirely once firefox-source is already present.
+# 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
-  # LOCAL PATCH: wget.exe (MSYS2's, and presumably any other copy) is
-  # blocked outright on this machine by a Windows Defender Application
-  # Control policy ("An Application Control policy has blocked this
-  # file" -- confirmed directly, not a PATH/permission-bits issue).
-  # curl is unaffected (checked both Windows' own and MSYS2's) -- use it
-  # instead. unzip is also unaffected, kept as-is.
+  # 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
@@ -111,7 +90,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 # LOCAL PATCH: this VS-project-file-generation convenience feature (not needed for a command-line-only build) does an unguarded os.environ["MOZILLABUILD"] lookup and crashes with KeyError when it's unset, which it is here (we don't use the official Mozilla Build package) -- confirmed via a real build failure, not speculative
+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
@@ -126,16 +105,8 @@ mkdir -p ../../../../_spidermonkey_install/
   --disable-tests \
   $(if [[ "$OSTYPE" == "darwin"* ]]; then echo "--enable-linker=ld64"; fi) \
   --enable-optimize
-# LOCAL PATCH: the original --disable-explicit-resource-management flag
-# (worked around Bugzilla 1940342, a header/lib enum mismatch from when
-# the `using` syntax was newly landing in nightly circa early 2025) is
-# now an unrecognized configure option on this newer mozilla-central
-# snapshot -- confirmed via a real `InvalidOptionError: Unknown option`
-# build failure. The explicit-resource-management feature has evidently
-# shipped/stabilized since, taking the flag (and presumably the bug it
-# worked around) with it. Removed rather than guessing at a replacement
-# flag; if header/lib enum mismatches resurface, that bug tracker is the
-# place to check first.
+# --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"
 

src/BufferType.cc

@@ -94,28 +94,14 @@ PyObject *BufferType::fromJsTypedArray(JSContext *cx, JS::HandleObject typedArra
     return nullptr;
   }
 
-  // LOCAL PATCH (SpiderMonkey 157a1 API change, needs team review -- see
-  // handover doc): JS_GetArrayBufferViewFixedData was removed upstream;
-  // JS_GetArrayBufferViewData is its replacement, but trades the old
-  // function's own "return nullptr if the data is still inline/movable"
-  // runtime guard for a caller-supplied JS::AutoRequireNoGC token instead.
-  // AutoRequireNoGC (js/GCAPI.h) is a trivial marker type with no runtime
-  // behaviour of its own -- it's a compile-time "I've verified this is
-  // safe" token, not an active GC suppressor. The safety property the old
-  // function's guard provided (never returning a pointer into GC-movable
-  // inline TypedArray storage) is still expected to hold here because of
-  // the JS_GetArrayBufferViewBuffer() call above: per ITS OWN comment, it
-  // forces any inline/movable data to be promoted to a real, stably
-  // allocated ArrayBuffer first. This reasoning has NOT been independently
-  // verified against SpiderMonkey's actual GC internals (e.g. by stress
-  // testing with --enable-gczeal / a compacting-GC configuration) -- do
-  // that before trusting this for anything beyond experimentation.
-  // AutoRequireNoGC's own ctor/dtor are protected (it's a base marker type,
-  // not directly instantiable) -- use AutoAssertNoGC instead, which is
-  // publicly constructible AND (in diagnostic builds) actually verifies at
-  // runtime that no GC happens while it's alive, rather than being a pure
-  // no-op marker. Strictly better for confidence in this fix than the bare
-  // base class would have been even if it were public.
+  // 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<uint8_t *>(JS_GetArrayBufferViewData(typedArray, &isSharedMemory2, nogc));

src/JSFunctionProxy.cc

@@ -60,18 +60,9 @@ PyObject *JSFunctionProxyMethodDefinitions::JSFunctionProxy_call(PyObject *self,
     return NULL;
   }
 
-  // LOCAL PATCH (SpiderMonkey 157a1 JobQueue redesign, found via retesting
-  // the claims in SPIDERMONKEY_VERSION_BUMP.md -- fix #7's checkpoint list
-  // missed this site): this is the generic entry point Python uses to call
-  // back into any JS function it was handed -- e.g. a `setTimeout` callback
-  // dispatched from PyEventLoop, or a JS event-listener invoked directly by
-  // Python code. 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 into cx->microTaskQueues
-  // with nothing else scheduled to drain it -- this call happens outside of
-  // JS_ExecuteScript() and outside PromiseType.cc's two checkpoints. Confirmed
-  // via a real hang: awaiting a JS Promise that resolves via `setTimeout`
-  // never returned until this checkpoint was added here.
+  // 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()) {

src/JSMethodProxy.cc

@@ -71,11 +71,7 @@ PyObject *JSMethodProxyMethodDefinitions::JSMethodProxy_call(PyObject *self, PyO
     return NULL;
   }
 
-  // LOCAL PATCH (SpiderMonkey 157a1 JobQueue redesign): same missing
-  // checkpoint as JSFunctionProxy_call in JSFunctionProxy.cc -- see the
-  // comment there for the full explanation and the real hang that surfaced
-  // it. This is the same "Python calls back into a JS callable" bridge, just
-  // for bound methods instead of plain functions.
+  // Same checkpoint as JSFunctionProxy_call, for bound methods.
   js::RunJobs(cx);
 
   if (PyErr_Occurred()) {