Bug 234890 – ReadableStream::lock should check whether there is an exception when getting ReadableStreamDefaultReader private constructor

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

[youenn fablet]

ReadableStream::lock should check whether there is an exception when getting ReadableStreamDefaultReader private constructor

[youenn fablet]

Created attachment 448410 [details]
Patch

[Frédéric Wang (:fredw)]

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?

[youenn fablet]

(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.

[youenn fablet]

(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.

[youenn fablet]

Created attachment 448487 [details]
Patch

[youenn fablet]

I was able to write a test and added it in addition to write the utility function. @fredw, PTAL.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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.

[Mark Lam]

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.

[youenn fablet]

Created attachment 448514 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/87213143>