- Revision
- 104111
- Author
- [email protected]
- Date
- 2012-01-04 19:08:14 -0800 (Wed, 04 Jan 2012)
Log Message
[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.
Modified Paths
Diff
Modified: trunk/Source/WebKit/chromium/ChangeLog (104110 => 104111)
--- trunk/Source/WebKit/chromium/ChangeLog 2012-01-05 01:50:41 UTC (rev 104110)
+++ trunk/Source/WebKit/chromium/ChangeLog 2012-01-05 03:08:14 UTC (rev 104111)
@@ -1,3 +1,27 @@
+2012-01-04 David Levin <[email protected]>
+
+ [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 <[email protected]>
[chromium] Remove chromium compositor support for unused zoomAnimatorTransform
Modified: trunk/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp (104110 => 104111)
--- trunk/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp 2012-01-05 01:50:41 UTC (rev 104110)
+++ trunk/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp 2012-01-05 03:08:14 UTC (rev 104111)
@@ -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 @@
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 @@
}
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 @@
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 @@
// 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
Modified: trunk/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h (104110 => 104111)
--- trunk/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h 2012-01-05 01:50:41 UTC (rev 104110)
+++ trunk/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h 2012-01-05 03:08:14 UTC (rev 104111)
@@ -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 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 @@
// --> 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 @@
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 @@
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;
};
