[chromium] Make it safe to delete WorkerFileSystemContextObserver on any thread.
authorlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Jan 2012 03:08:14 +0000 (03:08 +0000)
committerlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Jan 2012 03:08:14 +0000 (03:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=75573

Reviewed by Dmitry Titov.

* src/WorkerFileSystemCallbacksBridge.cpp:
(WebKit::WorkerFileSystemContextObserver): Move the WorkerContextObserver
out of the WorkerFileSystemCallbacksBridge since an observer should be
destroyed on the WorkerContext thread. (Actually, it could be destroyed on either
thread if you are careful to make a certain method call on it while on the WorkerContext
thread but trying that is a more fragile pattern.)
(WebKit::WorkerFileSystemCallbacksBridge::WorkerFileSystemCallbacksBridge):
(WebKit::WorkerFileSystemCallbacksBridge::stop): Factor out the clean up and make it
clear what the mutex is guarding.
(WebKit::WorkerFileSystemCallbacksBridge::cleanUpAfterCallback): Delete
the observer. Due to where this is called from, it is always called on the WorkerContext thread.
(WebKit::WorkerFileSystemCallbacksBridge::runTaskOnWorkerThread): Replace some code with
the cleanUpAfterCallback call.
(WebKit::WorkerFileSystemCallbacksBridge::mayPostTaskToWorker):
* src/WorkerFileSystemCallbacksBridge.h: In addition to some comment clean ups and code factoring,
I made the desctructor private since no one should call it directly.

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

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

index 168357d..3327d63 100644 (file)
@@ -1,3 +1,27 @@
+2012-01-04  David Levin  <levin@chromium.org>
+
+        [chromium] Make it safe to delete WorkerFileSystemContextObserver on any thread.
+        https://bugs.webkit.org/show_bug.cgi?id=75573
+
+        Reviewed by Dmitry Titov.
+
+        * src/WorkerFileSystemCallbacksBridge.cpp:
+        (WebKit::WorkerFileSystemContextObserver): Move the WorkerContextObserver
+        out of the WorkerFileSystemCallbacksBridge since an observer should be
+        destroyed on the WorkerContext thread. (Actually, it could be destroyed on either
+        thread if you are careful to make a certain method call on it while on the WorkerContext
+        thread but trying that is a more fragile pattern.)
+        (WebKit::WorkerFileSystemCallbacksBridge::WorkerFileSystemCallbacksBridge):
+        (WebKit::WorkerFileSystemCallbacksBridge::stop): Factor out the clean up and make it
+        clear what the mutex is guarding.
+        (WebKit::WorkerFileSystemCallbacksBridge::cleanUpAfterCallback): Delete
+        the observer. Due to where this is called from, it is always called on the WorkerContext thread.
+        (WebKit::WorkerFileSystemCallbacksBridge::runTaskOnWorkerThread): Replace some code with
+        the cleanUpAfterCallback call.
+        (WebKit::WorkerFileSystemCallbacksBridge::mayPostTaskToWorker):
+        * src/WorkerFileSystemCallbacksBridge.h: In addition to some comment clean ups and code factoring,
+        I made the desctructor private since no one should call it directly.
+
 2012-01-04  James Robinson  <jamesr@chromium.org>
 
         [chromium] Remove chromium compositor support for unused zoomAnimatorTransform
index 8ce5e69..17350d1 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2010 Google Inc. All rights reserved.
+ * Copyright (C) 2010, 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -144,15 +144,55 @@ private:
     const String m_mode;
 };
 
+// Observes the worker context. By keeping this separate, it is easier to verify
+// that it only gets deleted on the worker context thread which is verified by ~Observer.
+class WorkerFileSystemContextObserver : public WebCore::WorkerContext::Observer {
+public:
+    static PassOwnPtr<WorkerFileSystemContextObserver> create(WorkerContext* context, WorkerFileSystemCallbacksBridge* bridge)
+    {
+        return adoptPtr(new WorkerFileSystemContextObserver(context, bridge));
+    }
+
+    // WorkerContext::Observer method.
+    virtual void notifyStop()
+    {
+        m_bridge->stop();
+    }
+
+private:
+    WorkerFileSystemContextObserver(WorkerContext* context, WorkerFileSystemCallbacksBridge* bridge)
+        : WebCore::WorkerContext::Observer(context)
+        , m_bridge(bridge)
+    {
+    }
+
+    // Since WorkerFileSystemCallbacksBridge manages the lifetime of this class,
+    // m_bridge will be valid throughout its lifetime.
+    WorkerFileSystemCallbacksBridge* m_bridge;
+};
+
 void WorkerFileSystemCallbacksBridge::stop()
 {
     ASSERT(m_workerContext->isContextThread());
-    MutexLocker locker(m_mutex);
-    m_workerLoaderProxy = 0;
+    {
+        MutexLocker locker(m_loaderProxyMutex);
+        m_workerLoaderProxy = 0;
+    }
 
-    if (m_callbacksOnWorkerThread) {
+    if (m_callbacksOnWorkerThread)
         m_callbacksOnWorkerThread->didFail(WebFileErrorAbort);
-        m_callbacksOnWorkerThread = 0;
+
+    cleanUpAfterCallback();
+}
+
+void WorkerFileSystemCallbacksBridge::cleanUpAfterCallback()
+{
+    ASSERT(m_workerContext->isContextThread());
+
+    m_callbacksOnWorkerThread = 0;
+    if (m_workerContextObserver) {
+        delete m_workerContextObserver;
+        m_workerContextObserver = 0;
     }
 }
 
@@ -339,9 +379,9 @@ void WorkerFileSystemCallbacksBridge::didReadDirectoryOnMainThread(const WebVect
 }
 
 WorkerFileSystemCallbacksBridge::WorkerFileSystemCallbacksBridge(WebCore::WorkerLoaderProxy* workerLoaderProxy, ScriptExecutionContext* scriptExecutionContext, WebFileSystemCallbacks* callbacks)
-    : WorkerContext::Observer(static_cast<WorkerContext*>(scriptExecutionContext))
-    , m_workerLoaderProxy(workerLoaderProxy)
+    : m_workerLoaderProxy(workerLoaderProxy)
     , m_workerContext(scriptExecutionContext)
+    , m_workerContextObserver(WorkerFileSystemContextObserver::create(static_cast<WorkerContext*>(m_workerContext), this).leakPtr())
     , m_callbacksOnWorkerThread(callbacks)
 {
     ASSERT(m_workerContext->isContextThread());
@@ -394,8 +434,11 @@ void WorkerFileSystemCallbacksBridge::runTaskOnWorkerThread(WebCore::ScriptExecu
         return;
     ASSERT(bridge->m_workerContext->isContextThread());
     taskToRun->performTask(scriptExecutionContext);
-    bridge->m_callbacksOnWorkerThread = 0;
-    bridge->stopObserving();
+
+    // taskToRun does the callback.
+    bridge->cleanUpAfterCallback();
+
+    // WorkerFileSystemCallbacksBridge may be deleted here when bridge goes out of scope.
 }
 
 void WorkerFileSystemCallbacksBridge::dispatchTaskToMainThread(PassOwnPtr<WebCore::ScriptExecutionContext::Task> task)
@@ -413,9 +456,11 @@ void WorkerFileSystemCallbacksBridge::mayPostTaskToWorker(PassOwnPtr<ScriptExecu
     // 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);
+    MutexLocker locker(m_loaderProxyMutex);
     if (m_workerLoaderProxy)
         m_workerLoaderProxy->postTaskForModeToWorkerContext(createCallbackTask(&runTaskOnWorkerThread, bridge, task), mode);
+
+    // WorkerFileSystemCallbacksBridge may be deleted here when bridge goes out of scope.
 }
 
 } // namespace WebCore
index 9c44cb1..a1725d0 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2010 Google Inc. All rights reserved.
+ * Copyright (C) 2010, 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -38,7 +38,6 @@
 #include "WebFileError.h"
 #include "platform/WebFileSystem.h"
 #include "platform/WebVector.h"
-#include "WorkerContext.h"
 #include <wtf/PassOwnPtr.h>
 #include <wtf/PassRefPtr.h>
 #include <wtf/Threading.h>
@@ -54,10 +53,11 @@ class MainThreadFileSystemCallbacks;
 class WebCommonWorkerClient;
 class ThreadableCallbacksBridgeWrapper;
 class WebFileSystemCallbacks;
+class WorkerFileSystemContextObserver;
 struct WebFileInfo;
 struct WebFileSystemEntry;
 
-// This class is used to post a openFileSystem request to the main thread and get called back for the request. This must be destructed on the worker thread.
+// Used to post a openFileSystem request to the main thread and get called back for the request.
 //
 // A typical flow for openFileSystem would look like this:
 // Bridge::postOpenFileSystemToMainThread() on WorkerThread
@@ -69,16 +69,8 @@ struct WebFileSystemEntry;
 //  --> Bridge::didXxxOnWorkerThread is called on WorkerThread
 //      This calls the original callbacks (m_callbacksOnWorkerThread) and
 //      releases a self-reference to the bridge.
-class WorkerFileSystemCallbacksBridge : public ThreadSafeRefCounted<WorkerFileSystemCallbacksBridge>, public WebCore::WorkerContext::Observer {
+class WorkerFileSystemCallbacksBridge : public ThreadSafeRefCounted<WorkerFileSystemCallbacksBridge> {
 public:
-    ~WorkerFileSystemCallbacksBridge();
-
-    // WorkerContext::Observer method.
-    virtual void notifyStop()
-    {
-        stop();
-    }
-
     void stop();
 
     static PassRefPtr<WorkerFileSystemCallbacksBridge> create(WebCore::WorkerLoaderProxy* workerLoaderProxy, WebCore::ScriptExecutionContext* workerContext, WebFileSystemCallbacks* callbacks)
@@ -107,7 +99,10 @@ public:
     void didReadDirectoryOnMainThread(const WebVector<WebFileSystemEntry>&, bool hasMore, const String& mode);
 
 private:
+    friend class ThreadSafeRefCounted<WorkerFileSystemCallbacksBridge>;
+
     WorkerFileSystemCallbacksBridge(WebCore::WorkerLoaderProxy*, WebCore::ScriptExecutionContext*, WebFileSystemCallbacks*);
+    ~WorkerFileSystemCallbacksBridge();
 
     // Methods that are to be called on the main thread.
     static void openFileSystemOnMainThread(WebCore::ScriptExecutionContext*, WebCommonWorkerClient*, WebFileSystem::Type, long long size, bool create, WorkerFileSystemCallbacksBridge*, const String& mode);
@@ -137,10 +132,16 @@ private:
     void dispatchTaskToMainThread(PassOwnPtr<WebCore::ScriptExecutionContext::Task>);
     void mayPostTaskToWorker(PassOwnPtr<WebCore::ScriptExecutionContext::Task>, const String& mode);
 
-    Mutex m_mutex;
+    void cleanUpAfterCallback();
+
+    Mutex m_loaderProxyMutex;
     WebCore::WorkerLoaderProxy* m_workerLoaderProxy;
+
     WebCore::ScriptExecutionContext* m_workerContext;
 
+    // Must be deleted on the WorkerContext thread.
+    WorkerFileSystemContextObserver* m_workerContextObserver;
+
     // This is self-destructed and must be fired on the worker thread.
     WebFileSystemCallbacks* m_callbacksOnWorkerThread;
 };