Bug 145600 – [Streams API] ReadableJSStream should handle JS source getters that throw

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

When getting pull, start, cancel JS functions, ReadableJSStream should be able to catch exceptions and go in error state.

[youenn fablet]

Created attachment 254263 [details]
Patch

[Darin Adler]

Comment on attachment 254263 [details]
Patch

It seems kind of strange how this code moves exceptions into and out of the VM. It’s more normal to just leave the exceptions in the VM. For example, getPropertyFromObject, callFunction, and ReadableJSStream::invoke, but then ReadableJSStream::doStart moves an exception from a local variable into the VM.

The better pattern is normally to remove all these separate exception out arguments and use the exception slot in the VM consistently. Then the code in ReadableJSStream::doStart just says:

    if (hadException)
       return;

[youenn fablet]

(In reply to comment #2)
> Comment on attachment 254263 [details]
> Patch
> 
> It seems kind of strange how this code moves exceptions into and out of the
> VM. It’s more normal to just leave the exceptions in the VM. For example,
> getPropertyFromObject, callFunction, and ReadableJSStream::invoke, but then
> ReadableJSStream::doStart moves an exception from a local variable into the
> VM.
> 
> The better pattern is normally to remove all these separate exception out
> arguments and use the exception slot in the VM consistently. Then the code
> in ReadableJSStream::doStart just says:
> 
>     if (hadException)
>        return;

You are right for "start".
But exceptions may be raised in the case of "pull" and "cancel" as well, where this is not practical.

For "pull" and "cancel", we will put the stream in error state, reject related promises... 

We could have two code paths but a single one seems better as the way to invoke all 3 functions is the same, except for exception handling.

[youenn fablet]

Created attachment 254479 [details]
Always throwExxxception option, fixed case of undefined passsed to ReadableStream constructor

[youenn fablet]

(In reply to comment #2)
> Comment on attachment 254263 [details]
> Patch
> 
> It seems kind of strange how this code moves exceptions into and out of the
> VM. It’s more normal to just leave the exceptions in the VM. For example,
> getPropertyFromObject, callFunction, and ReadableJSStream::invoke, but then
> ReadableJSStream::doStart moves an exception from a local variable into the
> VM.
> 
> The better pattern is normally to remove all these separate exception out
> arguments and use the exception slot in the VM consistently. Then the code
> in ReadableJSStream::doStart just says:
> 
>     if (hadException)
>        return;

Latest patch follows this pattern.
This is clearly nicer for "start".

We can check when integrating "pull", "cancel" and "size" whether to accommodate the code or not.

[Darin Adler]

Comment on attachment 254479 [details]
Always throwExxxception option, fixed case of undefined passsed to ReadableStream constructor

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

> Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:84
> +        return JSValue::encode(exec->exception());

When there’s an exception, the return value is normally ignored. So instead of returning the exception as a JSValue, it’s typical to return jsUndefined() instead. As this patch does below. Why return the exception here?

> Source/WebCore/bindings/js/ReadableJSStream.cpp:109
> +    if (exec.argumentCount() && !exec.argument(0).isUndefined()) {
> +        JSValue value = exec.argument(0);

Best way to write this is:

    JSValue value = exec.argument(0);
    if (!value.isUndefined()) {

There is no reason to check argumentCount if we are also checking if argument 0 is undefined, and there is no good reason to call argument(0) twice.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:116
> +        jsSource =  JSFinalObject::create(exec.vm(), JSFinalObject::createStructure(exec.vm(), exec.callee()->globalObject(), jsNull(), 1));

Stray space here after the "=" sign.

[youenn fablet]

Comment on attachment 254479 [details]
Always throwExxxception option, fixed case of undefined passsed to ReadableStream constructor

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

>> Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:84
>> +        return JSValue::encode(exec->exception());
> 
> When there’s an exception, the return value is normally ignored. So instead of returning the exception as a JSValue, it’s typical to return jsUndefined() instead. As this patch does below. Why return the exception here?

OK.
It was based on all the "return throwVMError(...)" that can be seen in various parts of Source/WebCore/bindings/js.

>> Source/WebCore/bindings/js/ReadableJSStream.cpp:109
>> +        JSValue value = exec.argument(0);
> 
> Best way to write this is:
> 
>     JSValue value = exec.argument(0);
>     if (!value.isUndefined()) {
> 
> There is no reason to check argumentCount if we are also checking if argument 0 is undefined, and there is no good reason to call argument(0) twice.

OK

>> Source/WebCore/bindings/js/ReadableJSStream.cpp:116
>> +        jsSource =  JSFinalObject::create(exec.vm(), JSFinalObject::createStructure(exec.vm(), exec.callee()->globalObject(), jsNull(), 1));
> 
> Stray space here after the "=" sign.

OK

[youenn fablet]

Created attachment 254492 [details]
Fixing according review

[Darin Adler]

Comment on attachment 254492 [details]
Fixing according review

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

> Source/WebCore/bindings/js/ReadableJSStream.cpp:116
> +    if (!value.isUndefined()) {
> +        if (!value.isObject()) {
> +            throwVMError(&exec, createTypeError(&exec, ASCIILiteral("ReadableStream constructor first argument, if any, should be an object")));
> +            return nullptr;
> +        }
> +        jsSource = value.getObject();
> +    } else
> +        jsSource = JSFinalObject::create(exec.vm(), JSFinalObject::createStructure(exec.vm(), exec.callee()->globalObject(), jsNull(), 1));

I would write this to make the logic clearer and avoid the nesting:

    JSValue value = exec.argument(0);
    if (value.isObject())
        jsSource = value.getObject();
    else if (value.isUndefined())
        jsSource = JSFinalObject::create(exec.vm(), JSFinalObject::createStructure(exec.vm(), exec.callee()->globalObject(), jsNull(), 1));
    else {
        throwVMError(&exec, createTypeError(&exec, ASCIILiteral("ReadableStream constructor first argument, if any, should be an object")));
        return nullptr;
    }

[youenn fablet]

Created attachment 254552 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 254552 [details]
Patch for landing

Clearing flags on attachment: 254552

Committed r185356: <http://trac.webkit.org/changeset/185356>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.