Bug 131530

Summary: JSMainThreadExecState::call() should clear exceptions before returning
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: 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 Flags
the patch ggaren: review+

Description Mark Lam 2014-04-10 22:24:36 PDT
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.
Comment 1 Mark Lam 2014-04-10 22:25:04 PDT
<rdar://problem/16525609>
Comment 2 Mark Lam 2014-04-11 11:44:45 PDT
Created attachment 229145 [details]
the patch
Comment 3 Geoffrey Garen 2014-04-11 12:08:16 PDT
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.
Comment 4 Mark Lam 2014-04-11 13:26:18 PDT
Thanks for the review.  The patch has been updated with the feedback, and landed in r167142: <http://trac.webkit.org/r167142>.