Bug 196089

Summary: JSC::createError should clear exception thrown by errorDescriptionForValue
Product: WebKit Reporter: Tadeu Zagallo <tzagallo>
Component: JavaScriptCoreAssignee: Tadeu Zagallo <tzagallo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, rniwa, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=196032
https://bugs.webkit.org/show_bug.cgi?id=196305
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews105 for mac-highsierra-wk2
none
Patch for landing
none
Patch for landing
none
Patch for landing none

Description Tadeu Zagallo 2019-03-21 10:32:57 PDT
...
Comment 1 Tadeu Zagallo 2019-03-21 10:39:23 PDT
Created attachment 365566 [details]
Patch
Comment 2 Mark Lam 2019-03-21 12:06:00 PDT
Comment on attachment 365566 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365566&action=review

r=me with fix.

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:279
>      String valueDescription = errorDescriptionForValue(exec, value);
> -    if (!valueDescription)
> +    if (!valueDescription) {
> +        scope.clearException();

I think it's a hack that we return an OOME when we fail to create the requested Error object.  I suspect that this is a semantic error because:

    var error = new RangeError();
    error.toString()l // expects "RangeError", but may get "Out of memory".

Anyway, that was a pre-existing thing and can be fixed later.  For this patch, you also need to placate the exception validator before the call to tryMakeString() below.  The way to do this is to add the following before the "if (!valueDescription)" above:
    ASSERT(scope.exception() || valueDescription);

The assertion tickles scope.exception(), thereby telling the validator that we did something with the exception.  This placates the validator.  Note: the assertion assumes that we will never return a null string unless there's no exception.  If that's not the case, just do a real exception check instead.
Comment 3 EWS Watchlist 2019-03-21 12:48:34 PDT
Comment on attachment 365566 [details]
Patch

Attachment 365566 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11602569

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html
Comment 4 EWS Watchlist 2019-03-21 12:48:36 PDT
Created attachment 365596 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 5 Tadeu Zagallo 2019-03-21 13:47:51 PDT
Created attachment 365608 [details]
Patch for landing
Comment 6 Mark Lam 2019-03-21 13:50:36 PDT
Comment on attachment 365608 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=365608&action=review

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:278
> +    ASSERT(scope.exception() || valueDescription);

You also need to clear the exception because you’re in a CatchScope here.
Comment 7 Tadeu Zagallo 2019-03-21 14:07:03 PDT
Created attachment 365615 [details]
Patch for landing
Comment 8 Tadeu Zagallo 2019-03-21 14:37:45 PDT
Created attachment 365619 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2019-03-21 15:05:02 PDT
Comment on attachment 365619 [details]
Patch for landing

Clearing flags on attachment: 365619

Committed r243335: <https://trac.webkit.org/changeset/243335>
Comment 10 WebKit Commit Bot 2019-03-21 15:05:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-03-21 15:21:01 PDT
<rdar://problem/49126673>