Bug 131530 – JSMainThreadExecState::call() should clear exceptions before returning

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

[Mark Lam]

Currently, JSMainThreadExecState::call() does not clear any pending exceptions in the VM before returning.  On returning, the JSMainThreadExecState destructor may re-enter the VM to notify MutationObservers.  This can result in a crash because the VM expects exceptions to be cleared at entry.

[Mark Lam]

<rdar://problem/16525609>

[Mark Lam]

Created attachment 229145 [details]
the patch

[Geoffrey Garen]

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.

[Mark Lam]

Thanks for the review.  The patch has been updated with the feedback, and landed in r167142: <http://trac.webkit.org/r167142>.