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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index af2ff69..3271eb4 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,53 @@
+2012-02-08  Adam Klein  <adamk@chromium.org>
+
+        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  <andersca@apple.com>
 
         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 (file)
--- 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<v8::Value> result = V8Proxy::instrumentedCallFunction(0 /* page */, function, object, m_arguments.size(), args.get());
+    v8::Handle<v8::Value> 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 (file)
--- 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<v8::Handle<v8::Value> > args = adoptArrayPtr(new v8::Handle<v8::Value>[1]);
     args[0] = toV8(node);
 
-    v8::Handle<v8::Value> result = V8Proxy::instrumentedCallFunction(0 /* page */, callback, object, 1, args.get());
+    v8::Handle<v8::Value> 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 (file)
--- a/Source/WebCore/bindings/v8/V8Proxy.cpp
+++ b/Source/WebCore/bindings/v8/V8Proxy.cpp
@@ -378,7 +378,7 @@ v8::Local<v8::Value> V8Proxy::runScript(v8::Handle<v8::Script> script)
     v8::TryCatch tryCatch;
     tryCatch.SetVerbose(true);
     {
-        V8RecursionScope recursionScope;
+        V8RecursionScope recursionScope(frame()->document());
         result = script->Run();
     }
 
@@ -404,10 +404,10 @@ v8::Local<v8::Value> V8Proxy::callFunction(v8::Handle<v8::Function> function, v8
 {
     // Keep Frame (and therefore ScriptController and V8Proxy) alive.
     RefPtr<Frame> protect(frame());
-    return V8Proxy::instrumentedCallFunction(m_frame->page(), function, receiver, argc, args);
+    return V8Proxy::instrumentedCallFunction(frame(), function, receiver, argc, args);
 }
 
-v8::Local<v8::Value> V8Proxy::instrumentedCallFunction(Page* page, v8::Handle<v8::Function> function, v8::Handle<v8::Object> receiver, int argc, v8::Handle<v8::Value> args[])
+v8::Local<v8::Value> V8Proxy::instrumentedCallFunction(Frame* frame, v8::Handle<v8::Function> function, v8::Handle<v8::Object> receiver, int argc, v8::Handle<v8::Value> args[])
 {
     V8GCController::checkMemoryUsage();
 
@@ -415,7 +415,7 @@ v8::Local<v8::Value> V8Proxy::instrumentedCallFunction(Page* page, v8::Handle<v8
         return handleMaxRecursionDepthExceeded();
 
     InspectorInstrumentationCookie cookie;
-    if (InspectorInstrumentation::hasFrontends()) {
+    if (InspectorInstrumentation::hasFrontends() && frame) {
         String resourceName("undefined");
         int lineNumber = 1;
         v8::ScriptOrigin origin = function->GetScriptOrigin();
@@ -423,12 +423,12 @@ v8::Local<v8::Value> V8Proxy::instrumentedCallFunction(Page* page, v8::Handle<v8
             resourceName = toWebCoreString(origin.ResourceName());
             lineNumber = function->GetScriptLineNumber() + 1;
         }
-        cookie = InspectorInstrumentation::willCallFunction(page, resourceName, lineNumber);
+        cookie = InspectorInstrumentation::willCallFunction(frame->page(), resourceName, lineNumber);
     }
 
     v8::Local<v8::Value> 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 (file)
--- 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<v8::Value> callFunction(v8::Handle<v8::Function>, v8::Handle<v8::Object>, int argc, v8::Handle<v8::Value> argv[]);
 
         // call the function with the given receiver and arguments and report times to DevTools.
-        static v8::Local<v8::Value> instrumentedCallFunction(Page*, v8::Handle<v8::Function>, v8::Handle<v8::Object> receiver, int argc, v8::Handle<v8::Value> args[]);
+        static v8::Local<v8::Value> instrumentedCallFunction(Frame*, v8::Handle<v8::Function>, v8::Handle<v8::Object> receiver, int argc, v8::Handle<v8::Value> args[]);
 
         // Call the function as constructor with the given arguments.
         v8::Local<v8::Value> newInstance(v8::Handle<v8::Function>, int argc, v8::Handle<v8::Value> argv[]);

diff --git a/Source/WebCore/bindings/v8/V8RecursionScope.cpp b/Source/WebCore/bindings/v8/V8RecursionScope.cpp
index 3564fcc..cad7498 100644 (file)
--- 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 (file)
--- 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 (file)
--- a/Source/WebCore/bindings/v8/V8WindowErrorHandler.cpp
+++ b/Source/WebCore/bindings/v8/V8WindowErrorHandler.cpp
@@ -58,7 +58,7 @@ v8::Local<v8::Value> V8WindowErrorHandler::callListenerFunction(ScriptExecutionC
         v8::Handle<v8::Value> 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 (file)
--- a/Source/WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp
+++ b/Source/WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp
@@ -83,8 +83,8 @@ bool invokeCallback(v8::Persistent<v8::Object> callback, int argc, v8::Handle<v8
 
     v8::Handle<v8::Object> thisObject = v8::Context::GetCurrent()->Global();
 
-    Page* page = scriptExecutionContext && scriptExecutionContext->isDocument() ? static_cast<Document*>(scriptExecutionContext)->page() : 0;
-    v8::Handle<v8::Value> result = V8Proxy::instrumentedCallFunction(page, callbackFunction, thisObject, argc, argv);
+    Frame* frame = scriptExecutionContext && scriptExecutionContext->isDocument() ? static_cast<Document*>(scriptExecutionContext)->frame() : 0;
+    v8::Handle<v8::Value> 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 (file)
--- 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<v8::Value> argv[argc] = { v8String(prefix) };
     v8::Handle<v8::Function> function = lookupNamespaceURIFunc.IsEmpty() ? v8::Handle<v8::Function>::Cast(m_resolver) : lookupNamespaceURIFunc;
 
-    v8::Handle<v8::Value> retval = V8Proxy::instrumentedCallFunction(0 /* page */, function, m_resolver, argc, argv);
+    v8::Handle<v8::Value> 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 (file)
--- a/Source/WebCore/dom/WebKitMutationObserver.cpp
+++ b/Source/WebCore/dom/WebKitMutationObserver.cpp
@@ -41,6 +41,7 @@
 #include "MutationRecord.h"
 #include "Node.h"
 #include <wtf/ListHashSet.h>
+#include <wtf/MainThread.h>
 
 namespace WebCore {
 
@@ -115,6 +116,7 @@ static MutationObserverSet& activeMutationObservers()
 
 void WebKitMutationObserver::enqueueMutationRecord(PassRefPtr<MutationRecord> 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 (file)
--- a/Source/WebKit/chromium/ChangeLog
+++ b/Source/WebKit/chromium/ChangeLog
@@ -1,3 +1,13 @@
+2012-02-08  Adam Klein  <adamk@chromium.org>
+
+        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  <scottmg@chromium.org>
 
         Roll Chromium DEPS

diff --git a/Source/WebKit/chromium/src/WebDevToolsFrontendImpl.cpp b/Source/WebKit/chromium/src/WebDevToolsFrontendImpl.cpp
index c658ca0..dc126f0 100644 (file)
--- 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