Summary: | JSMainThreadExecState::call() should clear exceptions before returning | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | commit-queue, fpizlo, ggaren, mhahnenberg, mkwst, mmirman, msaboff, oliver | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Mark Lam
2014-04-10 22:24:36 PDT
Created attachment 229145 [details]
the patch
Comment on attachment 229145 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=229145&action=review r=me > Source/JavaScriptCore/bindings/ScriptFunctionCall.cpp:145 > + JSValue returnedException; > if (m_callHandler) > - result = m_callHandler(m_exec, function, callType, callData, thisObject, m_arguments); > + result = m_callHandler(m_exec, function, callType, callData, thisObject, m_arguments, &returnedException); > else > - result = JSC::call(m_exec, function, callType, callData, thisObject, m_arguments); > + result = JSC::call(m_exec, function, callType, callData, thisObject, m_arguments, &returnedException); > > - if (m_exec->hadException()) { > + if (returnedException) { > hadException = true; > return Deprecated::ScriptValue(); > } Previously, this function did not clear the VM's exception. Now, it does. Was that behavior change intentional? If so, you should explain your intent in the ChangeLog, and add a test for this case to your test case. > Source/JavaScriptCore/bindings/ScriptFunctionCall.h:74 > + typedef JSC::JSValue (*ScriptFunctionCallHandler)(JSC::ExecState* exec, JSC::JSValue functionObject, JSC::CallType callType, const JSC::CallData& callData, JSC::JSValue thisValue, const JSC::ArgList& args, JSC::JSValue* returnedException); Let's just call it "exception". > Source/JavaScriptCore/inspector/InspectorEnvironment.h:37 > +typedef JSC::JSValue (*InspectorFunctionCallHandler)(JSC::ExecState* exec, JSC::JSValue functionObject, JSC::CallType callType, const JSC::CallData& callData, JSC::JSValue thisValue, const JSC::ArgList& args, JSC::JSValue* exception); Ditto. > Source/JavaScriptCore/runtime/CallData.cpp:42 > +JSValue call(ExecState* exec, JSValue functionObject, CallType callType, const CallData& callData, JSValue thisValue, const ArgList& args, JSValue* returnedException) Ditto. > Source/WebCore/ChangeLog:64 > + * bindings/js/JSNodeFilterCondition.cpp: > + (WebCore::JSNodeFilterCondition::acceptNode): Needs a comment explaining why FilterCondition wants to pass through its exception. > Source/WebCore/bindings/js/JSMainThreadExecState.cpp:49 > +JSC::JSValue functionCallHandlerFromAnyThread(JSC::ExecState* exec, JSC::JSValue functionObject, JSC::CallType callType, const JSC::CallData& callData, JSC::JSValue thisValue, const JSC::ArgList& args, JSC::JSValue* returnedException) Ditto. > Source/WebCore/bindings/js/JSMainThreadExecState.h:69 > + ASSERT(!exec->hadException()); Let's move this ASSERT to ~JSMainThreadExecState, to catch all cases. Thanks for the review. The patch has been updated with the feedback, and landed in r167142: <http://trac.webkit.org/r167142>. |