[webkit-reviews] review denied: [Bug 179826] WebAssembly JS API: throw when a promise can't be created : [Attachment 327191] patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied JF Bastien <jfbastien at apple.com>'s
request for review:
Bug 179826: WebAssembly JS API: throw when a promise can't be created
https://bugs.webkit.org/show_bug.cgi?id=179826

Attachment 327191: patch

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




--- Comment #4 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 327191
  --> https://bugs.webkit.org/attachment.cgi?id=327191
patch

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

I suspect once you've changed these to using ThrowScopes, some jsc tests will
start failing (due to missing exception checks).  Please change these and
retest.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:97
>      auto scope = DECLARE_CATCH_SCOPE(vm);
>      auto* globalObject = exec->lexicalGlobalObject();
>  
> -    JSPromiseDeferred* promise = JSPromiseDeferred::create(exec,
globalObject);
> -    CLEAR_AND_RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +    JSPromiseDeferred* promise = createPromise(vm, exec, globalObject);
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());

This does not look right.  Being in a CatchScope means we should never return
with an exception.  I think you'll want to change that DECLARE_CATCH_SCOPE to
DECLARE_THROW_SCOPE.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:199
>      auto scope = DECLARE_CATCH_SCOPE(vm);
> -
> -    JSPromiseDeferred* promise = JSPromiseDeferred::create(exec,
exec->lexicalGlobalObject());
> -    CLEAR_AND_RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +    auto* globalObject = exec->lexicalGlobalObject();
> +    
> +    JSPromiseDeferred* promise = createPromise(vm, exec, globalObject);
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());

Ditto.	You should be using a DECLARE_THROW_SCOPE here.