Bug 173105

Summary: WebAssembly: add support for stream APIs
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: adamk, alaisterpattrick, annulen, ap, banti17997, benjamin, boyvaishnav, calvaris, cdumez, cemllyrios, cemlysoy22, chi187, clopez, cmarcelo, dave, donatellamiller162, dpaddock, dschuff, ews-watchlist, fpizlo, gskachkov, gyuyoung.kim, hotaru, jbreviewsonyt, jfbastien, joepeck, kbr, keith_miller, maksim.nesterenko, mark.lam, mereramaao, msaboff, priya.sharma001976, rajaali99330, rob, ryuan.choi, saam, sergio, singhdarsh28, steven, tzagallo, webkit-bug-importer, youennf, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=173892
https://bugs.webkit.org/show_bug.cgi?id=221025
Bug Depends on: 181600, 183442, 183443, 183444, 184888    
Bug Blocks: 161709    
Attachments:
Description Flags
Patch
none
WASM and CSP test cases.
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch keith_miller: review+

Description JF Bastien 2017-06-08 09:57:55 PDT
Post-MVP APIs were added: https://github.com/WebAssembly/design/blob/master/Web.md#additional-web-embedding-api

WebAssembly.compileStreaming
WebAssembly.instantiateStreaming
Comment 1 JF Bastien 2017-06-08 09:58:43 PDT
I think this is fairly low priority. When we do it we should make sure we actually stream-compile, not just download all and then start compilation.
Comment 2 JF Bastien 2017-06-29 10:58:47 PDT
When implementing this, don't forget to test CSP. I added a FIXME in 173892.

This will be trickier than current wasm CSP because the streaming APIs refer to URLs, not just array buffers.

Also consider what we do for SRI.
Comment 3 GSkachkov 2017-12-17 01:00:34 PST
Can I take this task? I will have free time so possible I'll manage implement this.
Comment 4 JF Bastien 2017-12-17 08:05:24 PST
Sure.
Comment 5 GSkachkov 2018-01-03 12:00:28 PST
Created attachment 330407 [details]
Patch

WiP. Link WebCore and JSC, and handle response from fetch. Next steps: Implement streaming parsing, run parsing in separate thread, CSP&SPI
Comment 6 JF Bastien 2018-02-20 16:10:38 PST
Comment on attachment 330407 [details]
Patch

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

A few comments to start.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:114
> +void WebAssemblyPrototype::webAssemblyModuleValidateAsync(ExecState* exec, Vector<uint8_t>&& source, JSPromiseDeferred* promise)

This and the internal one can just be file-level static functions.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:120
> +    RETURN_IF_EXCEPTION(throwScope, void());

I don't think this makes sense here?

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:156
> +    RETURN_IF_EXCEPTION(throwScope, JSValue::encode(jsUndefined()));

Ditto.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:170
> +            promise->reject(exec, createTypeError(exec, ASCIILiteral("CompileStreaming is not supported in jsc, only in browser environment.")));

I might be wrong, but I think compileStreaming can take a blob URL? In that case it probably should work in JSC. What do other streaming things do in JSC?

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:291
> +        } else {

Add a FIXME here: if there's an importObject and it contains a Memory, then we can compile the module with the right memory type (fast or not) by looking at the memory's type.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:296
> +                compileAndInstantiate(vm, exec, promise, firstArgument, importObject);

The above 4 lines seem wrong per spec: https://www.w3.org/TR/2018/WD-wasm-web-api-1-20180215/#streaming-modules

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:391
> +        promise->reject(exec, createTypeError(exec, ASCIILiteral("first argument must be an Response or Promise for Response #5")));

This doesn't seem like the right error message for an OOM condition.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:402
> +    // TODO: Implement CSP and SPI check before start consume content from Response

I think you mean SRI.
Comment 7 GSkachkov 2018-03-08 03:20:40 PST
Thanks for review!!!
During the implementation this feature, I see that is more complicated that I expected, so I split it into 3 tasks:
1) Just adding javascript API, but without streaming compilation: 
   * WebAssembly.compileStreaming
   * WebAssembly.instantiateStreaming
2) Streaming compilation, compile and verify as soon as wasm chunk is delivered over network
3) Implement allow to compile each function in separate thread.

So I hope it allow to decrease size and complexity of the patches
Comment 25 Rob 2019-05-09 16:52:58 PDT
It seems that development here has stalled. We would love to see this added, but the purpose of this comment is to verify that the stream APIs will properly support origins and SRI hashes in CSPs. The comments here seem to indicate that that is a priority, but it's not super clear.

Here is a proposal that discusses the appropriate behavior of these functions: https://github.com/WebAssembly/content-security-policy/blob/master/proposals/CSP.md#proposed-origin-bound-permission

Ideally SRI hashes would be supported by the other WebAssembly APIs as well, but getting the stream APIs right from the beginning would be a huge win.
Comment 26 Rob 2019-05-10 14:58:14 PDT
Created attachment 369602 [details]
WASM and CSP test cases.

Here is a set of tests and a summary of major browser behavior.
Comment 27 Adam Klein 2019-12-10 16:47:34 PST
Any updates here?
Comment 28 Radar WebKit Bug Importer 2019-12-15 02:59:19 PST
<rdar://problem/57945940>
Comment 29 Maksim Nesterenko 2020-08-18 02:25:36 PDT
Hello, is there any plans on adding this?
Comment 30 Yusuke Suzuki 2020-09-01 10:26:41 PDT
This is currently planned in my TODO tasks :)
Comment 31 Maksim Nesterenko 2020-09-02 00:11:52 PDT
Okay, I'm really looking forward to it :) 

Currently, it's a huge lack of support because without instantiateStreaming it's impossible to use wasm under environments with content security policies (where usually unsafe-eval is blocked).
Comment 32 Kenneth Russell 2020-12-23 11:21:08 PST
Eagerly looking forward to this too - Google Meet's background blur feature uses WebAssembly.instantiateStreaming, so adding this would get it one step closer to running in Safari!
Comment 33 styfle 2021-01-19 16:51:35 PST
Looking forward to this feature as well!

Currently, wasm-bindgen generates a lot of boilerplate to support Safari, which is the only browser that doesn't support WebAssembly.instantiateStreaming().


References:

https://github.com/rustwasm/wasm-bindgen

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/instantiateStreaming#browser_compatibility
Comment 34 Yusuke Suzuki 2021-01-25 14:00:57 PST
*** Bug 184888 has been marked as a duplicate of this bug. ***
Comment 35 Yusuke Suzuki 2021-01-25 14:02:23 PST
*** Bug 183444 has been marked as a duplicate of this bug. ***
Comment 36 Yusuke Suzuki 2021-01-25 14:02:31 PST
*** Bug 183443 has been marked as a duplicate of this bug. ***
Comment 37 Yusuke Suzuki 2021-01-25 14:02:41 PST
*** Bug 184886 has been marked as a duplicate of this bug. ***
Comment 38 Yusuke Suzuki 2021-01-25 14:03:09 PST
Created attachment 418334 [details]
Patch
Comment 39 Yusuke Suzuki 2021-01-25 22:54:10 PST
Created attachment 418374 [details]
Patch
Comment 40 Yusuke Suzuki 2021-01-26 01:19:52 PST
Created attachment 418377 [details]
Patch
Comment 41 Yusuke Suzuki 2021-01-26 01:49:54 PST
Created attachment 418382 [details]
Patch
Comment 42 Yusuke Suzuki 2021-01-26 03:40:58 PST
Created attachment 418388 [details]
Patch
Comment 43 EWS Watchlist 2021-01-26 03:42:01 PST
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
Comment 44 Yusuke Suzuki 2021-01-26 16:44:26 PST
Created attachment 418488 [details]
Patch
Comment 45 Yusuke Suzuki 2021-01-26 19:54:30 PST
Created attachment 418497 [details]
Patch
Comment 46 Yusuke Suzuki 2021-01-26 21:24:03 PST
Created attachment 418505 [details]
Patch
Comment 47 Yusuke Suzuki 2021-01-26 21:25:57 PST
Created attachment 418506 [details]
Patch
Comment 48 Yusuke Suzuki 2021-01-26 21:31:15 PST
Created attachment 418507 [details]
Patch
Comment 49 Yusuke Suzuki 2021-01-26 21:33:47 PST
Created attachment 418508 [details]
Patch
Comment 50 Yusuke Suzuki 2021-01-27 14:40:08 PST
Created attachment 418585 [details]
Patch
Comment 51 Keith Miller 2021-01-27 15:13:34 PST
Comment on attachment 418585 [details]
Patch

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

r=me with some comments.

> Source/JavaScriptCore/wasm/WasmStreamingCompiler.cpp:89
> +        ensureWorklist().enqueue(WTFMove(plan));

Wait, why does the ChangeLog say you don't use the WasmWorklist? is `ensureWorklist()` not the Wasm one?

> Source/JavaScriptCore/wasm/WasmStreamingCompiler.cpp:139
> +    auto* promise = std::exchange(m_promise, nullptr);

Why is this needed? Isn't the promise retained by the DeferredWorkTimer?

> Source/JavaScriptCore/wasm/WasmStreamingPlan.h:53
> +    // For some reason friendship doesn't extend to parent classes...

Heh, I read this and thought to myself... This doesn't seem like something Yusuke would right. Turns out I wrote it and you copied it here, lol

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:319
> +    bool isResponseCorsSameOrigin = inputResponse->type() == ResourceResponse::Type::Basic || inputResponse->type() == ResourceResponse::Type::Cors || inputResponse->type() == ResourceResponse::Type::Default;

I feel like this should be a member function on JSFetchResponse. It's a bit magical here but wouldn't need a comment if the condition of the if was `if (inputResponse->isCorsSameOrigin())`

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:326
> +    auto contentType = inputResponse->headers().fastGet(HTTPHeaderName::ContentType);

Ditto but less important.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:347
> +        inputResponse->consumeBodyReceivedByChunk([globalObject, compiler = WTFMove(compiler)](auto&& result) mutable {

Does this happen on the main thread out of curiosity or can it happen elsewhere?
Comment 52 Yusuke Suzuki 2021-01-27 15:54:10 PST
Comment on attachment 418585 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WasmStreamingCompiler.cpp:89
>> +        ensureWorklist().enqueue(WTFMove(plan));
> 
> Wait, why does the ChangeLog say you don't use the WasmWorklist? is `ensureWorklist()` not the Wasm one?

LLIntPlan is not using Worklist. Instead, we generate StreamingPlan one per one wasm function, and they are enqueued into worklist. I'll add more description in ChangeLog.

>> Source/JavaScriptCore/wasm/WasmStreamingCompiler.cpp:139
>> +    auto* promise = std::exchange(m_promise, nullptr);
> 
> Why is this needed? Isn't the promise retained by the DeferredWorkTimer?

Just make sure that m_promise is nullptr after scheduling deferredWorkTimer resolution. This also allows the destructor to ensure that scheduled task is resolved by checking `m_promise`.

>> Source/JavaScriptCore/wasm/WasmStreamingPlan.h:53
>> +    // For some reason friendship doesn't extend to parent classes...
> 
> Heh, I read this and thought to myself... This doesn't seem like something Yusuke would right. Turns out I wrote it and you copied it here, lol

Yeah, I copied it from OMGPlan since both shares the idea "compile one function at a time".

>> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:319
>> +    bool isResponseCorsSameOrigin = inputResponse->type() == ResourceResponse::Type::Basic || inputResponse->type() == ResourceResponse::Type::Cors || inputResponse->type() == ResourceResponse::Type::Default;
> 
> I feel like this should be a member function on JSFetchResponse. It's a bit magical here but wouldn't need a comment if the condition of the if was `if (inputResponse->isCorsSameOrigin())`

Sounds nice! Changed.

>> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:326
>> +    auto contentType = inputResponse->headers().fastGet(HTTPHeaderName::ContentType);
> 
> Ditto but less important.

Sounds nice, changed.

>> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:347
>> +        inputResponse->consumeBodyReceivedByChunk([globalObject, compiler = WTFMove(compiler)](auto&& result) mutable {
> 
> Does this happen on the main thread out of curiosity or can it happen elsewhere?

This is under the main thread. Invoked via didReceiveData callback from Fetch's BodyLoader.
Comment 53 Yusuke Suzuki 2021-01-27 15:58:41 PST
*** Bug 170511 has been marked as a duplicate of this bug. ***
Comment 54 Yusuke Suzuki 2021-01-27 17:31:41 PST
Committed r271993: <https://trac.webkit.org/changeset/271993>