Bug 173105

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.

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.

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

Sure.

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

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

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.

Created attachment 369602 [details] WASM and CSP test cases. Here is a set of tests and a summary of major browser behavior.

Any updates here?

<rdar://problem/57945940>

Hello, is there any plans on adding this?

This is currently planned in my TODO tasks :)

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

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!

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

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

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

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

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

Created attachment 418334 [details] Patch

Created attachment 418374 [details] Patch

Created attachment 418377 [details] Patch

Created attachment 418382 [details] Patch

Created attachment 418388 [details] Patch

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

Created attachment 418488 [details] Patch

Created attachment 418497 [details] Patch

Created attachment 418505 [details] Patch

Created attachment 418506 [details] Patch

Created attachment 418507 [details] Patch

Created attachment 418508 [details] Patch

Created attachment 418585 [details] Patch

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

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

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