JSMainThreadExecState::call() should clear exceptions before returning.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 11 Apr 2014 20:24:56 +0000 (20:24 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 11 Apr 2014 20:24:56 +0000 (20:24 +0000)
commitf6eceea7b731e08713541c0317681f8504d09929
tree683d749a87a2c0754b5d3a2489c20dba6ff89cf7
parent28cbf1009c3c2ea4b0dd7e99267d863ac8834ee9
JSMainThreadExecState::call() should clear exceptions before returning.
<https://webkit.org/b/131530>

Reviewed by Geoffrey Garen.

Source/JavaScriptCore:

Added a version of JSC::call() that return any uncaught exception instead
of leaving it pending in the VM.

As part of this change, I updated various parts of the code base to use the
new API as needed.

* bindings/ScriptFunctionCall.cpp:
(Deprecated::ScriptFunctionCall::call):
- ScriptFunctionCall::call() is only used by the inspector to inject scripts.
  The injected scripts that will include Inspector scripts that should catch
  and handle any exceptions that were thrown.  We should not be seeing any
  exceptions returned from this call.  However, we do have checks for
  exceptions in case there are bugs in the Inspector scripts which allowed
  the exception to leak through.  Hence, it is proper to clear the exception
  here, and only record the fact that an exception was seen (if present).

* bindings/ScriptFunctionCall.h:
* inspector/InspectorEnvironment.h:
* runtime/CallData.cpp:
(JSC::call):
* runtime/CallData.h:

Source/WebCore:

Test: fast/dom/regress-131530.html

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

We now change JSMainThreadExecState::call() to return the exception
(if present) via an argument, and clear it from the VM before returning.

As part of this change, I updated various parts of the code base to use the
new API as needed.

* bindings/js/JSCallbackData.cpp:
(WebCore::JSCallbackData::invokeCallback):
* bindings/js/JSCustomXPathNSResolver.cpp:
(WebCore::JSCustomXPathNSResolver::lookupNamespaceURI):
* bindings/js/JSDOMGlobalObjectTask.cpp:
- Assert that there's no unhandled exception after the Microtask returns.
  See comment for WebCore::JSMainThreadExecState::runTask below for more
  details.

* bindings/js/JSErrorHandler.cpp:
(WebCore::JSErrorHandler::handleEvent):
* bindings/js/JSEventListener.cpp:
(WebCore::JSEventListener::handleEvent):
* bindings/js/JSHTMLDocumentCustom.cpp:
(WebCore::JSHTMLDocument::open):
- Document.open() cannot be the first function on the JS stack.  Hence,
  there is no need to use JSMainThreadExecState to call into the VM, as
  this is only needed to catch the event of returning from the first
  function for the purpose of notifying MutationObservers.  Change to
  call JSC::call() directly.

* bindings/js/JSMainThreadExecState.cpp:
(WebCore::functionCallHandlerFromAnyThread):
* bindings/js/JSMainThreadExecState.h:
(WebCore::JSMainThreadExecState::call):
(WebCore::JSMainThreadExecState::evaluate):
- Remove the explicitly acquisition of the JSLock here because we now
  acquire the JSLock as part of the JSMainThreadExecState instance.
(WebCore::JSMainThreadExecState::runTask):
- Added an assert to verify that the task does not return with an
  unhandled exception.  Currently, the only Microtask in use is for the
  Promise implementation, which will eat the exception before returning.
  This assertion is added here to verify that this contract does not
  inadvertantly change in the future.
(WebCore::JSMainThreadExecState::JSMainThreadExecState):
- Now acquires the JSLock as well since by definition, we're only
  instantiating the JSMainThreadExecState because we're about to enter
  the VM.

* bindings/js/JSMutationCallback.cpp:
(WebCore::JSMutationCallback::call):
* bindings/js/JSNodeFilterCondition.cpp:
(WebCore::JSNodeFilterCondition::acceptNode):
- acceptNode() is only used in the TreeWalker and NodeIterator APIs which
  cannot be the first function on the JS stack.  Hence, we should call
  JSC::call() directly instead of going through JSMainThreadExecState.

* bindings/js/ScheduledAction.cpp:
(WebCore::ScheduledAction::executeFunctionInContext):
* bindings/objc/WebScriptObject.mm:
(WebCore::addExceptionToConsole):
(-[WebScriptObject callWebScriptMethod:withArguments:]):

LayoutTests:

* fast/dom/regress-131530-expected.txt: Added.
* fast/dom/regress-131530.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@167142 268f45cc-cd09-0410-ab3c-d52691b4dbfc
22 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/dom/regress-131530-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/regress-131530.html [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bindings/ScriptFunctionCall.cpp
Source/JavaScriptCore/bindings/ScriptFunctionCall.h
Source/JavaScriptCore/inspector/InspectorEnvironment.h
Source/JavaScriptCore/runtime/CallData.cpp
Source/JavaScriptCore/runtime/CallData.h
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSCallbackData.cpp
Source/WebCore/bindings/js/JSCustomXPathNSResolver.cpp
Source/WebCore/bindings/js/JSDOMGlobalObjectTask.cpp
Source/WebCore/bindings/js/JSErrorHandler.cpp
Source/WebCore/bindings/js/JSEventListener.cpp
Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp
Source/WebCore/bindings/js/JSMainThreadExecState.cpp
Source/WebCore/bindings/js/JSMainThreadExecState.h
Source/WebCore/bindings/js/JSMutationCallback.cpp
Source/WebCore/bindings/js/JSNodeFilterCondition.cpp
Source/WebCore/bindings/js/ScheduledAction.cpp
Source/WebCore/bindings/objc/WebScriptObject.mm