| Summary: | [ 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) | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jason Lawrence <Lawrence.j> | ||||||||||||
| Component: | Platform | Assignee: | youenn fablet <youennf> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bot-watchers-bugzilla, webkit-bug-importer, youennf, ysuzuki | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Mac | ||||||||||||||
| OS: | macOS 10.14 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Jason Lawrence
2020-05-14 14:35:10 PDT
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]. |