ReadableStream::lock should check whether there is an exception when getting ReadableStreamDefaultReader private constructor
Created attachment 448410 [details] Patch
Comment on attachment 448410 [details] Patch LGTM. Do we have a non-regression test for that? IIUC, the current return-if-exception line is not covered by our tests since otherwise it would currently crash when JSC::asObject is called?
(In reply to Frédéric Wang (:fredw) from comment #2) > Comment on attachment 448410 [details] > Patch > > LGTM. Do we have a non-regression test for that? IIUC, the current > return-if-exception line is not covered by our tests since otherwise it > would currently crash when JSC::asObject is called? I'll try to write one.
(In reply to youenn fablet from comment #3) > (In reply to Frédéric Wang (:fredw) from comment #2) > > Comment on attachment 448410 [details] > > Patch > > > > LGTM. Do we have a non-regression test for that? IIUC, the current > > return-if-exception line is not covered by our tests since otherwise it > > would currently crash when JSC::asObject is called? > > I'll try to write one. I tried to write one but I am not able to trigger the crash, using something like: const request = new Request("http://test.org", {"method": "POST", "body": "text" }); request.text(); request.body; I'll rewrite the patch to share the code and check between the two paths.
Created attachment 448487 [details] Patch
I was able to write a test and added it in addition to write the utility function. @fredw, PTAL.
Comment on attachment 448487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448487&action=review still LGTM but since this is a bigger refactoring, I'd prefer Mark Lam to check. > Source/WebCore/bindings/js/ReadableStream.cpp:-120 > - auto scope = DECLARE_CATCH_SCOPE(vm); In the past, there was a catch scope here but I'm not sure why since I don't see any explicit clearing of exceptions. > Source/WebCore/bindings/js/ReadableStream.cpp:135 > + args.append(readableStream()); I guess you should keep ASSERT(!args.hasOverflowed()) here? > LayoutTests/ChangeLog:9 > + * streams/readable-stream-lock-after-worker-terminates-crash.html: Added. So this new test is crashing in ReadableStream::lock without your patch right? Just to be sure since the crashing cast operation is now shared in invokeConstructor... I believe ReadableStream::create is already covered by streams/readable-stream-create-after-worker-terminates-crash.html > LayoutTests/streams/readable-stream-lock-after-worker-terminates-crash.html:11 > + const worker = new Worker('data:application/javascript;charset=utf-8,' + createRequest.toString() + ';createRequest()'); nit: you can probably use backticks here too.
Comment on attachment 448487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448487&action=review > Source/WebCore/bindings/js/ReadableStream.cpp:48 > + EXCEPTION_ASSERT(!scope.exception() || vm.hasPendingTerminationException()); I also forgot to say, in the past ReadableStream::create was not asserting for non-termination exception, but again I'm not sure whether such an exception could happen.
Comment on attachment 448487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448487&action=review I have some comments below but LGTM too. > Source/WebCore/bindings/js/ReadableStream.cpp:40 > +static inline ExceptionOr<JSObject*> invokeConstructor(JSC::JSGlobalObject& lexicalGlobalObject, const JSC::Identifier& identifier, const Function<void(MarkedArgumentBuffer&, JSC::JSGlobalObject&, JSDOMGlobalObject&)>& buildArguments) nit: since you have declared `using namespace JSC` above, I think you can drop all the `JSC::` qualifiers here and below. It'll make the code a lot more readable. >> Source/WebCore/bindings/js/ReadableStream.cpp:48 >> + EXCEPTION_ASSERT(!scope.exception() || vm.hasPendingTerminationException()); > > I also forgot to say, in the past ReadableStream::create was not asserting for non-termination exception, but again I'm not sure whether such an exception could happen. It can happen if the property being fetched from the globalObject is a lazy property which may entail some complex code. Given the RETURN_IF_EXCEPTION below, I'd almost say that this EXCEPTION_ASSERT is not necessary for exception check validation. However, it does serve a purpose to document an invariant that we're not expecting any exception to be thrown here other than the TerminationException. So, I think it's good to keep it. >> Source/WebCore/bindings/js/ReadableStream.cpp:-120 >> - auto scope = DECLARE_CATCH_SCOPE(vm); > > In the past, there was a catch scope here but I'm not sure why since I don't see any explicit clearing of exceptions. This was a bug. Since there's a possibility of a TerminationException, the use of a CatchScope here was incorrect. This patch is doing the right thing. >> Source/WebCore/bindings/js/ReadableStream.cpp:135 >> + args.append(readableStream()); > > I guess you should keep ASSERT(!args.hasOverflowed()) here? Or you can move ASSERT(!args.hasOverflowed()) to after the call to buildArguments() in invokeConstructor(). Asserting it here would be nicer as the crash trace will point to which call to invokeConstructor() caused the assertion failure.
Created attachment 448514 [details] Patch for landing
Committed r287711 (245796@main): <https://commits.webkit.org/245796@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 448514 [details].
<rdar://problem/87213143>