From f93467b10e06b1e988c4ac37cad82c7d04c05023 Mon Sep 17 00:00:00 2001 From: "adamk@chromium.org" Date: Thu, 9 Feb 2012 03:34:29 +0000 Subject: [PATCH] DOM mutations should not be delivered on worker threads https://bugs.webkit.org/show_bug.cgi?id=77898 Reviewed by Dmitry Titov. Source/WebCore: In V8RecursionScope, only call WebKitMutationObserver::deliverAllMutations if in a Document context. This is accomplished through a change to V8Proxy::instrumentedCallFunction (which now takes a Frame* instead of a Page*), requiring an update to all callers of that function (accounting for the majority of files changed in this patch). Added ASSERT(isMainThread()) in a deliverAllMutations to confirm that it's no longer called on worker threads, and in enqueueMutationRecord, where the same global store of active observers is accessed. See also http://crbug.com/112586, where the problem was initially reported. * bindings/v8/ScriptFunctionCall.cpp: (WebCore::ScriptCallback::call): * bindings/v8/V8NodeFilterCondition.cpp: (WebCore::V8NodeFilterCondition::acceptNode): * bindings/v8/V8Proxy.cpp: (WebCore::V8Proxy::runScript): (WebCore::V8Proxy::callFunction): (WebCore::V8Proxy::instrumentedCallFunction): * bindings/v8/V8Proxy.h: (WebCore): (V8Proxy): * bindings/v8/V8RecursionScope.cpp: (WebCore::V8RecursionScope::didLeaveScriptContext): * bindings/v8/V8RecursionScope.h: (WebCore): (WebCore::V8RecursionScope::V8RecursionScope): (V8RecursionScope): (WebCore::V8RecursionScope::~V8RecursionScope): * bindings/v8/V8WindowErrorHandler.cpp: (WebCore::V8WindowErrorHandler::callListenerFunction): * bindings/v8/custom/V8CustomVoidCallback.cpp: (WebCore::invokeCallback): * bindings/v8/custom/V8CustomXPathNSResolver.cpp: (WebCore::V8CustomXPathNSResolver::lookupNamespaceURI): * dom/WebKitMutationObserver.cpp: (WebCore::WebKitMutationObserver::enqueueMutationRecord): (WebCore::WebKitMutationObserver::deliverAllMutations): Source/WebKit/chromium: * src/WebDevToolsFrontendImpl.cpp: (WebKit::WebDevToolsFrontendImpl::dispatchOnInspectorFrontend): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@107170 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- Source/WebCore/ChangeLog | 50 ++++++++++++++++++++++ Source/WebCore/bindings/v8/ScriptFunctionCall.cpp | 2 +- .../WebCore/bindings/v8/V8NodeFilterCondition.cpp | 2 +- Source/WebCore/bindings/v8/V8Proxy.cpp | 12 +++--- Source/WebCore/bindings/v8/V8Proxy.h | 3 +- Source/WebCore/bindings/v8/V8RecursionScope.cpp | 6 ++- Source/WebCore/bindings/v8/V8RecursionScope.h | 15 +++++-- .../WebCore/bindings/v8/V8WindowErrorHandler.cpp | 2 +- .../bindings/v8/custom/V8CustomVoidCallback.cpp | 4 +- .../bindings/v8/custom/V8CustomXPathNSResolver.cpp | 2 +- Source/WebCore/dom/WebKitMutationObserver.cpp | 3 ++ Source/WebKit/chromium/ChangeLog | 10 +++++ .../chromium/src/WebDevToolsFrontendImpl.cpp | 2 +- 13 files changed, 93 insertions(+), 20 deletions(-) diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index af2ff69..3271eb4 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,53 @@ +2012-02-08 Adam Klein + + DOM mutations should not be delivered on worker threads + https://bugs.webkit.org/show_bug.cgi?id=77898 + + Reviewed by Dmitry Titov. + + In V8RecursionScope, only call WebKitMutationObserver::deliverAllMutations + if in a Document context. + + This is accomplished through a change to V8Proxy::instrumentedCallFunction + (which now takes a Frame* instead of a Page*), requiring an update to all + callers of that function (accounting for the majority of files changed + in this patch). + + Added ASSERT(isMainThread()) in a deliverAllMutations to confirm that + it's no longer called on worker threads, and in enqueueMutationRecord, + where the same global store of active observers is accessed. + + See also http://crbug.com/112586, where the problem was initially + reported. + + * bindings/v8/ScriptFunctionCall.cpp: + (WebCore::ScriptCallback::call): + * bindings/v8/V8NodeFilterCondition.cpp: + (WebCore::V8NodeFilterCondition::acceptNode): + * bindings/v8/V8Proxy.cpp: + (WebCore::V8Proxy::runScript): + (WebCore::V8Proxy::callFunction): + (WebCore::V8Proxy::instrumentedCallFunction): + * bindings/v8/V8Proxy.h: + (WebCore): + (V8Proxy): + * bindings/v8/V8RecursionScope.cpp: + (WebCore::V8RecursionScope::didLeaveScriptContext): + * bindings/v8/V8RecursionScope.h: + (WebCore): + (WebCore::V8RecursionScope::V8RecursionScope): + (V8RecursionScope): + (WebCore::V8RecursionScope::~V8RecursionScope): + * bindings/v8/V8WindowErrorHandler.cpp: + (WebCore::V8WindowErrorHandler::callListenerFunction): + * bindings/v8/custom/V8CustomVoidCallback.cpp: + (WebCore::invokeCallback): + * bindings/v8/custom/V8CustomXPathNSResolver.cpp: + (WebCore::V8CustomXPathNSResolver::lookupNamespaceURI): + * dom/WebKitMutationObserver.cpp: + (WebCore::WebKitMutationObserver::enqueueMutationRecord): + (WebCore::WebKitMutationObserver::deliverAllMutations): + 2012-02-08 Anders Carlsson Don't use the wheel event handler count to track if a page has horizontal scrollbars diff --git a/Source/WebCore/bindings/v8/ScriptFunctionCall.cpp b/Source/WebCore/bindings/v8/ScriptFunctionCall.cpp index a8bda1d..5c96b56 100644 --- a/Source/WebCore/bindings/v8/ScriptFunctionCall.cpp +++ b/Source/WebCore/bindings/v8/ScriptFunctionCall.cpp @@ -197,7 +197,7 @@ ScriptValue ScriptCallback::call(bool& hadException) for (size_t i = 0; i < m_arguments.size(); ++i) args[i] = m_arguments[i].v8Value(); - v8::Handle result = V8Proxy::instrumentedCallFunction(0 /* page */, function, object, m_arguments.size(), args.get()); + v8::Handle result = V8Proxy::instrumentedCallFunction(0 /* frame */, function, object, m_arguments.size(), args.get()); if (exceptionCatcher.HasCaught()) { hadException = true; diff --git a/Source/WebCore/bindings/v8/V8NodeFilterCondition.cpp b/Source/WebCore/bindings/v8/V8NodeFilterCondition.cpp index 2feaf68..2a663e8 100644 --- a/Source/WebCore/bindings/v8/V8NodeFilterCondition.cpp +++ b/Source/WebCore/bindings/v8/V8NodeFilterCondition.cpp @@ -83,7 +83,7 @@ short V8NodeFilterCondition::acceptNode(ScriptState* state, Node* node) const OwnArrayPtr > args = adoptArrayPtr(new v8::Handle[1]); args[0] = toV8(node); - v8::Handle result = V8Proxy::instrumentedCallFunction(0 /* page */, callback, object, 1, args.get()); + v8::Handle result = V8Proxy::instrumentedCallFunction(0 /* frame */, callback, object, 1, args.get()); if (exceptionCatcher.HasCaught()) { state->setException(exceptionCatcher.Exception()); diff --git a/Source/WebCore/bindings/v8/V8Proxy.cpp b/Source/WebCore/bindings/v8/V8Proxy.cpp index d6ab381..376e12f 100644 --- a/Source/WebCore/bindings/v8/V8Proxy.cpp +++ b/Source/WebCore/bindings/v8/V8Proxy.cpp @@ -378,7 +378,7 @@ v8::Local V8Proxy::runScript(v8::Handle script) v8::TryCatch tryCatch; tryCatch.SetVerbose(true); { - V8RecursionScope recursionScope; + V8RecursionScope recursionScope(frame()->document()); result = script->Run(); } @@ -404,10 +404,10 @@ v8::Local V8Proxy::callFunction(v8::Handle function, v8 { // Keep Frame (and therefore ScriptController and V8Proxy) alive. RefPtr protect(frame()); - return V8Proxy::instrumentedCallFunction(m_frame->page(), function, receiver, argc, args); + return V8Proxy::instrumentedCallFunction(frame(), function, receiver, argc, args); } -v8::Local V8Proxy::instrumentedCallFunction(Page* page, v8::Handle function, v8::Handle receiver, int argc, v8::Handle args[]) +v8::Local V8Proxy::instrumentedCallFunction(Frame* frame, v8::Handle function, v8::Handle receiver, int argc, v8::Handle args[]) { V8GCController::checkMemoryUsage(); @@ -415,7 +415,7 @@ v8::Local V8Proxy::instrumentedCallFunction(Page* page, v8::HandleGetScriptOrigin(); @@ -423,12 +423,12 @@ v8::Local V8Proxy::instrumentedCallFunction(Page* page, v8::HandleGetScriptLineNumber() + 1; } - cookie = InspectorInstrumentation::willCallFunction(page, resourceName, lineNumber); + cookie = InspectorInstrumentation::willCallFunction(frame->page(), resourceName, lineNumber); } v8::Local result; { - V8RecursionScope recursionScope; + V8RecursionScope recursionScope(frame ? frame->document() : 0); result = function->Call(receiver, argc, args); } diff --git a/Source/WebCore/bindings/v8/V8Proxy.h b/Source/WebCore/bindings/v8/V8Proxy.h index 176a84d..fe49563 100644 --- a/Source/WebCore/bindings/v8/V8Proxy.h +++ b/Source/WebCore/bindings/v8/V8Proxy.h @@ -57,7 +57,6 @@ namespace WebCore { class DOMWindow; class Frame; class Node; - class Page; class ScriptExecutionContext; class ScriptSourceCode; class SecurityOrigin; @@ -162,7 +161,7 @@ namespace WebCore { v8::Local callFunction(v8::Handle, v8::Handle, int argc, v8::Handle argv[]); // call the function with the given receiver and arguments and report times to DevTools. - static v8::Local instrumentedCallFunction(Page*, v8::Handle, v8::Handle receiver, int argc, v8::Handle args[]); + static v8::Local instrumentedCallFunction(Frame*, v8::Handle, v8::Handle receiver, int argc, v8::Handle args[]); // Call the function as constructor with the given arguments. v8::Local newInstance(v8::Handle, int argc, v8::Handle argv[]); diff --git a/Source/WebCore/bindings/v8/V8RecursionScope.cpp b/Source/WebCore/bindings/v8/V8RecursionScope.cpp index 3564fcc..cad7498 100644 --- a/Source/WebCore/bindings/v8/V8RecursionScope.cpp +++ b/Source/WebCore/bindings/v8/V8RecursionScope.cpp @@ -32,11 +32,12 @@ #include "V8RecursionScope.h" #include "IDBPendingTransactionMonitor.h" +#include "ScriptExecutionContext.h" #include "WebKitMutationObserver.h" namespace WebCore { -void V8RecursionScope::didLeaveScriptContext() +void V8RecursionScope::didLeaveScriptContext(ScriptExecutionContext* context) { // FIXME: Instrument any work that takes place when script exits to c++ (e.g. Mutation Observers). @@ -48,7 +49,8 @@ void V8RecursionScope::didLeaveScriptContext() #endif #if ENABLE(MUTATION_OBSERVERS) - WebKitMutationObserver::deliverAllMutations(); + if (context && context->isDocument()) + WebKitMutationObserver::deliverAllMutations(); #endif } diff --git a/Source/WebCore/bindings/v8/V8RecursionScope.h b/Source/WebCore/bindings/v8/V8RecursionScope.h index 38aae85..6cfbbab 100644 --- a/Source/WebCore/bindings/v8/V8RecursionScope.h +++ b/Source/WebCore/bindings/v8/V8RecursionScope.h @@ -35,20 +35,29 @@ namespace WebCore { +class ScriptExecutionContext; + class V8RecursionScope { WTF_MAKE_NONCOPYABLE(V8RecursionScope); public: - V8RecursionScope() { V8BindingPerIsolateData::current()->incrementRecursionLevel(); } + explicit V8RecursionScope(ScriptExecutionContext* context) + : m_context(context) + { + V8BindingPerIsolateData::current()->incrementRecursionLevel(); + } + ~V8RecursionScope() { if (!V8BindingPerIsolateData::current()->decrementRecursionLevel()) - didLeaveScriptContext(); + didLeaveScriptContext(m_context); } static int recursionLevel() { return V8BindingPerIsolateData::current()->recursionLevel(); } private: - static void didLeaveScriptContext(); + static void didLeaveScriptContext(ScriptExecutionContext*); + + ScriptExecutionContext* m_context; }; } // namespace WebCore diff --git a/Source/WebCore/bindings/v8/V8WindowErrorHandler.cpp b/Source/WebCore/bindings/v8/V8WindowErrorHandler.cpp index 797c624..2379723 100644 --- a/Source/WebCore/bindings/v8/V8WindowErrorHandler.cpp +++ b/Source/WebCore/bindings/v8/V8WindowErrorHandler.cpp @@ -58,7 +58,7 @@ v8::Local V8WindowErrorHandler::callListenerFunction(ScriptExecutionC v8::Handle parameters[3] = { v8String(errorEvent->message()), v8String(errorEvent->filename()), v8::Integer::New(errorEvent->lineno()) }; v8::TryCatch tryCatch; tryCatch.SetVerbose(true); - returnValue = V8Proxy::instrumentedCallFunction(0 /* page */, callFunction, thisValue, 3, parameters); + returnValue = V8Proxy::instrumentedCallFunction(0 /* frame */, callFunction, thisValue, 3, parameters); } return returnValue; } diff --git a/Source/WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp b/Source/WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp index e3a2a03..da42750 100644 --- a/Source/WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp +++ b/Source/WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp @@ -83,8 +83,8 @@ bool invokeCallback(v8::Persistent callback, int argc, v8::Handle thisObject = v8::Context::GetCurrent()->Global(); - Page* page = scriptExecutionContext && scriptExecutionContext->isDocument() ? static_cast(scriptExecutionContext)->page() : 0; - v8::Handle result = V8Proxy::instrumentedCallFunction(page, callbackFunction, thisObject, argc, argv); + Frame* frame = scriptExecutionContext && scriptExecutionContext->isDocument() ? static_cast(scriptExecutionContext)->frame() : 0; + v8::Handle result = V8Proxy::instrumentedCallFunction(frame, callbackFunction, thisObject, argc, argv); callbackReturnValue = !result.IsEmpty() && result->BooleanValue(); return exceptionCatcher.HasCaught(); diff --git a/Source/WebCore/bindings/v8/custom/V8CustomXPathNSResolver.cpp b/Source/WebCore/bindings/v8/custom/V8CustomXPathNSResolver.cpp index a126dd0..0b5db98 100644 --- a/Source/WebCore/bindings/v8/custom/V8CustomXPathNSResolver.cpp +++ b/Source/WebCore/bindings/v8/custom/V8CustomXPathNSResolver.cpp @@ -79,7 +79,7 @@ String V8CustomXPathNSResolver::lookupNamespaceURI(const String& prefix) v8::Handle argv[argc] = { v8String(prefix) }; v8::Handle function = lookupNamespaceURIFunc.IsEmpty() ? v8::Handle::Cast(m_resolver) : lookupNamespaceURIFunc; - v8::Handle retval = V8Proxy::instrumentedCallFunction(0 /* page */, function, m_resolver, argc, argv); + v8::Handle retval = V8Proxy::instrumentedCallFunction(0 /* frame */, function, m_resolver, argc, argv); // Eat exceptions from namespace resolver and return an empty string. This will most likely cause NAMESPACE_ERR. if (try_catch.HasCaught()) diff --git a/Source/WebCore/dom/WebKitMutationObserver.cpp b/Source/WebCore/dom/WebKitMutationObserver.cpp index a7eb1a1..6015362 100644 --- a/Source/WebCore/dom/WebKitMutationObserver.cpp +++ b/Source/WebCore/dom/WebKitMutationObserver.cpp @@ -41,6 +41,7 @@ #include "MutationRecord.h" #include "Node.h" #include +#include namespace WebCore { @@ -115,6 +116,7 @@ static MutationObserverSet& activeMutationObservers() void WebKitMutationObserver::enqueueMutationRecord(PassRefPtr mutation) { + ASSERT(isMainThread()); m_records.append(mutation); activeMutationObservers().add(this); } @@ -132,6 +134,7 @@ void WebKitMutationObserver::deliver() void WebKitMutationObserver::deliverAllMutations() { + ASSERT(isMainThread()); static bool deliveryInProgress = false; if (deliveryInProgress) return; diff --git a/Source/WebKit/chromium/ChangeLog b/Source/WebKit/chromium/ChangeLog index 401c861..bf64487 100644 --- a/Source/WebKit/chromium/ChangeLog +++ b/Source/WebKit/chromium/ChangeLog @@ -1,3 +1,13 @@ +2012-02-08 Adam Klein + + DOM mutations should not be delivered on worker threads + https://bugs.webkit.org/show_bug.cgi?id=77898 + + Reviewed by Dmitry Titov. + + * src/WebDevToolsFrontendImpl.cpp: + (WebKit::WebDevToolsFrontendImpl::dispatchOnInspectorFrontend): + 2012-02-08 Scott Graham Roll Chromium DEPS diff --git a/Source/WebKit/chromium/src/WebDevToolsFrontendImpl.cpp b/Source/WebKit/chromium/src/WebDevToolsFrontendImpl.cpp index c658ca0..dc126f0f 100644 --- a/Source/WebKit/chromium/src/WebDevToolsFrontendImpl.cpp +++ b/Source/WebKit/chromium/src/WebDevToolsFrontendImpl.cpp @@ -123,7 +123,7 @@ void WebDevToolsFrontendImpl::dispatchOnInspectorFrontend(const WebString& messa args.append(ToV8String(message)); v8::TryCatch tryCatch; tryCatch.SetVerbose(true); - V8Proxy::instrumentedCallFunction(frame->frame()->page(), function, inspectorBackend, args.size(), args.data()); + V8Proxy::instrumentedCallFunction(frame->frame(), function, inspectorBackend, args.size(), args.data()); } } // namespace WebKit -- 2.7.4