...
Created attachment 403246 [details] proposed patch.
Comment on attachment 403246 [details] proposed patch. Taking out of review. Let's see what the EWS says.
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 &&.
Created attachment 403248 [details] proposed patch.
Created attachment 403382 [details] proposed patch.
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?
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.
Created attachment 403384 [details] proposed patch.
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?
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.
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.
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.
Filed https://bugs.webkit.org/show_bug.cgi?id=213917 to handle the remaining crash.
Landed in r263883: <http://trac.webkit.org/r263883>.
<rdar://problem/65064585>
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.