Bug 173105 – WebAssembly: add support for stream APIs

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[JF Bastien]

Post-MVP APIs were added: https://github.com/WebAssembly/design/blob/master/Web.md#additional-web-embedding-api

WebAssembly.compileStreaming
WebAssembly.instantiateStreaming

[JF Bastien]

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.

[JF Bastien]

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.

[GSkachkov]

Can I take this task? I will have free time so possible I'll manage implement this.

[JF Bastien]

Sure.

[GSkachkov]

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

[JF Bastien]

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.

[GSkachkov]

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

[Rob]

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.

[Rob]

Created attachment 369602 [details]
WASM and CSP test cases.

Here is a set of tests and a summary of major browser behavior.

[Adam Klein]

Any updates here?

[Radar WebKit Bug Importer]

<rdar://problem/57945940>

[Maksim Nesterenko]

Hello, is there any plans on adding this?

[Yusuke Suzuki]

This is currently planned in my TODO tasks :)

[Maksim Nesterenko]

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).

[Kenneth Russell]

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!

[styfle]

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

[Yusuke Suzuki]

*** Bug 184888 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

*** Bug 183444 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

*** Bug 183443 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

*** Bug 184886 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Created attachment 418334 [details]
Patch

[Yusuke Suzuki]

Created attachment 418374 [details]
Patch

[Yusuke Suzuki]

Created attachment 418377 [details]
Patch

[Yusuke Suzuki]

Created attachment 418382 [details]
Patch

[Yusuke Suzuki]

Created attachment 418388 [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]

Created attachment 418488 [details]
Patch

[Yusuke Suzuki]

Created attachment 418497 [details]
Patch

[Yusuke Suzuki]

Created attachment 418505 [details]
Patch

[Yusuke Suzuki]

Created attachment 418506 [details]
Patch

[Yusuke Suzuki]

Created attachment 418507 [details]
Patch

[Yusuke Suzuki]

Created attachment 418508 [details]
Patch

[Yusuke Suzuki]

Created attachment 418585 [details]
Patch

[Keith Miller]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

*** Bug 170511 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Committed r271993: <https://trac.webkit.org/changeset/271993>