ASSERTION FAILED: !m_needExceptionCheck with --validateExceptionChecks=1; ProxyObject.getOwnPropertySlotCommon/JSFunction.callerGetter

[Michael Saboff]

When run with a debug version of JSC and the --validateExceptionChecks option set true, the following crashes:

function foo() {}
let a = {...new Proxy(foo, {})}

ERROR: Unchecked JS exception: 
    This scope can throw a JS exception: getOwnPropertySlotCommon @ ./runtime/ProxyObject.cpp:376
        (ExceptionScope::m_recursionDepth was 5)
    But the exception was unchecked as of this scope: callerGetter @ ./runtime/JSFunction.cpp:358 
        (ExceptionScope::m_recursionDepth was 5)

Unchecked exception detected at:
    1   0x10c2b8b8e JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation&)
    2   0x10c2982ca JSC::ThrowScope::ThrowScope(JSC::VM&, JSC::ExceptionEventLocation)
    3   0x10c298313 JSC::ThrowScope::ThrowScope(JSC::VM&, JSC::ExceptionEventLocation)
    4   0x10bf9e806 JSC::JSFunction::callerGetter(JSC::ExecState*, long long, JSC::PropertyName)
    5   0x10c0e53ef JSC::PropertySlot::customGetter(JSC::ExecState*, JSC::PropertyName) const
    6   0x10afe1ae1 JSC::PropertySlot::getValue(JSC::ExecState*, JSC::PropertyName) const
    7   0x10c01a224 JSC::JSObject::getOwnPropertyDescriptor(JSC::ExecState*, JSC::PropertyName, JSC::PropertyDescriptor&)
    8   0x10bfb8bab JSC::globalFuncPropertyIsEnumerable(JSC::ExecState*)
    9   0x55792a20116b
    10  0x10af9ee81 llint_entry    
    11  0x10af9ef12 llint_entry    
    12  0x10af8ba30 vmEntryToJavaScript
    13  0x10bbc82b7 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
    14  0x10bbc7880 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
    15  0x10bedb955 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
    16  0x107f06e16 runWithOptions(GlobalObject*, CommandLine&, bool&)
    17  0x107edc56a jscmain(int, char**)::$_6::operator()(JSC::VM&, GlobalObject*, bool&) const
    18  0x107eba2cf int runJSC<jscmain(int, char**)::$_6>(CommandLine const&, bool, jscmain(int, char**)::$_6 const&)
    19  0x107eb8daf jscmain(int, char**)
    20  0x107eb8c1e main
    21  0x7fff57fca0a5 start

ASSERTION FAILED: !m_needExceptionCheck
./runtime/VM.cpp(1203) : void JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation &) 
1   0x10abeacc9 WTFCrash
2   0x10abeda8b WTFCrashWithInfo(int, char const*, char const*, int) 
3   0x10c2b8cb8 JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation&)
4   0x10c2982ca JSC::ThrowScope::ThrowScope(JSC::VM&, JSC::ExceptionEventLocation)
5   0x10c298313 JSC::ThrowScope::ThrowScope(JSC::VM&, JSC::ExceptionEventLocation)
6   0x10bf9e806 JSC::JSFunction::callerGetter(JSC::ExecState*, long long, JSC::PropertyName)
7   0x10c0e53ef JSC::PropertySlot::customGetter(JSC::ExecState*, JSC::PropertyName) const
8   0x10afe1ae1 JSC::PropertySlot::getValue(JSC::ExecState*, JSC::PropertyName) const
9   0x10c01a224 JSC::JSObject::getOwnPropertyDescriptor(JSC::ExecState*, JSC::PropertyName, JSC::PropertyDescriptor&)
10  0x10bfb8bab JSC::globalFuncPropertyIsEnumerable(JSC::ExecState*)
11  0x55792a20116b
12  0x10af9ee81 llint_entry
13  0x10af9ef12 llint_entry
14  0x10af8ba30 vmEntryToJavaScript
15  0x10bbc82b7 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
16  0x10bbc7880 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
17  0x10bedb955 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
18  0x107f06e16 runWithOptions(GlobalObject*, CommandLine&, bool&)
19  0x107edc56a jscmain(int, char**)::$_6::operator()(JSC::VM&, GlobalObject*, bool&) const
20  0x107eba2cf int runJSC<jscmain(int, char**)::$_6>(CommandLine const&, bool, jscmain(int, char**)::$_6 const&)
21  0x107eb8daf jscmain(int, char**)
22  0x107eb8c1e main
23  0x7fff57fca0a5 start

Looks like we aren't properly checking for exceptions up the caller tree of

[Michael Saboff]

<rdar://problem/49693909>

[Michael Saboff]

Created attachment 368740 [details]
Patch

[Saam Barati]

Comment on attachment 368740 [details]
Patch

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

This seems wrong to me. Why can't you just change Proxy's implementation?

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:841
> -    scope.release();
>      PropertyDescriptor descriptor;
>      bool enumerable = object->getOwnPropertyDescriptor(exec, propertyName, descriptor) && descriptor.enumerable();
> +
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());

this seems right. Why change it?

[Yusuke Suzuki]

Comment on attachment 368740 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:841
>> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());
> 
> this seems right. Why change it?

You can use RETURN_IF_EXCEPTION(scope,{ }), but I don't think this change is necessary. getOwnPropertyDescriptor can the only one operation that can cause exception. so `scope.release()` and returning is OK.

> Source/JavaScriptCore/runtime/JSObject.cpp:3493
>              descriptor.setSetter(getCustomGetterSetterFunctionForGetterSetter(exec, propertyName, getterSetter, JSCustomGetterSetterFunction::Type::Setter));

Can getCustomGetterSetterFunctionForGetterSetter throw exception? I think,

if (getterSetter->getter()) {
    auto* getter = getCustomGetterSetterFunctionForGetterSetter(exec, propertyName, getterSetter, JSCustomGetterSetterFunction::Type::Getter);
    RETURN_IF_EXCEPTION(scope, false);
    descriptor.setGetter(getter);
}

is necessary.

> Source/JavaScriptCore/runtime/JSObject.cpp:3495
>          descriptor.setDescriptor(slot.getValue(exec, propertyName), slot.attributes());

And if we do the above thing, making it to,

JSValue value = slot.getValue(exec, propertyName);
RETURN_IF_EXCEPTION(scope, false);
descriptor.setDescriptor(value, slot.attributes());

would be nice.

> Source/JavaScriptCore/runtime/JSObject.cpp:3497
> +    RETURN_IF_EXCEPTION(scope, false);

And we don't need this if the above changes are done.

[Michael Saboff]

Created attachment 368745 [details]
Updated patch responding to review comments

[Michael Saboff]

Created attachment 368747 [details]
Refined the patch a little more

[WebKit Commit Bot]

Comment on attachment 368747 [details]
Refined the patch a little more

Clearing flags on attachment: 368747

Committed r244862: <https://trac.webkit.org/changeset/244862>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Saboff]

This change broke other tests.  Rolling out...

[Michael Saboff]

Rolled out in r244872: <https://trac.webkit.org/changeset/244872>

[Michael Saboff]

This was fixed in the related bug <https://bugs.webkit.org/show_bug.cgi?id=197693>.

*** This bug has been marked as a duplicate of bug 197693 ***