[webkit-reviews] review granted: [Bug 131530] JSMainThreadExecState::call() should clear exceptions before returning : [Attachment 229145] the patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 131530: JSMainThreadExecState::call() should clear exceptions before
returning
https://bugs.webkit.org/show_bug.cgi?id=131530

Attachment 229145: the patch
https://bugs.webkit.org/attachment.cgi?id=229145&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.