2011-03-30 David Levin <levin@chromium.org>
authorlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 31 Mar 2011 03:24:49 +0000 (03:24 +0000)
committerlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 31 Mar 2011 03:24:49 +0000 (03:24 +0000)
        Reviewed by Dmitry Titov.

        UnlockNonLocked condition reached in WorkerFileSystemsCallbackBridge::mayPostTaskToWorker
        https://bugs.webkit.org/show_bug.cgi?id=57382

        There were two issues to address:
        1. The use of a non-thread safe class (RefPtr) in a ThreadSafeRefCounted class.
           The problem was that this RefPtr could be changed on either thread.
        2. Keeping WorkerFileSystemCallbacksBridge alive for while it was being used
           including while its mutex was in use.

        * src/WorkerFileSystemCallbacksBridge.cpp:
        (WebKit::WorkerFileSystemCallbacksBridge::runTaskOnMainThread):
        Changed to take a PassRefPtr and leak the ref count as opposed to relying on
        dispatchTaskToMainThread to store the pointer in m_selfRef.
        (WebKit::WorkerFileSystemCallbacksBridge::dispatchTaskToMainThread): Remove the
        m_selfRef and hand off a PassRefPtr instead.
        (WebKit::WorkerFileSystemCallbacksBridge::mayPostTaskToWorker): Balance out the leaked ref
        and ensure that WorkerFileSystemCallbacksBridge stays alive while the mutex is held.
        * src/WorkerFileSystemCallbacksBridge.h: Removed m_selfRef and derefIfWorkerIsStopped which
          was simply due to m_selfRef.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@82540 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/chromium/ChangeLog
Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp
Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h

index 8becdba..2b0e13c 100644 (file)
@@ -1,3 +1,27 @@
+2011-03-30  David Levin  <levin@chromium.org>
+
+        Reviewed by Dmitry Titov.
+
+        UnlockNonLocked condition reached in WorkerFileSystemsCallbackBridge::mayPostTaskToWorker
+        https://bugs.webkit.org/show_bug.cgi?id=57382
+
+        There were two issues to address:
+        1. The use of a non-thread safe class (RefPtr) in a ThreadSafeRefCounted class.
+           The problem was that this RefPtr could be changed on either thread.
+        2. Keeping WorkerFileSystemCallbacksBridge alive for while it was being used
+           including while its mutex was in use.
+
+        * src/WorkerFileSystemCallbacksBridge.cpp:
+        (WebKit::WorkerFileSystemCallbacksBridge::runTaskOnMainThread):
+        Changed to take a PassRefPtr and leak the ref count as opposed to relying on
+        dispatchTaskToMainThread to store the pointer in m_selfRef.
+        (WebKit::WorkerFileSystemCallbacksBridge::dispatchTaskToMainThread): Remove the
+        m_selfRef and hand off a PassRefPtr instead.
+        (WebKit::WorkerFileSystemCallbacksBridge::mayPostTaskToWorker): Balance out the leaked ref
+        and ensure that WorkerFileSystemCallbacksBridge stays alive while the mutex is held.
+        * src/WorkerFileSystemCallbacksBridge.h: Removed m_selfRef and derefIfWorkerIsStopped which
+          was simply due to m_selfRef.
+
 2011-03-29  John Abd-El-Malek  <jam@chromium.org>
 
         Reviewed by Tony Chang.
index d05322f..5147e15 100644 (file)
@@ -45,6 +45,7 @@
 #include "WorkerThread.h"
 #include <wtf/MainThread.h>
 #include <wtf/Threading.h>
+#include <wtf/UnusedParam.h>
 
 namespace WebCore {
 
@@ -337,26 +338,14 @@ void WorkerFileSystemCallbacksBridge::didReadDirectoryOnWorkerThread(ScriptExecu
     bridge->m_callbacksOnWorkerThread->didReadDirectory(entries, hasMore);
 }
 
-bool WorkerFileSystemCallbacksBridge::derefIfWorkerIsStopped()
-{
-    WebWorkerBase* worker = 0;
-    {
-        MutexLocker locker(m_mutex);
-        worker = m_worker;
-    }
 
-    if (!worker) {
-        m_selfRef.clear();
-        return true;
-    }
-    return false;
-}
-
-void WorkerFileSystemCallbacksBridge::runTaskOnMainThread(WebCore::ScriptExecutionContext* scriptExecutionContext, WorkerFileSystemCallbacksBridge* bridge, PassOwnPtr<WebCore::ScriptExecutionContext::Task> taskToRun)
+void WorkerFileSystemCallbacksBridge::runTaskOnMainThread(WebCore::ScriptExecutionContext* scriptExecutionContext, PassRefPtr<WorkerFileSystemCallbacksBridge> bridge, PassOwnPtr<WebCore::ScriptExecutionContext::Task> taskToRun)
 {
     ASSERT(isMainThread());
-    if (bridge->derefIfWorkerIsStopped())
-        return;
+
+    // Every task run will result in one call to mayPostTaskToWorker, which is where this ref is released.
+    WorkerFileSystemCallbacksBridge* leaked = bridge.leakRef();
+    UNUSED_PARAM(leaked);
     taskToRun->performTask(scriptExecutionContext);
 }
 
@@ -372,19 +361,22 @@ void WorkerFileSystemCallbacksBridge::runTaskOnWorkerThread(WebCore::ScriptExecu
 
 void WorkerFileSystemCallbacksBridge::dispatchTaskToMainThread(PassOwnPtr<WebCore::ScriptExecutionContext::Task> task)
 {
-    ASSERT(!m_selfRef);
     ASSERT(m_worker);
     ASSERT(m_workerContext->isContextThread());
-    m_selfRef = this;
-    m_worker->dispatchTaskToMainThread(createCallbackTask(&runTaskOnMainThread, this, task));
+    m_worker->dispatchTaskToMainThread(createCallbackTask(&runTaskOnMainThread, RefPtr<WorkerFileSystemCallbacksBridge>(this).release(), task));
 }
 
 void WorkerFileSystemCallbacksBridge::mayPostTaskToWorker(PassOwnPtr<ScriptExecutionContext::Task> task, const String& mode)
 {
     ASSERT(isMainThread());
+
+    // Balancing out the ref() done in runTaskOnMainThread. (Since m_mutex is a member and the deref may result
+    // in the destruction of WorkerFileSystemCallbacksBridge, the ordering of the RefPtr and the MutexLocker
+    // is very important, to ensure that the m_mutex is still valid when it gets unlocked.)
+    RefPtr<WorkerFileSystemCallbacksBridge> bridge = adoptRef(this);
     MutexLocker locker(m_mutex);
     if (m_worker)
-        m_worker->postTaskForModeToWorkerContext(createCallbackTask(&runTaskOnWorkerThread, m_selfRef.release(), task), mode);
+        m_worker->postTaskForModeToWorkerContext(createCallbackTask(&runTaskOnWorkerThread, bridge, task), mode);
 }
 
 } // namespace WebCore
index 9a869dc..5af8529 100644 (file)
@@ -122,25 +122,18 @@ private:
     friend class MainThreadFileSystemCallbacks;
 
     // Methods that dispatch WebFileSystemCallbacks on the worker threads.
-    // They release a selfRef of the WorkerFileSystemCallbacksBridge.
     static void didFailOnWorkerThread(WebCore::ScriptExecutionContext*, WorkerFileSystemCallbacksBridge*, WebFileError);
     static void didOpenFileSystemOnWorkerThread(WebCore::ScriptExecutionContext*, WorkerFileSystemCallbacksBridge*, const String& name, const String& rootPath);
     static void didSucceedOnWorkerThread(WebCore::ScriptExecutionContext*, WorkerFileSystemCallbacksBridge*);
     static void didReadMetadataOnWorkerThread(WebCore::ScriptExecutionContext*, WorkerFileSystemCallbacksBridge*, const WebFileInfo&);
     static void didReadDirectoryOnWorkerThread(WebCore::ScriptExecutionContext*, WorkerFileSystemCallbacksBridge*, const WebVector<WebFileSystemEntry>&, bool hasMore);
 
-    // For early-exist; this deref's selfRef and returns true if the worker is already null.
-    bool derefIfWorkerIsStopped();
-
-    static void runTaskOnMainThread(WebCore::ScriptExecutionContext*, WorkerFileSystemCallbacksBridge*, PassOwnPtr<WebCore::ScriptExecutionContext::Task>);
+    static void runTaskOnMainThread(WebCore::ScriptExecutionContext*, PassRefPtr<WorkerFileSystemCallbacksBridge>, PassOwnPtr<WebCore::ScriptExecutionContext::Task>);
     static void runTaskOnWorkerThread(WebCore::ScriptExecutionContext*, PassRefPtr<WorkerFileSystemCallbacksBridge>, PassOwnPtr<WebCore::ScriptExecutionContext::Task>);
 
     void dispatchTaskToMainThread(PassOwnPtr<WebCore::ScriptExecutionContext::Task>);
     void mayPostTaskToWorker(PassOwnPtr<WebCore::ScriptExecutionContext::Task>, const String& mode);
 
-    // m_selfRef keeps a reference to itself while there's a pending callback on the main thread.
-    RefPtr<WorkerFileSystemCallbacksBridge> m_selfRef;
-
     Mutex m_mutex;
     WebWorkerBase* m_worker;
     WebCore::ScriptExecutionContext* m_workerContext;