Pump V8's foreground task queue so async WebAssembly settles - #232
Conversation
AppRuntime_V8 creates a default v8::Platform but nothing ever called v8::platform::PumpMessageLoop. V8 finishes asynchronous WebAssembly compilation on a background thread and then posts a *foreground* task to settle the promise on the isolate thread, so WebAssembly.compile, instantiate and instantiateStreaming never resolved or rejected - any Emscripten module hung forever. Sync `new WebAssembly.Module` was unaffected, which made this look like a hang rather than a failure. AppRuntime_JSI already did the equivalent via TaskRunnerAdapter; the direct-V8 path was simply missing it. DrainMicrotasks now pumps the queue with kDoNotWait, so it never blocks the JavaScript thread. Because that only runs after a dispatched callback, the platform is also wrapped in a DispatchingPlatform whose foreground task runner nudges the app dispatcher, giving a pump when the app is otherwise idle (no render loop, no timers). The wrapper leaves the default platform owning the queue so task ordering, nestability and delays keep V8's own semantics, and the wake is coalesced through an atomic flag so a burst of posted tasks cannot flood the dispatcher. The three new tests time out without this change.
There was a problem hiding this comment.
Pull request overview
This PR fixes a hang in the direct-V8 runtime path where V8 foreground tasks (notably those used to settle async WebAssembly compilation promises) were never pumped, preventing WebAssembly.compile/instantiate/*Streaming from resolving or rejecting.
Changes:
- Wraps V8’s default platform with a
DispatchingPlatform/WakingTaskRunnerthat nudges the app dispatcher when foreground tasks are posted. - Updates
AppRuntime::DrainMicrotasks(V8) to pump V8’s foreground task queue usingv8::platform::PumpMessageLoop(..., kDoNotWait). - Adds unit tests covering async WebAssembly compile/instantiate settling and rejection behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Tests/UnitTests/Scripts/tests.ts | Adds WebAssembly async promise-settling and rejection tests to prevent regressions. |
| Core/AppRuntime/Source/AppRuntime_V8.cpp | Introduces a dispatch-waking V8 platform wrapper and pumps V8’s foreground queue during DrainMicrotasks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
c2f542c to
fda5f78
Compare
|
CI found two things my local V8 build could not, both fixed in fda5f78. 1. Android build break. Android builds against V8 11.0, desktop against 11.9. Rather than guess version thresholds I pulled the Android package and read its 2. The WebAssembly tests ran on every engine. The fix is V8-only, so JSC/Chakra/Hermes/QuickJS did not fail -- they hung, three times 30s apiece. Those engines' runtimes have the same class of gap; fixing them is a separate change. I added a
Worth noting the earlier green runs on this PR were queue artifacts -- this was the first run that actually built Android, so the breakage was there from the start rather than introduced by the last push. |
CI caught two problems the local V8 build could not. Android builds against V8 11.0 while desktop uses 11.9, and DispatchingPlatform forwarded the whole v8::Platform interface, including five members that do not exist in 11.0 (ThreadIsolatedAllocator, CreateBlockingScope, CurrentClockTimeMilliseconds, its high-resolution variant, and the TaskPriority overload of GetForegroundTaskRunner). Overriding a method the base class does not declare is a hard error, so those are now version-gated and the two GetForegroundTaskRunner overloads share a helper. All nine of 11.0's pure virtuals are still overridden; the gated-out members fall back to v8::Platform's own defaults, which are benign for each of them. The WebAssembly tests also ran on every engine, but the fix is V8-only, so JSC/Chakra/Hermes/QuickJS hit three 30s timeouts instead of failing fast. Expose the configured engine as a hostEngine global, mirroring hostPlatform, and skip the suite off V8. Chakra: 217 passing, 3 pending, 11.7s (was ~90s of timeouts). V8: 220 passing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
fda5f78 to
4a5d722
Compare
|
Follow-up: that push missed a second definition site, now folded in (the commit above is amended to Android does not build the One thing worth flagging while I was in there: |
Replace the wake-and-pump bridge with a foreground task runner that posts v8::Task::Run onto AppRuntime's dispatcher, matching Chromium's V8ForegroundTaskRunner model. Delayed posts reuse a native DeadlineScheduler extracted from TimeoutDispatcher so setTimeout and V8 delayed tasks share one queue. Also name the unit-test compile definition NAPI_JAVASCRIPT_ENGINE to match JSRUNTIMEHOST_PLATFORM. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da74bc94-a7dc-4817-bd81-59b5c6b123fc
Round absolute V8 execution times upward to microsecond precision and retire completed delayed task records through shared callback state. Deactivate retained runners before stopping timers and disposing the isolate. Move DelayedTaskScheduler into Foundation, restore JsRuntime's original API, and register AppRuntime's scheduler with the environment for the scheduling polyfill to borrow. Keep an owned fallback for standalone hosts and guard timeout callbacks independently of dispatcher lifetime. Extract the V8 platform adapter and foreground runner, replace implicit host clearing with explicit registration, and add timing, completion, cancellation, ownership, and shutdown regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 54aaa1e1-b4b3-46e7-9de4-5a56add4ac42
Include napi/env.h before V8 declarations so Android's -Werror build retains the established third-party warning guards and V8 configuration. Reproduced the unused-parameter failure and verified all three V8 sources with Android NDK Clang, without disabling project warnings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 54aaa1e1-b4b3-46e7-9de4-5a56add4ac42
|
CI follow-up fixed in 3014935: the extracted V8 headers included v8-platform.h directly, bypassing the existing napi/env.h warning guards. Android then treated V8 11.0's unused parameters as errors. Both headers now use the existing wrapper; project warning settings are unchanged. Reproduced the failure with Android NDK Clang and verified all three V8 implementation files compile with -Werror after the fix. Desktop V8's eight runner regressions and the JavaScript suite also pass. The five implemented review threads are resolved, and @bghgary's review has been re-requested. Monitoring the new CI run. |
There was a problem hiding this comment.
🟡 Changes recommended
Both scheduler ID generators can invoke undefined signed-overflow behavior instead of safely wrapping.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 23/23 changed files
- Comments generated: 2
- Review effort level: Balanced
Use a shared constexpr increment helper that checks INT32_MAX before adding and wraps directly to 1. Preserve both generators' live-ID collision checks. Cover initial allocation, INT32_MAX - 1, INT32_MAX, and post-wrap values, including a constant-expression assertion that rejects signed overflow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 54aaa1e1-b4b3-46e7-9de4-5a56add4ac42
Replace the one-off Run shutdown callback with an engine-selected ShutdownEnvironment hook. V8 unregisters foreground routing there; the other engines provide no-op implementations. Keep delayed scheduling and environment association in one internal Foundation abstraction, exposed through FoundationInternal. Use a dedicated Babylon global property instead of depending on JsRuntime's _native object. Inline overflow-safe ID wrapping in each allocator and replace the shared helper tests with boundary coverage through Schedule and Dispatch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 54aaa1e1-b4b3-46e7-9de4-5a56add4ac42
Make SetForJavaScript, ClearFromJavaScript, and GetFromJavaScript static. Update AppRuntime, Scheduling, and the environment-association regression to use the consistent names without changing scheduler ownership. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 54aaa1e1-b4b3-46e7-9de4-5a56add4ac42
Updates the JsRuntimeHost pin from `a6b98eaa1a9887b35adceed21f8c7d44c4f38e43` to `f47991dd5729fac1d6d477e66ed8a843da1d4832`, the latest upstream `main`. Includes BabylonJS/JsRuntimeHost#232: - Dispatches V8 foreground tasks through AppRuntime so asynchronous WebAssembly compilation promises settle instead of hanging. - Shares the delayed-task scheduler between V8 foreground work and the scheduling polyfill, with explicit ownership and shutdown handling. This is a one-line dependency-pin update based on current Babylon Native `master`. The standard-stream logger follow-up, BabylonJS/JsRuntimeHost#236, is still open and is not included in this revision. Co-authored-by: Branimir Karadzic <branimirkaradzic@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
V8 finishes asynchronous WebAssembly compilation on a background thread and then posts a foreground task to settle the promise on the isolate thread.
AppRuntime_JSIalready ran those viaTaskRunnerAdapter; the direct-V8 path dropped them, so asynchronous WebAssembly promises could hang. Synchronousnew WebAssembly.Modulewas unaffected.Foreground posts now go through
V8ForegroundTaskRunner, which dispatchesv8::Task::Runthrough AppRuntime. Delayed posts useDelayedTaskScheduler, extracted into Foundation from the timer queue behindsetTimeout/setInterval. AppRuntime registers its scheduler with the environment for the polyfill to borrow; standaloneJsRuntimeinstances retain their original API and do not acquire timer ownership.TimeoutDispatcherowns a fallback scheduler when no host scheduler is registered. Interval rearming remains on the JavaScript thread after the callback.Delayed V8 posts round their absolute requested time upward to microsecond precision, and completed pending records are removed even when completion precedes
Schedulereturning. Shared callback state and explicit host unregistration guard teardown independently of retained task-runner references.V8Platformand the foreground runner are in dedicated files; the wake-and-pump bridge andPumpMessageLoopare gone.Added asynchronous WebAssembly promise-settlement tests plus native timing, pending-record, cancellation, scheduler-ownership, and shutdown regressions.
Validation for 7192ac0 (Windows Release): V8 11.9 passed 17 selected native tests, including the JavaScript suite with 228 passing; Chakra passed 9 selected native tests, including 225 JavaScript tests with the 3 V8-only tests skipped. The 16 native timer/lifetime tests also passed 25 repeated iterations. The V8 implementation files syntax-compile against the actual Android V8 11.0 headers; Android device execution remains for CI.