[webkit-reviews] review granted: [Bug 213819] ReadableStream::create() should handle any exceptions that may be thrown during construction. : [Attachment 403384] proposed patch.

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 213819: ReadableStream::create() should handle any exceptions that may be
thrown during construction.
https://bugs.webkit.org/show_bug.cgi?id=213819

Attachment 403384: proposed patch.

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




--- Comment #9 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 403384
  --> https://bugs.webkit.org/attachment.cgi?id=403384
proposed patch.

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

> Source/WebCore/ChangeLog:8
> +	   Win EWS detected that ReadableStream::create() can throw exceptions,
and we were

Looking at Win EWS, it seems creating a ReadableStream has an issue as
@setupReadableStreamDefaultController is not available.
This seems to indicate an underlying bug that would be nice to investigate.
That said, I would think that creation of a ReadableStream can throw an
exception, for instance when a worker is getting terminated, so this patch
looks good to me.

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:320
> +    if (!m_body->hasReadableStream()) {

It seems we will retry creating a readable stream every time
FetchBodyOwner::readableStream is called, while before the patch, we would do
it once (since after that we are disturbed).
I do not think this is a big deal though.

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:337
>	   m_body->readableStream()->lock();

Maybe early return { } so that there is no else.

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:340
> +	   auto streamOrException = ReadableStream::create(state,
m_readableStreamSource);

If creation fails, we should probably set m_readableStreamSource back to
nullptr to keep a consistent state.

> Source/WebCore/bindings/js/ReadableStream.cpp:57
> +    JSObject* object = JSC::construct(&lexicalGlobalObject, constructor,
constructData, args);

auto*

> Source/WebCore/bindings/js/ReadableStream.cpp:62
>      return create(globalObject, *newReadableStream);

Combine both lines?