[webkit-reviews] review denied: [Bug 183442] WebAssembly: add support for stream APIs - JavaScript API : [Attachment 337296] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has denied GSkachkov
<gskachkov at gmail.com>'s request for review:
Bug 183442: WebAssembly: add support for stream APIs - JavaScript API
https://bugs.webkit.org/show_bug.cgi?id=183442

Attachment 337296: Patch

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




--- Comment #4 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 337296
  --> https://bugs.webkit.org/attachment.cgi?id=337296
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. at resolve(argument). at 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.