Bug 197485

Summary: ASSERTION FAILED: !m_needExceptionCheck with --validateExceptionChecks=1; ProxyObject.getOwnPropertySlotCommon/JSFunction.callerGetter
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: commit-queue, ews-watchlist, keith_miller, mark.lam, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
saam: review-
Updated patch responding to review comments
none
Refined the patch a little more none

Michael Saboff
Reported 2019-05-01 17:46:20 PDT
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
Attachments
Patch (4.20 KB, patch)
2019-05-01 17:54 PDT, Michael Saboff 		saam: review-
DetailsFormatted DiffDiff
Updated patch responding to review comments (3.33 KB, patch)
2019-05-01 18:24 PDT, Michael Saboff 		no flags
DetailsFormatted DiffDiff
Refined the patch a little more (3.37 KB, patch)
2019-05-01 18:37 PDT, Michael Saboff 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Michael Saboff
Comment 1 2019-05-01 17:46:31 PDT
<rdar://problem/49693909>
Michael Saboff
Comment 2 2019-05-01 17:54:32 PDT
Created attachment 368740 [details] Patch
Saam Barati
Comment 3 2019-05-01 18:00:00 PDT
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 4 2019-05-01 18:01:44 PDT
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
Comment 5 2019-05-01 18:24:26 PDT
Created attachment 368745 [details] Updated patch responding to review comments
Michael Saboff
Comment 6 2019-05-01 18:37:02 PDT
Created attachment 368747 [details] Refined the patch a little more
WebKit Commit Bot
Comment 7 2019-05-01 19:04:12 PDT
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
Comment 8 2019-05-01 19:04:13 PDT
All reviewed patches have been landed. Closing bug.
Michael Saboff
Comment 9 2019-05-02 09:06:47 PDT
This change broke other tests. Rolling out...
Michael Saboff
Comment 10 2019-05-02 09:18:29 PDT
Rolled out in r244872: <https://trac.webkit.org/changeset/244872>
Michael Saboff
Comment 11 2019-05-22 13:49:58 PDT
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 ***
Note You need to log in before you can comment on or make changes to this bug.