Skip to content

Pump V8's foreground task queue so async WebAssembly settles - #232

Open
bkaradzic-microsoft wants to merge 3 commits into
BabylonJS:mainfrom
bkaradzic-microsoft:v8-pump-foreground-tasks
Open

Pump V8's foreground task queue so async WebAssembly settles#232
bkaradzic-microsoft wants to merge 3 commits into
BabylonJS:mainfrom
bkaradzic-microsoft:v8-pump-foreground-tasks

Conversation

@bkaradzic-microsoft

Copy link
Copy Markdown
Member

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. With nothing draining that queue, WebAssembly.compile, instantiate and instantiateStreaming never resolved or rejected, so any Emscripten module hung forever. Synchronous 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. That only runs after a dispatched callback, so the platform is additionally wrapped in a DispatchingPlatform whose foreground task runner nudges the app dispatcher — giving a pump even 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; it only adds the wake-up. The wake is coalesced through an atomic flag so a burst of posted tasks cannot flood the dispatcher, and PumpMessageLoop is handed the inner platform because it downcasts to libplatform's DefaultPlatform.

All three new tests time out at 30s without this change and pass in ~10ms with it.

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.
Copilot AI lite review requested due to automatic review settings August 26, 2026 17:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/WakingTaskRunner that nudges the app dispatcher when foreground tasks are posted.
  • Updates AppRuntime::DrainMicrotasks (V8) to pump V8’s foreground task queue using v8::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.

Comment thread Core/AppRuntime/Source/AppRuntime_V8.cpp
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@bkaradzic-microsoft

Copy link
Copy Markdown
Member Author

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. DispatchingPlatform forwarded the entire v8::Platform interface, and five of those members simply do not exist in 11.0: GetThreadIsolatedAllocator, CreateBlockingScope, CurrentClockTimeMilliseconds, CurrentClockTimeMillisecondsHighResolution, and the TaskPriority overload of GetForegroundTaskRunner. Overriding a method the base class does not declare is a hard error, hence no type named 'ThreadIsolatedAllocator' in namespace 'v8' and friends.

Rather than guess version thresholds I pulled the Android package and read its v8-version.h (11.0.226.16) and v8-platform.h directly, so the gating matches the header rather than my memory of when each member landed. I also checked all nine of 11.0's pure virtuals are still overridden -- that is the part that would silently break the build again. Where a member is gated out we fall back to v8::Platform's own default, which is benign for each: a null allocator, a null blocking scope, and clock values derived from CurrentClockTimeMillis.

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 hostEngine global mirroring the existing hostPlatform and scoped the suite with a beforeEach skip. Verified both directions rather than just the happy path:

engine result
V8 220 passing, tests execute
Chakra 217 passing, 3 pending, 11.7s (was ~90s of timeouts)

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
@bkaradzic-microsoft

Copy link
Copy Markdown
Member Author

Follow-up: that push missed a second definition site, now folded in (the commit above is amended to 4a5d722).

Android does not build the UnitTests target -- it builds a separate UnitTestsJNI library from its own CMakeLists.txt, which compiles the same Shared.cpp. So adding JSRUNTIMEHOST_ENGINE only to UnitTests left the JNI target without it: error: use of undeclared identifier 'JSRUNTIMEHOST_ENGINE', which broke all three Android legs including JSC and QuickJS.

One thing worth flagging while I was in there: JSRUNTIMEHOST_PLATFORM is passed to the JNI target as -DJSRUNTIMEHOST_PLATFORM=\"\" -- empty. It is set as an ordinary variable inside the root project's scope, and add_subdirectory does not propagate that back up to the JNI CMakeLists.txt, so hostPlatform is "" on Android today. NAPI_JAVASCRIPT_ENGINE does not have this problem because gradle passes it on the CMake command line, making it a cache variable visible in every scope, so hostEngine resolves correctly. I have left the pre-existing hostPlatform quirk alone as it is unrelated to this PR, but it is a live bug for any test that gates on hostPlatform on Android.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants