[webkit-reviews] review denied: [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) : [Attachment 399470] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied youenn fablet <youennf at gmail.com>'s
request for review:
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)
https://bugs.webkit.org/show_bug.cgi?id=211923

Attachment 399470: Patch

https://bugs.webkit.org/attachment.cgi?id=399470&action=review




--- Comment #4 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 399470
  --> https://bugs.webkit.org/attachment.cgi?id=399470
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.