Bug 164968 - Fix exception scope verification failures in jsc.cpp.
Summary: Fix exception scope verification failures in jsc.cpp.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks: 162351
  Show dependency treegraph
 
Reported: 2016-11-18 16:23 PST by Mark Lam
Modified: 2017-03-15 15:50 PDT (History)
8 users (show)

See Also:


Attachments
proposed patch. (6.49 KB, patch)
2016-11-18 16:29 PST, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch with more fixes. (8.49 KB, patch)
2016-11-21 08:31 PST, Mark Lam
darin: review+
Details | Formatted Diff | Diff
Updated patch. (8.38 KB, patch)
2016-11-21 11:03 PST, Mark Lam
mark.lam: review-
Details | Formatted Diff | Diff
proposed patch. (8.48 KB, patch)
2016-11-24 13:56 PST, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff
re-based patch for landing. (7.73 KB, patch)
2017-03-15 14:20 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
re-based patch for landing: w/ build fix for release builds. (7.75 KB, patch)
2017-03-15 14:55 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-11-18 16:23:51 PST
Patch coming.
Comment 1 Mark Lam 2016-11-18 16:29:19 PST
Created attachment 295213 [details]
proposed patch.
Comment 2 Mark Lam 2016-11-21 08:31:15 PST
Created attachment 295294 [details]
proposed patch with more fixes.
Comment 3 Darin Adler 2016-11-21 09:05:06 PST
Comment on attachment 295294 [details]
proposed patch with more fixes.

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

> Source/JavaScriptCore/jsc.cpp:385
> +        RETURN_IF_EXCEPTION(scope, encodedJSValue());

Too late since we already have so may of these, but I really prefer to use "{ }" in cases like this one rather than "encodedJSValue()".

> Source/JavaScriptCore/jsc.cpp:1443
> +    RELEASE_ASSERT(!scope.exception());

I saw the comment about out of memory, but is this really the right thing to do? This is a command line tool, so shouldn't we write something out to stderr and then exit instead of aborting? Seems more elegant to me. In all the places where the code now says RELEASE_ASSERT.

> Source/JavaScriptCore/jsc.cpp:2508
> +    RETURN_IF_EXCEPTION(scope, JSValue());

Same thing would work here, using { } instead of JSValue().

In fact, if we made a version of the RETURN_IF_EXCEPTION macro that used { }, we could use it in functions that return JSValue, EncodedJSValue, and any pointer type. We should really do that. The only place we couldn’t use it is in functions that return void, I think.
Comment 4 Mark Lam 2016-11-21 11:02:00 PST
Comment on attachment 295294 [details]
proposed patch with more fixes.

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

>> Source/JavaScriptCore/jsc.cpp:1443
>> +    RELEASE_ASSERT(!scope.exception());
> 
> I saw the comment about out of memory, but is this really the right thing to do? This is a command line tool, so shouldn't we write something out to stderr and then exit instead of aborting? Seems more elegant to me. In all the places where the code now says RELEASE_ASSERT.

I'll defer landing this patch until I can do more analysis on this issue, and see if we can do better.

>> Source/JavaScriptCore/jsc.cpp:2508
>> +    RETURN_IF_EXCEPTION(scope, JSValue());
> 
> Same thing would work here, using { } instead of JSValue().
> 
> In fact, if we made a version of the RETURN_IF_EXCEPTION macro that used { }, we could use it in functions that return JSValue, EncodedJSValue, and any pointer type. We should really do that. The only place we couldn’t use it is in functions that return void, I think.

I'll change these to use { }.  We had entertained having different versions of the RETURN_IF_EXCEPTION() macro before for different return types (2 at the time), but opted to go with just 1 macro for consistency.  If we go with a special macro for returning { }, then we'll end up with 3 macros: 1 for void, 1 for { }, and 1 for anything else (there are a few places where we need that).  I'll stick with the 1 RETURN_IF_EXCEPTION() macro that we have currently for now.
Comment 5 Mark Lam 2016-11-21 11:03:17 PST
Created attachment 295302 [details]
Updated patch.

Still need to look into the use of RELEASE_ASSERTs, and see if we can do better.
Comment 6 Mark Lam 2016-11-24 13:46:11 PST
Comment on attachment 295302 [details]
Updated patch.

It is invalid to replace returning encodedJSValue() with returning { }.  On 32-bit builds, the former is non-zero, while the latter is 0.  Will fix this patch.
Comment 7 Mark Lam 2016-11-24 13:56:24 PST
Created attachment 295418 [details]
proposed patch.
Comment 8 Saam Barati 2016-11-28 14:09:36 PST
Comment on attachment 295418 [details]
proposed patch.

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

> Source/JavaScriptCore/jsc.cpp:2510
>      JSString* type = jsCast<JSString*>(wasmValue.get(exec, makeIdentifier(vm, "type")));
> +    RETURN_IF_EXCEPTION(scope, { });
>      JSValue value = wasmValue.get(exec, makeIdentifier(vm, "value"));
> +    RETURN_IF_EXCEPTION(scope, { });

I'd make these assertions since they're for testing WASM
Comment 9 Mark Lam 2017-03-15 14:18:28 PDT
Comment on attachment 295418 [details]
proposed patch.

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

>> Source/JavaScriptCore/jsc.cpp:2510
>> +    RETURN_IF_EXCEPTION(scope, { });
> 
> I'd make these assertions since they're for testing WASM

I've applied this change.

> Source/JavaScriptCore/jsc.cpp:2590
> +            for (unsigned argIndex = 0; argIndex < arguments->length(); ++argIndex) {
> +                JSValue boxedValue = box(exec, vm, arguments->getIndexQuickly(argIndex));
> +                RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +                boxedArgs.append(boxedValue);
> +            }

Since we've changed box() to never throw but to assert no exceptions instead, this change is now unnecessary.

> Source/JavaScriptCore/jsc.cpp:2594
> +            RETURN_IF_EXCEPTION(scope, encodedJSValue());

Ditto.  This can be removed too.
Comment 10 Mark Lam 2017-03-15 14:20:20 PDT
Created attachment 304546 [details]
re-based patch for landing.
Comment 11 Mark Lam 2017-03-15 14:55:15 PDT
Created attachment 304553 [details]
re-based patch for landing: w/ build fix for release builds.
Comment 12 Mark Lam 2017-03-15 15:50:43 PDT
Thanks for the reviews.  Landed in r214016: <http://trac.webkit.org/r214016>.