Bug 238291 – [WebAssembly][Modules] Support Wasm module import from a JS Worker module

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Asumu Takikawa]

A previous patch (https://bugs.webkit.org/show_bug.cgi?id=236268) added support for loading Wasm module scripts in general in WebCore, but they are not yet supported in Worker scripts.

This bug is for part of the Worker support: allowing imports of Wasm modules from inside a JS module Worker. The other aspect that's needed (not in this bug) is allowing Wasm scripts for Worker.new.

[Asumu Takikawa]

Created attachment 455591 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Yusuke Suzuki]

Comment on attachment 455591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=455591&action=review

The patch overall looks great! Some comments.

> Source/WebCore/ChangeLog:16
> +        Instead of ignoring all other tasks, the internal timer task and
> +        tasks used for WorkerEventLoop are allowed in order to allow
> +        JS VM microtasks to be queued and run.

Could you explain a bit more detail about this?

1. What problem happens (in which code)
2. Why does it happen?
3. What is the solution to that?

> Source/WebCore/workers/WorkerEventLoop.cpp:77
> +const String WorkerEventLoop::taskMode()

`const` is not necessary as it is copying value.

> Source/WebCore/workers/WorkerRunLoop.cpp:75
> +    const String mode() const

Return `const String&`.

> Source/WebCore/workers/WorkerRunLoop.cpp:174
> +    JSC::JSRunLoopTimer::TimerNotificationCallback timerAddedTask = createSharedTask<JSC::JSRunLoopTimer::TimerNotificationType>([this, predicateMode] {

Let's pass via WTFMove(predicateMode) instead of copying.

[Asumu Takikawa]

Created attachment 455694 [details]
Patch

[Asumu Takikawa]

Thanks for the comments! I forgot to add a file (`WebAssemblyScriptBufferSourceProvider.h`) previously so I updated that. I'll address your other smaller comments in the next update.

> Could you explain a bit more detail about this?
>
> 1. What problem happens (in which code)
> 2. Why does it happen?
> 3. What is the solution to that?

Sure, I've updated the ChangeLog entry to have more information now and I'll explain more below.

1. The problem is that Wasm module loading gets stuck and doesn't make progress, causing tests to time out. The reason is that Wasm relies on asynchronous operations to finish module loading, and the callbacks for these are not run. This means the code ends up in `WorkerOrWorkletScriptController::loadModuleSynchronously` forever.

2. This happens because the Worker run loop, when run in a non-default mode like for module loading, never times out when looking for tasks. It also does not trigger the `timerAddedTask` that it installs as a callback for the JS VM's timer set notification. Because a non-default Worker run loop also does not run any WorkerEventLoop tasks (such as from WorkerEventLoop::scheduleToRun), microtasks from the VM for things like the Wasm module loading and promise resolution don't get run.

3. This patch solves this by allowing both the timerAddedTask (for timer set notification) and WorkerEventLoop tasks to get run by non-default run loops like for module loading.

For (3), I'm not sure if there are better solutions for this, but I'd be open to suggestions if there are. For example, maybe it's better to set a non-infinite timeout for all run loops (though I think I tried this and it wasn't sufficient). I'm also not sure if the debugging mode should perhaps be treated differently (maybe it shouldn't ever run WorkerEventLoop tasks?).

[Asumu Takikawa]

Created attachment 456045 [details]
Patch

[Asumu Takikawa]

I've just refined this patch to solve the problem in IMO a better way.

Instead of modifying WorkerEventLoop.cpp at all, this adds a new parameter called `useTimeout` to WorkerRunLoop's `runInMode`. It defaults to false, but when it is true, the run loop will trigger timer-based timeouts even in a non-default run mode (such as for module loading).

Then in the use of the run loop for Worker module loading, we set the parameter to true and schedule a timer so that eventually the run loop will time out and run JS VM microtasks if needed.

This should also fix the test failure that came up in the previous patch with `LayoutTests/http/wpt/resource-timing/rt-initiatorType.worker.html`.

[Yusuke Suzuki]

(In reply to Asumu Takikawa from comment #7)
> I've just refined this patch to solve the problem in IMO a better way.
> 
> Instead of modifying WorkerEventLoop.cpp at all, this adds a new parameter
> called `useTimeout` to WorkerRunLoop's `runInMode`. It defaults to false,
> but when it is true, the run loop will trigger timer-based timeouts even in
> a non-default run mode (such as for module loading).
> 
> Then in the use of the run loop for Worker module loading, we set the
> parameter to true and schedule a timer so that eventually the run loop will
> time out and run JS VM microtasks if needed.
> 
> This should also fix the test failure that came up in the previous patch
> with `LayoutTests/http/wpt/resource-timing/rt-initiatorType.worker.html`.

Hmmm, I think probably the original approach would be simpler because it requires 20ms delay to run timer-scheduled tasks. If we are loading content from memory-cache, then I think 20ms is slow.

[Asumu Takikawa]

> Hmmm, I think probably the original approach would be simpler because it requires 20ms delay to run timer-scheduled tasks. If we are loading content from memory-cache, then I think 20ms is slow.

That's fair. I could make the timer period shorter as well, it was set somewhat arbitrarily (e.g., a 5ms timer also passes the tests). I also wonder if the timer should be reset after the WorkerRunLoop runs, so that it has an opportunity to trigger each time (the timer period could also back off after each loop).

I could try to make the original approach work, but it seemed to cause an ordering issue in `LayoutTests/http/wpt/resource-timing/rt-initiatorType.worker.html`. Not sure why though.

[Radar WebKit Bug Importer]

<rdar://problem/91067489>

[Asumu Takikawa]

Created attachment 456183 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 456183 [details]
Patch

Discussed with Asumu on Slack. We should change WorkerRunLoop to drive it while loading correctly instead.

[Asumu Takikawa]

Created attachment 457305 [details]
Patch

[Asumu Takikawa]

I switched this patch back to the original approach where WorkerEventLoop tasks are treated specially. This time, I added a default-false bool argument for WorkerRunLoop::runMode that controls whether the WorkerEventLoop tasks should run in the mode (and set it for worker module loading run loops).

This should fix the regression the patch had earlier, since typical worker run loops will have the flag set to false.

[Yusuke Suzuki]

Comment on attachment 457305 [details]
Patch

r=me

[EWS]

Committed r292799 (249581@main): <https://commits.webkit.org/249581@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457305 [details].