Bug 183442 – WebAssembly: add support for stream APIs - JavaScript API

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[GSkachkov]

Add function compileStreaming & instantiateStreaming to WebAssembly that work in the same as compile&instantiate(start to compile only after whole wasm file loaded), but have the same parameters as in https://www.w3.org/TR/2018/WD-wasm-web-api-1-20180215/#streaming-modules

[GSkachkov]

Created attachment 337019 [details]
Patch

WiP. Patch without tests. Current patch crash during WebAssembly.instantiateStreaming in case of large wasm modules

[EWS Watchlist]

Attachment 337019 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[GSkachkov]

Created attachment 337296 [details]
Patch

Patch for review

[Yusuke Suzuki]

Comment on attachment 337296 [details]
Patch

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

r- given that EWS bots are red.

> Source/JavaScriptCore/runtime/JSGlobalObject.h:222
> +    typedef void (*CompileStreamingPtr)(JSGlobalObject*, ExecState*, JSPromiseDeferred*, JSValue);

Use `using`.

> Source/JavaScriptCore/runtime/JSGlobalObject.h:225
> +    typedef void (*InstantiateStreamingPtr)(JSGlobalObject*, ExecState*, JSPromiseDeferred*, JSValue, JSObject *);

Ditto.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:91
> +    Wasm::Module::validateAsync(&vm.wasmContext, WTFMove(source), createSharedTask<Wasm::Module::CallbackType>([promise, globalObject, &vm] (Wasm::Module::ValidationResult&& result) mutable {
> +        vm.promiseDeferredTimer->scheduleWorkSoon(promise, [promise, globalObject, result = WTFMove(result), &vm] () mutable {

This function is wrong. Its lambda captures promise, importObject, globalObject. But nobody protects them from GC.
While webAssemblyCompileFunc protects them with addPendingPromise, webAssemblyModuleValidateAsync does not call it. So it can be collected under the context of webAssemblyModuleValidateAsync.
We should call addPendingPromise & dependencies too in this extracted function. And webAssemblyCompileFunc should not call addPendingPromise. I think webAssemblyModuleInstantinateAsyncInternal does that thing.

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

Can we define it as a simple global static function in this file scope instead of WebAssemblyPrototype's static function?

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:221
> +    Vector<Strong<JSCell>> dependencies;
> +    dependencies.append(Strong<JSCell>(vm, importObject));
> +    vm.promiseDeferredTimer->addPendingPromise(promise, WTFMove(dependencies));

This is really important.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:295
> +        JSValue source = exec->argument(0);

Drop this line.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:296
> +        globalObject->globalObjectMethodTable()->compileStreaming(globalObject, exec, promise, source);

Call `globalObject->globalObjectMethodTable()->compileStreaming(globalObject, exec, promise, exec->argument(0));`

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:324
> +                // Fixme: 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.

Use `FIXME: `. And add bugzilla URL for FIXMEs.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:325
> +                JSValue source = exec->argument(0);

Drop this line.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:326
> +                globalObject->globalObjectMethodTable()->instantiateStreaming(globalObject, exec, promise, source, importObject);

Just calling `globalObject->globalObjectMethodTable()->instantiateStreaming(globalObject, exec, promise, exec->argument(0), importObject);`

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.h:45
> +    static void webAssemblyModuleValidateAsyncInternal(ExecState*, Vector<uint8_t>&&, JSPromiseDeferred*);
> +    static void webAssemblyModuleInstantinateAsyncInternal(ExecState*, Vector<uint8_t>&&, JSObject*, JSPromiseDeferred*);

Why are these two functions are static member functions of WebAssemblyPrototype? Can we define them as static functions in WebAssemblyPrototype.cpp?

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:419
> +    handleStreamingAction(globalObject, exec, source, promise, [promise] (JSC::ExecState* exec, const char* data, size_t byteSize) mutable {

JSC value `promise` in the lambda is not protected. The lambda does not allow GC to visit its captured values. So it can be collected.
Let's protect promise, and dependencies by addPendingPromise. Maybe, promise is protected well in the caller side, but `source` would not be.

But personally, if you rewrite some part of compileStreaming/instantiateStreaming in builtin JS, we can just type check this `source` here. And we can extract C++ unwrapped Response. At that time, we can just hold that Ref<>.
When calling this function, promise would be already protected by calling addPendingPromise. So if you rewrite the current implementation so, we do not need to call addPendingPromise here.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:420
> +        Vector<uint8_t> arrayBuffer;

And I believe we should add some assertion that `promise` is protected by `addPendingPromise` mechanism.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:428
> +    handleStreamingAction(globalObject, exec, source, promise, [promise, importedObject] (JSC::ExecState* exec, const char* data, size_t byteSize) mutable {

Ditto. I believe we should add some assertion that `promise` and `importedObject` is protected by `addPendingPromise` mechanism.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:443
> +        JSC::JSFunction* fulfillHandler = JSC::JSNativeStdFunction::create(vm, globalObject, 1, String(), [promise, m_callback = WTFMove(callback), globalObject](JSC::ExecState* fulfillState) {

I think we do not need to do this here, I would like to remove JSNativeStdFunction usage as much as I can.
And m_callback is not a member variable.

I have a bit simple idea: how about resolving promise in JSC's instantiateStreaming etc. function?

You define instantiateStreaming etc. as JSC builtin. And inside it, you wrap the given value with the promise.

function instantiateStreaming(argument)
{
    return @Promise.@resolve(argument).@then(@instantiateStreamingInternal);
}

And in JSC's C implementated @instantiateStreamingInternal function, you can just call this hook. And in this hook, we just check JSFetchResponse type instead of handling Promise case.
So, you can remove JSNativeStdFunction here. And you can remove this function. instantiateStreaming and compileStreaming just type checks JSFetchResponse and call handleResponseOnStreamingAction.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:469
> +        inputResponse->consumeBodyReceivedByChunk([promise, m_callback = WTFMove(callback), globalObject, data = SharedBuffer::create()] (auto&& result) mutable {

m_callback is not a right name. It is not a member function.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:498
> +        // FIXME: Implement loading for the Blob type

Add bugzilla URL.

[Yusuke Suzuki]

Comment on attachment 337296 [details]
Patch

And we should make this behind the flag until `streaming` feature is done. Currently, we are not streaming.

[GSkachkov]

(In reply to Yusuke Suzuki from comment #5)
> Comment on attachment 337296 [details]
> Patch
> 
> And we should make this behind the flag until `streaming` feature is done.
> Currently, we are not streaming.

Thanks for review! I'll try to fix during next week :)

[JF Bastien]

Comment on attachment 337296 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:381
> +static bool tryCopySourceToArrayBuffer(JSC::ExecState* exec, JSC::JSPromiseDeferred* promise, const char* data, size_t byteSize, Vector<uint8_t>&& arrayBuffer)

arrayBuffer can't be an rvalue reference, you don't pass it out of the function and the caller tries to reuse it! It should just be  normal reference, no move at the callsites.

Or you can rename to tryAllocate and return the vector of the right size here, in a std::optional.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:384
> +        promise->reject(exec, createTypeError(exec, ASCIILiteral("Not enough memmory to load webAssemlby sources")));

Typo WebAssembly

[GSkachkov]

Comment on attachment 337296 [details]
Patch

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

>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:214
>> +void WebAssemblyPrototype::webAssemblyModuleInstantinateAsyncInternal(ExecState* exec, Vector<uint8_t>&& source, JSObject* importObject, JSPromiseDeferred* promise)
> 
> Can we define it as a simple global static function in this file scope instead of WebAssemblyPrototype's static function?

Done

>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:295
>> +        JSValue source = exec->argument(0);
> 
> Drop this line.

done

>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:296
>> +        globalObject->globalObjectMethodTable()->compileStreaming(globalObject, exec, promise, source);
> 
> Call `globalObject->globalObjectMethodTable()->compileStreaming(globalObject, exec, promise, exec->argument(0));`

Done

>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:324
>> +                // Fixme: 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.
> 
> Use `FIXME: `. And add bugzilla URL for FIXMEs.

Done

>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:326
>> +                globalObject->globalObjectMethodTable()->instantiateStreaming(globalObject, exec, promise, source, importObject);
> 
> Just calling `globalObject->globalObjectMethodTable()->instantiateStreaming(globalObject, exec, promise, exec->argument(0), importObject);`

done

>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.h:45
>> +    static void webAssemblyModuleInstantinateAsyncInternal(ExecState*, Vector<uint8_t>&&, JSObject*, JSPromiseDeferred*);
> 
> Why are these two functions are static member functions of WebAssemblyPrototype? Can we define them as static functions in WebAssemblyPrototype.cpp?

done

>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:381
>> +static bool tryCopySourceToArrayBuffer(JSC::ExecState* exec, JSC::JSPromiseDeferred* promise, const char* data, size_t byteSize, Vector<uint8_t>&& arrayBuffer)
> 
> arrayBuffer can't be an rvalue reference, you don't pass it out of the function and the caller tries to reuse it! It should just be  normal reference, no move at the callsites.
> 
> Or you can rename to tryAllocate and return the vector of the right size here, in a std::optional.

Done

>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:384
>> +        promise->reject(exec, createTypeError(exec, ASCIILiteral("Not enough memmory to load webAssemlby sources")));
> 
> Typo WebAssembly

Done

>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:443
>> +        JSC::JSFunction* fulfillHandler = JSC::JSNativeStdFunction::create(vm, globalObject, 1, String(), [promise, m_callback = WTFMove(callback), globalObject](JSC::ExecState* fulfillState) {
> 
> I think we do not need to do this here, I would like to remove JSNativeStdFunction usage as much as I can.
> And m_callback is not a member variable.
> 
> I have a bit simple idea: how about resolving promise in JSC's instantiateStreaming etc. function?
> 
> You define instantiateStreaming etc. as JSC builtin. And inside it, you wrap the given value with the promise.
> 
> function instantiateStreaming(argument)
> {
>     return @Promise.@resolve(argument).@then(@instantiateStreamingInternal);
> }
> 
> And in JSC's C implementated @instantiateStreamingInternal function, you can just call this hook. And in this hook, we just check JSFetchResponse type instead of handling Promise case.
> So, you can remove JSNativeStdFunction here. And you can remove this function. instantiateStreaming and compileStreaming just type checks JSFetchResponse and call handleResponseOnStreamingAction.

Created separated builtin module, WebAssemblyPrototype.js

>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:469
>> +        inputResponse->consumeBodyReceivedByChunk([promise, m_callback = WTFMove(callback), globalObject, data = SharedBuffer::create()] (auto&& result) mutable {
> 
> m_callback is not a right name. It is not a member function.

Renamed

>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:498
>> +        // FIXME: Implement loading for the Blob type
> 
> Add bugzilla URL.

Done

[GSkachkov]

Created attachment 338599 [details]
Patch

Patch with fixes

[JF Bastien]

Comment on attachment 338599 [details]
Patch

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

> Source/JavaScriptCore/runtime/Options.h:487
> +    v(bool, useWebAssemblyStreamingApi, true, Normal, "Allow to run WebAssembly's Streaming API") \

Can you add a feature flag the same way as here:
  https://bugs.webkit.org/show_bug.cgi?id=184312
?

You still have an option, but it's configurable at build time whether it's on or off.

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

Looks like you want to take ownership of "source" in this function, so it should be an rvalue reference.

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

Ditto rvalue ref for "source".

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

Is this code even reachable? Seems like it should be an assert instead.

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

Ditto

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:369
> +    if (Options::useWebAssemblyStreamingApi()) {

Here I think it should only be added then no in the shell as well.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:385
> +        promise->reject(exec, createTypeError(exec, ASCIILiteral("Not enough memmory to load webAssembly sources")));

Typo "memory"

Also this should be createOutOfMemoryError?

[GSkachkov]

Created attachment 338662 [details]
Patch

Fix comments

[GSkachkov]

Comment on attachment 338599 [details]
Patch

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

>> Source/JavaScriptCore/runtime/Options.h:487
>> +    v(bool, useWebAssemblyStreamingApi, true, Normal, "Allow to run WebAssembly's Streaming API") \
> 
> Can you add a feature flag the same way as here:
>   https://bugs.webkit.org/show_bug.cgi?id=184312
> ?
> 
> You still have an option, but it's configurable at build time whether it's on or off.

In new patch I've introduced ENABLE_WEBASSEMBLY_STREAMING_API feature flag

>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:85
>> +static void webAssemblyModuleValidateAsyncInternal(ExecState* exec, JSPromiseDeferred* promise, Vector<uint8_t>& source)
> 
> Looks like you want to take ownership of "source" in this function, so it should be an rvalue reference.

Done

>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:156
>> +void WebAssemblyPrototype::webAssemblyModuleValidateAsync(ExecState* exec, JSPromiseDeferred* promise, Vector<uint8_t>& source)
> 
> Ditto rvalue ref for "source".

Done

>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:316
>> +        promise->reject(exec, createTypeError(exec, ASCIILiteral("CompileStreaming is not supported in jsc, only in browser environment.")));
> 
> Is this code even reachable? Seems like it should be an assert instead.

Fixed

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

Fixed

>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:385
>> +        promise->reject(exec, createTypeError(exec, ASCIILiteral("Not enough memmory to load webAssembly sources")));
> 
> Typo "memory"
> 
> Also this should be createOutOfMemoryError?

Fixed

[GSkachkov]

> And I believe we should add some assertion that `promise` is protected by `addPendingPromise` mechanism.

@Yusuke Suzuki Do already have function that answer if object is already protected by `addPendingPromise` mechanism, or it has to be introduced in this patch?

[Adrian Perez]

Comment on attachment 338662 [details]
Patch

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

I have no comments about the functionality of the patch itself, 
but the changes to the CMake build files are breaking the EWS bots.
Please fix at least that and re-upload.

> Source/JavaScriptCore/CMakeLists.txt:1122
> +    $(JAVASCRIPTCORE_DIR)/builtins/WebAssemblyPrototype.js

Please use ${JAVASCRIPTCORE_DIR} here, with brackets, which is how CMake
variables have to be expanded. Using parentheses causes CMake to generate
invalid output that Ninja cannot parse, and this patch is breaking the
EWSs which use Ninja (like GTK+ and WPE).

[GSkachkov]

Created attachment 338861 [details]
Patch

EWS build

[GSkachkov]

Created attachment 338864 [details]
Patch

Fix build

[GSkachkov]

Created attachment 338865 [details]
Patch

Fix build #2

[EWS Watchlist]

Comment on attachment 338865 [details]
Patch

Attachment 338865 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7465945

New failing tests:
stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate

[GSkachkov]

Created attachment 338869 [details]
Patch

Fix build #4

[Yusuke Suzuki]

Comment on attachment 338869 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/ChangeLog:73
> +        * CMakeLists.txt:
> +        * Configurations/FeatureDefines.xcconfig:
> +        * DerivedSources.make:
> +        * JavaScriptCore.xcodeproj/project.pbxproj:
> +        * builtins/BuiltinNames.h:
> +        * builtins/WebAssemblyPrototype.js: Copied from Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.h.
> +        (compileStreaming):
> +        (instantiateStreaming):
> +        * jsc.cpp:
> +        * runtime/JSGlobalObject.cpp:
> +        (JSC::JSGlobalObject::init):
> +        * runtime/JSGlobalObject.h:
> +        * runtime/Options.h:
> +        * wasm/js/WebAssemblyPrototype.cpp:
> +        (JSC::webAssemblyModuleValidateAsyncInternal):
> +        (JSC::webAssemblyCompileFunc):
> +        (JSC::WebAssemblyPrototype::webAssemblyModuleValidateAsync):
> +        (JSC::webAssemblyModuleInstantinateAsyncInternal):
> +        (JSC::WebAssemblyPrototype::webAssemblyModuleInstantinateAsync):
> +        (JSC::webAssemblyCompileStreamingInternal):
> +        (JSC::webAssemblyInstantiateStreamingInternal):
> +        (JSC::WebAssemblyPrototype::create):
> +        (JSC::WebAssemblyPrototype::finishCreation):
> +        * wasm/js/WebAssemblyPrototype.h:

Please fix ChangeLog, which includes duplicate entries.

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:357
> +ENABLE_WEBASSEMBLY_STREAMING_API = ENABLE_WEBASSEMBLY_STREAMING_API;

Could you ensure this is disabled right now? (then, we do not have any compileStreaming/instantiateStreaming functions exposed to the Web right now). We should disable this feature until we fully implement streaming functionality.

> Source/JavaScriptCore/builtins/WebAssemblyPrototype.js:26
> +function compileStreaming(argument) {

Add "use strict";

> Source/JavaScriptCore/builtins/WebAssemblyPrototype.js:30
> +function instantiateStreaming(argument) {

Ditto.

> Source/JavaScriptCore/runtime/Options.h:540
> +    v(enableWebAssemblyStreamingApi, useWebAssemblyStreamingApi, SameOption) \

Remove this.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:231
> +    dependencies.append(Strong<JSCell>(vm, importObject));

Let's add `globalObject` too.

> Source/WebCore/Configurations/FeatureDefines.xcconfig:357
> +ENABLE_WEBASSEMBLY_STREAMING_API = ENABLE_WEBASSEMBLY_STREAMING_API;

Ditto.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:440
> +void JSDOMWindowBase::handleStreamingAction(JSC::JSGlobalObject* globalObject, JSC::ExecState* exec, JSC::JSPromiseDeferred* promise, JSC::JSValue source, std::function<void(JSC::ExecState* exec, const char* data, size_t byteSize)>&& callback)
> +{
> +    ASSERT(source);
> +    VM& vm = exec->vm();
> +    if (auto inputResponse = JSFetchResponse::toWrapped(vm, source))
> +        handleResponseOnStreamingAction(globalObject, exec, inputResponse, promise, WTFMove(callback));
> +    else
> +        promise->reject(exec, createTypeError(exec, ASCIILiteral("first argument must be an Response or Promise for Response")));
> +}

This wrapper function seems unnecessary. Just calling handleResponseOnStreamingAction in instantiateStreaming and compileStreaming is simpler.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:442
> +void JSDOMWindowBase::handleResponseOnStreamingAction(JSC::JSGlobalObject* globalObject, JSC::ExecState* exec, FetchResponse* inputResponse, JSC::JSPromiseDeferred* promise, std::function<void(JSC::ExecState* exec, const char* data, size_t byteSize)>&& actionCallback)

Is it necessary to be a static member function of JSDOMWindowBase? Can we make it per-file static function?
And let's use Function<> instead of std::function.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:474
> +            auto callback = WTFMove(actionCallback);
> +            callback(exec, buffer->data(), buffer->size());

`actionCallback(exec, buffer->data(), buffer->size());` should work.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:484
> +        auto callback = WTFMove(actionCallback);
> +        callback(exec, buffer->data(), buffer->size());

Ditto.

[Yusuke Suzuki]

(In reply to GSkachkov from comment #13)
> > And I believe we should add some assertion that `promise` is protected by `addPendingPromise` mechanism.
> 
> @Yusuke Suzuki Do already have function that answer if object is already
> protected by `addPendingPromise` mechanism, or it has to be introduced in
> this patch?

I think we need to add this in this patch (and calling it in ASSERT).
So please add it before landing.

[EWS Watchlist]

Comment on attachment 338869 [details]
Patch

Attachment 338869 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7466813

New failing tests:
imported/w3c/web-platform-tests/wasm/wasm_stream_instantinate_test.html
imported/w3c/web-platform-tests/wasm/wasm_stream_compile_test.html

[EWS Watchlist]

Created attachment 338870 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[JF Bastien]

Comment on attachment 338869 [details]
Patch

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

> Source/cmake/WebKitFeatures.cmake:177
> +    WEBKIT_OPTION_DEFINE(ENABLE_WEBASSEMBLY_STREAMING_API "Toggle WebAssembly streaming api support." PRIVATE ON)

The default here should be OFF

[JF Bastien]

Comment on attachment 338869 [details]
Patch

r=me as well


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

> Source/cmake/WebKitFeatures.cmake:177
> +    WEBKIT_OPTION_DEFINE(ENABLE_WEBASSEMBLY_STREAMING_API "Toggle WebAssembly streaming api support." PRIVATE ON)

The default here should be OFF

[GSkachkov]

Created attachment 339089 [details]
Patch

Check fixed builds

[GSkachkov]

Created attachment 339090 [details]
Patch

Rebase & check fixed builds

[GSkachkov]

Created attachment 339093 [details]
Patch

Check fixed ios builds

[EWS Watchlist]

Comment on attachment 339093 [details]
Patch

Attachment 339093 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7500575

New failing tests:
stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate

[GSkachkov]

Created attachment 339098 [details]
Patch

Check fixed ios-sim&32-bit  builds

[EWS Watchlist]

Comment on attachment 339098 [details]
Patch

Attachment 339098 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7501852

New failing tests:
imported/w3c/web-platform-tests/wasm/wasm_stream_instantiate_test.html
imported/w3c/web-platform-tests/wasm/wasm_stream_compile_test.html

[EWS Watchlist]

Created attachment 339100 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[GSkachkov]

Created attachment 339146 [details]
Patch

Check fixed debug build&tests

[GSkachkov]

Patch landed https://trac.webkit.org/r231194

[Radar WebKit Bug Importer]

<rdar://problem/39862193>

[Ryan Haddad]

The iOS Simulator debug build is broken by this change.

./bindings/js/JSDOMWindowBase.cpp:495:1: error: function 'compileStreaming' could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn]
{
^
./bindings/js/JSDOMWindowBase.cpp:499:1: error: function 'instantiateStreaming' could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn]
{
^

https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20%28Build%29/builds/5600

[GSkachkov]

Oh I see. I'll check if we can rollback

[GSkachkov]

(In reply to Ryan Haddad from comment #36)
> The iOS Simulator debug build is broken by this change.
> 
> ./bindings/js/JSDOMWindowBase.cpp:495:1: error: function 'compileStreaming'
> could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn]
> {
> ^
> ./bindings/js/JSDOMWindowBase.cpp:499:1: error: function
> 'instantiateStreaming' could be declared with attribute 'noreturn'
> [-Werror,-Wmissing-noreturn]
> {
> ^
> 
> https://build.webkit.org/builders/
> Apple%20iOS%2011%20Simulator%20Debug%20%28Build%29/builds/5600

I seems that we already faced with this issue 
https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/jit/JITArithmetic32_64.cpp#L347
At least in this code, assertion was commented out. 
I don't know what to do, or rollback patch, or just create new small patch with commented ASSERT_NOT_REACHED

[Alex Christensen]

Comment on attachment 339146 [details]
Patch

This has been requesting review for more than one year.  If this is still needed, please rebase and re-request review.