Bug 211923 – [ Mac wk1 Debug ] imported/w3c/web-platform-tests/fetch/api/basic/stream-safe-creation.any.html is flaky crashing with alerts - WTFCrashWithInfo - SC::JSObject::get(JSC::JSGlobalObject*, JSC::PropertyName)

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Jason Lawrence]

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)

[Radar WebKit Bug Importer]

<rdar://problem/63244249>

[Jason Lawrence]

I have marked this test as flaky crashing while this issue is investigated.
https://trac.webkit.org/changeset/261710/webkit

[youenn fablet]

Created attachment 399470 [details]
Patch

[Mark Lam]

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.

[youenn fablet]

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

[youenn fablet]

Created attachment 399479 [details]
Patch

[youenn fablet]

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

[Mark Lam]

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

[Mark Lam]

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.

[youenn fablet]

Created attachment 399482 [details]
Patch for landing

[youenn fablet]

Created attachment 399729 [details]
Patch for landing

[youenn fablet]

Test failures fixed by returning jsUndefined instead of JSValue in exception case.

[EWS]

Committed r261857: <https://trac.webkit.org/changeset/261857>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399729 [details].