Created attachment 399411 [details] stream-safe-creation.any-crash-log imported/w3c/web-platform-tests/fetch/api/basic/stream-safe-creation.any.html Description: This test is flaky crashing on Mac wk1 Debug. This test appears to be flaky crashing throughout the visual history, the earliest crash that I see is on 11/05/2019. History: https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Ffetch%2Fapi%2Fbasic%2Fstream-safe-creation.any.html&platform=mac&style=debug&flavor=wk1&limit=50000 Crash log: Thread 33 Crashed:: WebCore: Worker 0 com.apple.JavaScriptCore 0x00000001034e8070 WTFCrash + 16 (Assertions.cpp:303) 1 com.apple.WebCore 0x000000011a759cbb WTFCrashWithInfo(int, char const*, char const*, int) + 27 2 com.apple.WebCore 0x000000011aa7cb16 JSC::JSObject::get(JSC::JSGlobalObject*, JSC::PropertyName) const + 342 (JSObject.h:1493) 3 com.apple.WebCore 0x000000011cc69e4c WebCore::ReadableStreamDefaultController::invoke(JSC::JSGlobalObject&, JSC::JSObject&, char const*, JSC::JSValue) + 252 (ReadableStreamDefaultController.cpp:56) 4 com.apple.WebCore 0x000000011c265dc4 WebCore::ReadableStreamDefaultController::error(JSC::JSGlobalObject&, JSC::JSValue) + 68 (ReadableStreamDefaultController.h:57) 5 com.apple.WebCore 0x000000011c263a1d WebCore::ReadableStreamDefaultController::error(WebCore::Exception const&) + 125 (ReadableStreamDefaultController.h:109) 6 com.apple.WebCore 0x000000011c25e82c WebCore::FetchBodySource::error(WebCore::Exception const&) + 44 (FetchBodySource.cpp:94) 7 com.apple.WebCore 0x000000011c28a6c0 WebCore::FetchResponse::BodyLoader::didFail(WebCore::ResourceError const&) + 736 (FetchResponse.cpp:306) 8 com.apple.WebCore 0x000000011c2859cd WebCore::FetchLoader::didFail(WebCore::ResourceError const&) + 45 (FetchLoader.cpp:167) 9 com.apple.WebCore 0x000000011dde067c WebCore::ThreadableLoaderClientWrapper::didFail(WebCore::ResourceError const&) + 60 (ThreadableLoaderClientWrapper.h:87) 10 com.apple.WebCore 0x000000011dddfaef WebCore::WorkerThreadableLoader::MainThreadBridge::cancel() + 191 (WorkerThreadableLoader.cpp:182) 11 com.apple.WebCore 0x000000011dddfa29 WebCore::WorkerThreadableLoader::cancel() + 25 (WorkerThreadableLoader.cpp:86) 12 com.apple.WebCore 0x000000011c2857b9 WebCore::FetchLoader::stop() + 105 (FetchLoader.cpp:135) 13 com.apple.WebCore 0x000000011c28b2c0 WebCore::FetchResponse::BodyLoader::stop() + 96 (FetchResponse.cpp:400) 14 com.apple.WebCore 0x000000011c28b20c WebCore::FetchResponse::BodyLoader::didReceiveData(char const*, unsigned long) + 652 (FetchResponse.cpp:378) 15 com.apple.WebCore 0x000000011c285947 WebCore::FetchLoader::didReceiveData(char const*, int) + 71 (FetchLoader.cpp:154) 16 com.apple.WebCore 0x000000011de009d6 WebCore::ThreadableLoaderClientWrapper::didReceiveData(char const*, int) + 70 (ThreadableLoaderClientWrapper.h:73) 17 com.apple.WebCore 0x000000011de00934 WebCore::WorkerThreadableLoader::MainThreadBridge::didReceiveData(char const*, int)::$_13::operator()(WebCore::ScriptExecutionContext&) + 180 (WorkerThreadableLoader.cpp:215)
<rdar://problem/63244249>
I have marked this test as flaky crashing while this issue is investigated. https://trac.webkit.org/changeset/261710/webkit
Created attachment 399470 [details] Patch
Comment on attachment 399470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399470&action=review > Source/JavaScriptCore/ChangeLog:11 > + When calling get, we either do not allow any exception or allow terminate exceptions, we can happen in workers. /we can happen/which can happen/ > Source/JavaScriptCore/runtime/JSObject.h:76 > +JS_EXPORT_PRIVATE bool isTerminatedExecutionException(VM&, Exception*); You don't need this (see below). > Source/JavaScriptCore/runtime/JSObject.h:1494 > + EXCEPTION_ASSERT(!scope.exception() || isTerminatedExecutionException(vm, scope.exception()) || !hasProperty); I don't think this is the right fix. The assertion is saying that if hasProperty is set, we expect no exception to be thrown, which is turning out to not always be true. The correct fix here is to replace this assertion with: RETURN_IF_EXCEPTION(scope, { }); The point of the exception check is to make sure that we don't execute more code while an exception is pending. > Source/JavaScriptCore/runtime/JSObject.h:1507 > + EXCEPTION_ASSERT(!scope.exception() || isTerminatedExecutionException(vm, scope.exception()) || !hasProperty); Ditto.
> > Source/JavaScriptCore/runtime/JSObject.h:1494 > > + EXCEPTION_ASSERT(!scope.exception() || isTerminatedExecutionException(vm, scope.exception()) || !hasProperty); > > I don't think this is the right fix. The assertion is saying that if > hasProperty is set, we expect no exception to be thrown, which is turning > out to not always be true. The correct fix here is to replace this > assertion with: > RETURN_IF_EXCEPTION(scope, { }); But we still probably want to ASSERT that either there is no exception or this is a terminate exception.
Created attachment 399479 [details] Patch
(In reply to youenn fablet from comment #6) > Created attachment 399479 [details] > Patch Patch with the RETURN_IF_EXCEPTION. It still makes sense to me to put an ASSERT to check for terminated but this patch does not include it.
(In reply to youenn fablet from comment #7) > (In reply to youenn fablet from comment #6) > > Created attachment 399479 [details] > > Patch > > Patch with the RETURN_IF_EXCEPTION. > It still makes sense to me to put an ASSERT to check for terminated but this > patch does not include it. You're right. The ASSERT is informative, and there's no harm in having it in addition to the RETURN_IF_EXCEPTION.
Comment on attachment 399479 [details] Patch r=me. Please add back the EXCEPTION_ASSERT before the RETURN_IF_EXCEPTION. Sorry for not thinking of that earlier.
Created attachment 399482 [details] Patch for landing
Created attachment 399729 [details] Patch for landing
Test failures fixed by returning jsUndefined instead of JSValue in exception case.
Committed r261857: <https://trac.webkit.org/changeset/261857> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399729 [details].