When getting pull, start, cancel JS functions, ReadableJSStream should be able to catch exceptions and go in error state.
Created attachment 254263 [details] Patch
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;
(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.
Created attachment 254479 [details] Always throwExxxception option, fixed case of undefined passsed to ReadableStream constructor
(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.
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.
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
Created attachment 254492 [details] Fixing according review
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; }
Created attachment 254552 [details] Patch for landing
Comment on attachment 254552 [details] Patch for landing Clearing flags on attachment: 254552 Committed r185356: <http://trac.webkit.org/changeset/185356>
All reviewed patches have been landed. Closing bug.