Bug 164968 – Fix exception scope verification failures in jsc.cpp.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Patch coming.

[Mark Lam]

Created attachment 295213 [details]
proposed patch.

[Mark Lam]

Created attachment 295294 [details]
proposed patch with more fixes.

[Darin Adler]

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.

[Mark Lam]

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.

[Mark Lam]

Created attachment 295302 [details]
Updated patch.

Still need to look into the use of RELEASE_ASSERTs, and see if we can do better.

[Mark Lam]

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.

[Mark Lam]

Created attachment 295418 [details]
proposed patch.

[Saam Barati]

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

[Mark Lam]

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.

[Mark Lam]

Created attachment 304546 [details]
re-based patch for landing.

[Mark Lam]

Created attachment 304553 [details]
re-based patch for landing: w/ build fix for release builds.

[Mark Lam]

Thanks for the reviews.  Landed in r214016: <http://trac.webkit.org/r214016>.