Attachment 121156 Details for Bug 75573 – Patch

[patch] Patch

bug-75573-20120104142825.patch (text/plain), 9.70 KB, created by David Levin on 2012-01-04 14:28:27 PST

(hide)

Description:

Filename:

MIME Type:

Creator: David Levin

Created: 2012-01-04 14:28:27 PST

Size: 9.70 KB

patch

obsolete

>Subversion Revision: 104060
>diff --git a/Source/WebKit/chromium/ChangeLog b/Source/WebKit/chromium/ChangeLog
>index 09dd2e66356eb4702e530e6e5be3129e277ad440..9cddfacc9ee9671ee294fd673c052aaf922413f7 100644
>--- a/Source/WebKit/chromium/ChangeLog
>+++ b/Source/WebKit/chromium/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        * 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  Adrienne Walker  <enne@google.com>
> 
>         [chromium] Create unit tests for CCTiledLayerImpl
>diff --git a/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp b/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp
>index 8ce5e6976bb09193d3a7409a5a1cf78fc4c05571..305eedbd9518593a8ffbc6198df990c29dcd14c8 100644
>--- a/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp
>+++ b/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp
>@@ -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)
>+    {
>+    }
>+
>+    // Although WorkerFileSystemCallbacksBridge manages the lifetime of this class,
>+    // so WorkerFileSystemCallbacksBridge will be valid while this class is around.
>+    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
>diff --git a/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h b/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h
>index 9c44cb1a976e8d652e0604fc61e70a92e3a2ede9..a1725d07bf54847ad5d32b6c6e66bd4b706ac3e0 100644
>--- a/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h
>+++ b/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h
>@@ -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;
> };

Flags:

dimich: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 75573: 121154 | 121155 | 121156