Repository navigation
Conversation
Set the host cleanup callback, run queued cleanups after each microtask checkpoint, and drain the queue after the pre-initialization GC so the snapshot never holds queued cleanups. Fixes bytecodealliance#327.
| // not baked into the snapshot and run on its first resumption. | ||
| if (!core::EventLoop::drain_finalization_registry_cleanups(this)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I think here, it's possible that those cleanups will trigger tla rejection, so it needs to be checked again? Otherwise we are risking snapshoting a failing module.
https://github.com/bytecodealliance/StarlingMonkey/blob/main/runtime/engine.cpp#L604C1-L612C4
| using CleanupFunctions = JS::GCVector<JSFunction *, 0, js::SystemAllocPolicy>; | ||
| static PersistentRooted<CleanupFunctions> finalization_registry_cleanups; | ||
|
|
||
| // Called during a collection, so it must not collect: it only appends to the queue. If the |
There was a problem hiding this comment.
It's not clear what collection this comment refers to. Could we change it to something like: Called during GC, so this must not trigger another collection...?
| static void queue_finalization_registry_cleanup(JSFunction *do_cleanup, | ||
| JSObject * /*incumbent_global*/, | ||
| void * /*data*/) { | ||
| (void)finalization_registry_cleanups.get().append(do_cleanup); |
There was a problem hiding this comment.
Why we are not following here SpiderMonkey's shell exactly and treat append failure as fatal? Ignoring oom here doesn't feel right and IIUC append can only fail on allocation failure?
andreiltd
left a comment
andreiltd
left a comment
There was a problem hiding this comment.
Hey, thank you for the patch! I left few comments but overall this looks really good!
This branch has not been deployed
2 participants
Fixes #327.
Problem
StarlingMonkey never called
JS::SetHostCleanupFinalizationRegistryCallback. SpiderMonkey therefore queued the records of deadFinalizationRegistrytargets and nothing ever ran them, so noFinalizationRegistrycallback was called, however many collections ran. Code that frees a resource from such a callback (for example the resource handle drops in ComponentizeJS-generated bindings) never did so on a reused instance.Change
EventLoop::initsets the host cleanup callback. It only appends thedoCleanupfunction SpiderMonkey hands it to aPersistentRooted<GCVector<JSFunction *>>, because it is called during a collection and must not collect.EventLoop::run_event_loopruns the queued cleanups after each microtask checkpoint, as the SpiderMonkey shell does and as HTML's HostEnqueueFinalizationRegistryCleanupJob describes. The queue is swapped out first, each function is called in its own realm, and an exception is reported and cleared rather than propagated. If any cleanup ran, the loop goes back round for the microtasks they queued.Engine::eval_toplevel(only whileScriptPreInitializing), the fullNonIncrementalGCthat runs before the Wizer snapshot can queue cleanups. After it,EventLoop::drain_finalization_registry_cleanupsruns the queued cleanups and a microtask checkpoint, repeating until the queue is empty, so the snapshot holds an empty queue and nothing queued at build time runs on the first call. The loop is bounded at 16 rounds (MAX_CLEANUP_DRAIN_ROUNDS). A round only repeats if a cleanup itself caused a collection that queued more, and SpiderMonkey offers each registry once per collection. If the bound were ever reached, the remainder stays queued and runs in the first event loop after the snapshot is resumed, which is still correct behaviour for aFinalizationRegistry.Tests
Two new e2e tests (
tests/tests.cmake):finalization-registry: the reproduction from the issue. A handler registers a target, allocates 2 MiB buffers, yielding to the event loop between them, and answersfinalizedonce the callback has run. Fails without the change.finalization-registry-snapshot: registers a dead target at the top level (inside a function, so no top-level frame keeps it alive). The handler answers synchronously with the count, before the first event loop of the resumed instance, so it reports1only if the cleanup ran during pre-initialization. Fails without the drain (reports0) and passes with it.Testing done
Built
starling-raw.wasm(Release) and ran alle2e-*andintegration-*ctest targets: all pass. Both new tests were also run against a build without the change (and the snapshot test against a build with the event loop change but no drain) and fail as described. Built against a locally available SpiderMonkeyFIREFOX_147_0_4_RELEASE_STARLINGbuild, not the release tarball.clang-formatwas not available locally, so formatting was checked by hand against.clang-format(2 spaces, 100 columns).The tests were run on main at 6c86477; this branch is on ec69e70, which only adds the WPT tag update (#326) on top.