Bug 213819 – ReadableStream::create() should handle any exceptions that may be thrown during construction.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Mark Lam]

...

[Mark Lam]

Created attachment 403246 [details]
proposed patch.

[Mark Lam]

Comment on attachment 403246 [details]
proposed patch.

Taking out of review.  Let's see what the EWS says.

[Darin Adler]

Comment on attachment 403246 [details]
proposed patch.

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

> Source/WebCore/bindings/js/ReadableStream.cpp:56
> +    RELEASE_ASSERT(!scope.exception() && object);

I suggest two separate RELEASE_ASSERT so it’s clearer which one failed if only one did instead of a single with &&.

[Mark Lam]

Created attachment 403248 [details]
proposed patch.

[Mark Lam]

Created attachment 403382 [details]
proposed patch.

[Geoffrey Garen]

Comment on attachment 403382 [details]
proposed patch.

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

> Source/WebCore/Modules/cache/DOMCache.cpp:369
> +        auto streamOfException = response->readableStream(*scriptExecutionContext()->execState());

I think you meant streamOrException?

[Mark Lam]

Comment on attachment 403382 [details]
proposed patch.

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

>> Source/WebCore/Modules/cache/DOMCache.cpp:369
>> +        auto streamOfException = response->readableStream(*scriptExecutionContext()->execState());
> 
> I think you meant streamOrException?

Thanks for catching that.  A streamOfException would actually be funny.  Will fix.

[Mark Lam]

Created attachment 403384 [details]
proposed patch.

[youenn fablet]

Comment on attachment 403384 [details]
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?

[Yusuke Suzuki]

Comment on attachment 403384 [details]
proposed patch.

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

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

Let's use jsCast instead of jsDynamicCast since here we are not assuming that this would fail.

[Mark Lam]

Comment on attachment 403384 [details]
proposed patch.

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

Thanks for the reviews.

>> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:337
>>          m_body->readableStream()->lock();
> 
> Maybe early return { } so that there is no else.

Fixed.

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

Fixed.

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

It is not clear from reading this code that JSC::construct() returns a JSObject*.  I think there's value here in documenting this by declaring it explicitly.

>>> Source/WebCore/bindings/js/ReadableStream.cpp:62
>>>      return create(globalObject, *newReadableStream);
>> 
>> Combine both lines?
> 
> Let's use jsCast instead of jsDynamicCast since here we are not assuming that this would fail.

Fixed and fixed.

[Mark Lam]

It looks like this patch is successful in fixing the majority of crashes due to the unhandled exception.  However, there appears to be 1 more test failures:

http/tests/fetch/clone-response-body.html

... with the following crash stack:

 # Child-SP          RetAddr           Call Site
00 000000f3`d88fcbd0 00007fff`364e5439 WebKit!abort(void)+0x35 [minkernel\crts\ucrt\src\appcrt\startup\abort.cpp @ 77]
01 (Inline Function) --------`-------- WebKit!WTF::Optional<WebCore::ReadableStreamDefaultController>::value+0xa0 [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf\Optional.h @ 550]
02 (Inline Function) --------`-------- WebKit!WebCore::ReadableStreamSource::controller+0xa0 [C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\Modules\streams\ReadableStreamSource.h @ 49]
03 000000f3`d88fcc00 00007fff`35de16a8 WebKit!WebCore::FetchBodySource::error(class WebCore::Exception * value = 0x000000f3`d88fcc88)+0xb9 [C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\Modules\fetch\FetchBodySource.cpp @ 92]
04 000000f3`d88fcc60 00007fff`360e0aad WebKit!WebCore::FetchResponse::BodyLoader::didFail(class WebCore::ResourceError * error = <Value unavailable error>)+0x238 [C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\Modules\fetch\FetchResponse.cpp @ 308]
05 000000f3`d88fcd10 00007fff`360e0536 WebKit!WebCore::CachedResource::checkNotify(class WebCore::NetworkLoadMetrics * metrics = 0x000000f3`d88fce08)+0x11d [C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\loader\cache\CachedResource.cpp @ 375]
06 000000f3`d88fcd90 00007fff`35a79abe WebKit!WebCore::CachedResource::cancelLoad(void)+0x246 [C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\loader\cache\CachedResource.cpp @ 417]
07 000000f3`d88fcee0 00007fff`35a6ae50 WebKit!WebCore::SubresourceLoader::didCancel(class WebCore::ResourceError * __formal = 0x000000f3`d88fd010)+0x1e [C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\loader\SubresourceLoader.cpp @ 831]
08 000000f3`d88fcf10 00007fff`35a6af7f WebKit!WebCore::ResourceLoader::cancel(class WebCore::ResourceError * error = <Value unavailable error>)+0x390 [C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\loader\ResourceLoader.cpp @ 665]

The reason for the crash is due to Optional<ReadableStreamDefaultController> m_controller (used in controller()) being uninitialized here:

void FetchBodySource::error(const Exception& value)
{
    controller().error(value);
    clean();
    m_bodyOwner = nullptr;
}

I'll file a separate bug to handle this since it's a different issue.

[Mark Lam]

Filed https://bugs.webkit.org/show_bug.cgi?id=213917 to handle the remaining crash.

[Mark Lam]

Landed in r263883: <http://trac.webkit.org/r263883>.

[Radar WebKit Bug Importer]

<rdar://problem/65064585>

[Fujii Hironori]

Comment on attachment 403384 [details]
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.

The @setupReadableStreamDefaultController issue is tracked by Bug 213792.