Implement Worker-MainThread bridge for FileSystem API in Chrome

content/child/fileapi/webfilesystem_impl.cc

Index: content/child/fileapi/webfilesystem_impl.cc
diff --git a/content/child/fileapi/webfilesystem_impl.cc b/content/child/fileapi/webfilesystem_impl.cc
index 9bb8664f9bcd3debf0d755ec8811c0665079c2df..30f7fd0309104a522c563c97a2d1c8f94ca5f6c8 100644
--- a/content/child/fileapi/webfilesystem_impl.cc
+++ b/content/child/fileapi/webfilesystem_impl.cc
@@ -5,14 +5,20 @@
 #include "content/child/fileapi/webfilesystem_impl.h"
 
 #include "base/bind.h"
+#include "base/logging.h"
+#include "base/message_loop/message_loop_proxy.h"
 #include "content/child/child_thread.h"
 #include "content/child/fileapi/file_system_dispatcher.h"
 #include "content/child/fileapi/webfilesystem_callback_adapters.h"
 #include "content/child/fileapi/webfilewriter_impl.h"
-#include "third_party/WebKit/public/web/WebFileSystemCallbacks.h"
+#include "content/child/thread_safe_sender.h"
+#include "content/common/fileapi/file_system_messages.h"
 #include "third_party/WebKit/public/platform/WebFileInfo.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/platform/WebURL.h"
+#include "third_party/WebKit/public/web/WebFileSystemCallbacks.h"
+#include "url/gurl.h"
+#include "webkit/child/worker_task_runner.h"
 #include "webkit/glue/webkit_glue.h"
 
 using WebKit::WebFileInfo;
@@ -21,6 +27,7 @@ using WebKit::WebFileSystemEntry;
 using WebKit::WebString;
 using WebKit::WebURL;
 using WebKit::WebVector;
+using webkit_glue::WorkerTaskRunner;
 
 namespace content {
 
@@ -39,107 +46,265 @@ void DidReadMetadataForCreateFileWriter(
                                  file_info.size);
 }
 
+int CurrentWorkerId() {
+  return WorkerTaskRunner::Instance()->CurrentWorkerId();
+}
+
+class CallbacksWrapper
+    : public WebFileSystemCallbacks,
+      public WorkerTaskRunner::Observer {
+ public:
+  CallbacksWrapper(WebFileSystemCallbacks* original_callbacks,
+                   ThreadSafeSender* thread_safe_sender)
+      : callbacks_(original_callbacks),
+        thread_safe_sender_(thread_safe_sender),
+        worker_id_(CurrentWorkerId()) {
+    if (worker_id_)
+      WorkerTaskRunner::Instance()->AddStopObserver(this);
+  }
+
+  virtual ~CallbacksWrapper() {
+    if (CurrentWorkerId())
+      WorkerTaskRunner::Instance()->RemoveStopObserver(this);
+  }
+
+  // WorkerTaskRunner::Observer overrides.
+  virtual void OnWorkerRunLoopStopped() OVERRIDE {
+    callbacks_->didFail(WebKit::WebFileErrorAbort);
+    callbacks_ = NULL;
+  }
+
+  // WebFileSystemCallbacks overrides.
+  virtual void didSucceed() OVERRIDE {
+    RunCallback(base::Bind(&WebFileSystemCallbacks::didSucceed,
+                           base::Unretained(callbacks_)));
+  }
+  virtual void didReadMetadata(const WebFileInfo& info) OVERRIDE {
+    RunCallback(base::Bind(&WebFileSystemCallbacks::didReadMetadata,
+                           base::Unretained(callbacks_), info));
+  }
+  virtual void didReadDirectory(const WebVector<WebFileSystemEntry>& entries,
+                                bool hasMore) OVERRIDE {
+    RunCallback(base::Bind(&WebFileSystemCallbacks::didReadDirectory,
+                           base::Unretained(callbacks_), entries, hasMore));
+  }
+  virtual void didOpenFileSystem(const WebString& name,
+                                 const WebURL& rootURL) OVERRIDE {
+    RunCallback(base::Bind(&WebFileSystemCallbacks::didOpenFileSystem,
+                           base::Unretained(callbacks_), name, rootURL));
+  }
+  virtual void didFail(WebKit::WebFileError error) OVERRIDE {
+    RunCallback(base::Bind(&WebFileSystemCallbacks::didFail,
+                           base::Unretained(callbacks_), error));
+  }
+  virtual bool shouldBlockUntilCompletion() const OVERRIDE {
+    return callbacks_->shouldBlockUntilCompletion();
+  }
+
+  void DidCreateSnapshotFile(const base::PlatformFileInfo& file_info,
+                             const base::FilePath& platform_path,
+                             int request_id) {
+    if (worker_id_ != CurrentWorkerId()) {
+      PostTaskToWorker(
+          base::Bind(&CallbacksWrapper::DidCreateSnapshotFile,
+                     base::Unretained(this), file_info, platform_path,
+                     request_id));
+      return;
+    }
+    scoped_ptr<CallbacksWrapper> deleter(this);
+    DCHECK(callbacks_);
+    WebFileInfo web_file_info;
+    webkit_glue::PlatformFileInfoToWebFileInfo(file_info, &web_file_info);
+    web_file_info.platformPath = platform_path.AsUTF16Unsafe();
+    callbacks_->didCreateSnapshotFile(web_file_info);
+
+    thread_safe_sender_->Send(
+        new FileSystemHostMsg_DidReceiveSnapshotFile(request_id));

[michaeln, 2013/07/24 22:21:12]

Drat, I think i gave you a bum steer, very sorry :(

The code as written in this snapshot is what should be good, but since blob msgs
(that happen under the call to didCreateSnapshotFile) still employ main thread
bridging while ThreadSafeSender does not, it's racey.

Your earlier code to have the ACK message go thru the 'main' thread had it right
for now. You can add a TODO(michaeln) on the code routing the msg thru the main
thread to change it after blob msgs are switched over.

[kinuko, 2013/07/25 04:32:41]

Yup... I vaguely remember that's how I decided to use main thread model in the
initial patch, though I had forgotten about that.

Done.

+  }
+
+ private:
+  void RunCallback(const base::Closure& callback) {
+    if (worker_id_ != CurrentWorkerId()) {
+      PostTaskToWorker(base::Bind(&CallbacksWrapper::RunCallback,
+                                  base::Unretained(this), callback));
+      return;
+    }
+    scoped_ptr<CallbacksWrapper> deleter(this);
+    DCHECK(callbacks_);
+    callback.Run();
+  }
+
+  void PostTaskToWorker(const base::Closure& closure) {
+    if (!WorkerTaskRunner::Instance()->PostTask(worker_id_, closure))
+      delete this;

[michaeln, 2013/07/24 22:21:12]

I'm really not sure about 'delete this' correctness in this case? Clearly, if
the worker runloop is well past having stopped, this delete is fine and good.

But I'm wondering about intermediary states in the act of worker shutdown where
the runloop is not yet stopped but it's internal message queue has stopped
accepting new message. I think that is an expected case based on reading thru
some code and we'll see 'false' here.

I don't think a false return from PostTask strictly means the
OnWorkerRunLoopStopped() has been called, but since the object ptr is not
removed as a stop observer in this delete, it will get called. If that's right
it'll be UAF at that time.

This is a bit tricky.

[kinuko, 2013/07/25 04:32:41]

Right. I overlooked PostTask returns false when WorkerRunLoop.postTask
(MessageQueue.append) returns false, which could happen before RunLoopStopped. 
Ok... I could add a locked flag, but I'd like to leak callbacks in the case.
(There're some other places we leak callbacks in the shutdown sequence)

[michaeln, 2013/07/25 21:39:08]

For a process shutdown leaks, who cares, but these are potential
per-worker-thread leaks. I don't think we should leave leaks at this level in
permanently. If we leave the leak in for now, tools will probably yell at us
some, and we should probably have a bug and a TODO referring to the bug.

IDB avoids this sort of problem by having a somewhat different split. In
IDB'land there is a single stateless MessageFilter class (lives on the IPC
thread) and there are per-thread stateful Dispatcher instances (live on context
threads). The Dispatcher instances and all per-call Callback like data
structures are 100% owned/accessed only the single thread, ptrs values to the
thread specific data are not ever seen on other threads. Key to allowing that to
work is that the the target thread_id/worker_id is baked into the IPC msgs and
responses. The Filter knows what to do with the response purely based on data
contained in the msg. If a worker runloops stops the Dispatcher and all pending
response statefulness can be deleted without concern for what the Filter is
doing atm or will do at a later time.

I don't know if you're working towards having the FileAPI function along those
lines or not? But having very strictly apartment threaded stuff seems to help.

[kinuko, 2013/07/26 12:29:55]

My initial implementation was exactlly following IDB's model, made Dispatcher
thread-local instance and had another MessageFilter to relay the response to the
correct caller thread ID. But then it turned out that we still need worker to
main-thread bridging due to Blob and API compatibility issues, so I went back to
intermediary bridge model like the one in Blink, hoping eventually we could
migrate to simpler model.  In the sense answer is yes.

I've revised this once again, added thread-local callbacks map (currently this
cannot be held by WebFileSystemImpl nor FileSystemDispatcher) and made the
bridge code trigger callbacks via this map.  Now we have more indirection, but
thread separation must be clearer. Can you take another look?

+  }
+
+  WebFileSystemCallbacks* callbacks_;
+  scoped_refptr<ThreadSafeSender> thread_safe_sender_;
+  int worker_id_;
+};
+
+template <typename Method, typename Params>
+void CallDispatcherOnMainThread(
+    base::MessageLoopProxy* loop,
+    Method method, const Params& params,
+    WebFileSystemCallbacks* callbacks) {
+  if (!loop->RunsTasksOnCurrentThread()) {
+    loop->PostTask(FROM_HERE,
+                   base::Bind(&CallDispatcherOnMainThread<Method, Params>,
+                              make_scoped_refptr(loop),
+                              method, params, callbacks));
+    return;
+  }
+  if (!ChildThread::current() ||
+      !ChildThread::current()->file_system_dispatcher()) {
+    callbacks->didFail(WebKit::WebFileErrorAbort);
+    return;
+  }
+  DispatchToMethod(ChildThread::current()->file_system_dispatcher(),
+                   method, params);
+}
+
 }  // namespace
 
-WebFileSystemImpl::WebFileSystemImpl() {
+WebFileSystemImpl::WebFileSystemImpl(
+    base::MessageLoopProxy* main_thread_loop,
+    ThreadSafeSender* sender)
+    : main_thread_loop_(main_thread_loop),
+      sender_(sender) {
+  // TODO(kinuko): Support creating and accessing WebFileSystemImpl on
+  // non-main thread. (crbug.com/257349)
+  DCHECK(main_thread_loop_->RunsTasksOnCurrentThread());
 }
 
-void WebFileSystemImpl::move(const WebURL& src_path,
-                             const WebURL& dest_path,
-                             WebFileSystemCallbacks* callbacks) {
-  FileSystemDispatcher* dispatcher =
-      ChildThread::current()->file_system_dispatcher();
-  dispatcher->Move(GURL(src_path),
-                   GURL(dest_path),
-                   base::Bind(&FileStatusCallbackAdapter, callbacks));
+WebFileSystemImpl::~WebFileSystemImpl() {
 }
 
-void WebFileSystemImpl::copy(const WebURL& src_path,
-                             const WebURL& dest_path,
-                             WebFileSystemCallbacks* callbacks) {
-  FileSystemDispatcher* dispatcher =
-      ChildThread::current()->file_system_dispatcher();
-  dispatcher->Copy(GURL(src_path),
-                   GURL(dest_path),
-                   base::Bind(&FileStatusCallbackAdapter, callbacks));
+// WebFileSystem implementation.
+void WebFileSystemImpl::move(
+    const WebKit::WebURL& src_path,
+    const WebKit::WebURL& dest_path,
+    WebKit::WebFileSystemCallbacks* callbacks) {
+  CallbacksWrapper* wrapper = new CallbacksWrapper(callbacks, sender_.get());
+  CallDispatcherOnMainThread(
+      main_thread_loop_.get(),
+      &FileSystemDispatcher::Move,
+      MakeTuple(GURL(src_path), GURL(dest_path),
+                base::Bind(&FileStatusCallbackAdapter, wrapper)),
+      wrapper);
 }
 
-void WebFileSystemImpl::remove(const WebURL& path,
-                               WebFileSystemCallbacks* callbacks) {
-  FileSystemDispatcher* dispatcher =
-      ChildThread::current()->file_system_dispatcher();
-  dispatcher->Remove(
-      GURL(path),
-      false /* recursive */,
-      base::Bind(&FileStatusCallbackAdapter, callbacks));
+void WebFileSystemImpl::copy(
+    const WebKit::WebURL& src_path,
+    const WebKit::WebURL& dest_path,
+    WebKit::WebFileSystemCallbacks* callbacks) {
+  CallbacksWrapper* wrapper = new CallbacksWrapper(callbacks, sender_.get());
+  CallDispatcherOnMainThread(
+      main_thread_loop_.get(),
+      &FileSystemDispatcher::Copy,
+      MakeTuple(GURL(src_path), GURL(dest_path),
+                base::Bind(&FileStatusCallbackAdapter, wrapper)),
+      wrapper);
 }
 
-void WebFileSystemImpl::removeRecursively(const WebURL& path,
-                                          WebFileSystemCallbacks* callbacks) {
-  FileSystemDispatcher* dispatcher =
-      ChildThread::current()->file_system_dispatcher();
-  dispatcher->Remove(
-      GURL(path),
-      true /* recursive */,
-      base::Bind(&FileStatusCallbackAdapter, callbacks));
+void WebFileSystemImpl::remove(
+    const WebKit::WebURL& path,
+    WebKit::WebFileSystemCallbacks* callbacks) {
+  CallbacksWrapper* wrapper = new CallbacksWrapper(callbacks, sender_.get());
+  CallDispatcherOnMainThread(
+      main_thread_loop_.get(),
+      &FileSystemDispatcher::Remove,
+      MakeTuple(GURL(path), false /* recursive */,
+                base::Bind(&FileStatusCallbackAdapter, wrapper)),
+      wrapper);
 }
 
-void WebFileSystemImpl::readMetadata(const WebURL& path,
-                                     WebFileSystemCallbacks* callbacks) {
-  FileSystemDispatcher* dispatcher =
-      ChildThread::current()->file_system_dispatcher();
-  dispatcher->ReadMetadata(
-      GURL(path),
-      base::Bind(&ReadMetadataCallbackAdapter, callbacks),
-      base::Bind(&FileStatusCallbackAdapter, callbacks));
+void WebFileSystemImpl::removeRecursively(
+    const WebKit::WebURL& path,
+    WebKit::WebFileSystemCallbacks* callbacks) {
+  CallbacksWrapper* wrapper = new CallbacksWrapper(callbacks, sender_.get());
+  CallDispatcherOnMainThread(
+      main_thread_loop_.get(),
+      &FileSystemDispatcher::Remove,
+      MakeTuple(GURL(path), true /* recursive */,
+                base::Bind(&FileStatusCallbackAdapter, wrapper)),
+      wrapper);
 }
 
-void WebFileSystemImpl::createFile(const WebURL& path,
-                                   bool exclusive,
-                                   WebFileSystemCallbacks* callbacks) {
-  FileSystemDispatcher* dispatcher =
-      ChildThread::current()->file_system_dispatcher();
-  dispatcher->CreateFile(
-      GURL(path), exclusive,
-      base::Bind(&FileStatusCallbackAdapter, callbacks));
+void WebFileSystemImpl::readMetadata(
+    const WebKit::WebURL& path,
+    WebKit::WebFileSystemCallbacks* callbacks) {
+  CallbacksWrapper* wrapper = new CallbacksWrapper(callbacks, sender_.get());
+  CallDispatcherOnMainThread(
+      main_thread_loop_.get(),
+      &FileSystemDispatcher::ReadMetadata,
+      MakeTuple(GURL(path),
+                base::Bind(&ReadMetadataCallbackAdapter, wrapper),
+                base::Bind(&FileStatusCallbackAdapter, wrapper)),
+      wrapper);
 }
 
-void WebFileSystemImpl::createDirectory(const WebURL& path,
-                                        bool exclusive,
-                                        WebFileSystemCallbacks* callbacks) {
-  FileSystemDispatcher* dispatcher =
-      ChildThread::current()->file_system_dispatcher();
-  dispatcher->CreateDirectory(
-      GURL(path), exclusive, false /* recursive */,
-      base::Bind(&FileStatusCallbackAdapter, callbacks));
+void WebFileSystemImpl::createFile(
+    const WebKit::WebURL& path,
+    bool exclusive,
+    WebKit::WebFileSystemCallbacks* callbacks) {
+  CallbacksWrapper* wrapper = new CallbacksWrapper(callbacks, sender_.get());
+  CallDispatcherOnMainThread(
+      main_thread_loop_.get(),
+      &FileSystemDispatcher::CreateFile,
+      MakeTuple(GURL(path), exclusive,
+                base::Bind(&FileStatusCallbackAdapter, wrapper)),
+      wrapper);
 }
 
-void WebFileSystemImpl::fileExists(const WebURL& path,
-                                   WebFileSystemCallbacks* callbacks) {
-  FileSystemDispatcher* dispatcher =
-      ChildThread::current()->file_system_dispatcher();
-  dispatcher->Exists(
-      GURL(path), false /* directory */,
-      base::Bind(&FileStatusCallbackAdapter, callbacks));
+void WebFileSystemImpl::createDirectory(
+    const WebKit::WebURL& path,
+    bool exclusive,
+    WebKit::WebFileSystemCallbacks* callbacks) {
+  CallbacksWrapper* wrapper = new CallbacksWrapper(callbacks, sender_.get());
+  CallDispatcherOnMainThread(
+      main_thread_loop_.get(),
+      &FileSystemDispatcher::CreateDirectory,
+      MakeTuple(GURL(path), exclusive, false /* recursive */,
+                base::Bind(&FileStatusCallbackAdapter, wrapper)),
+      wrapper);
 }
 
-void WebFileSystemImpl::directoryExists(const WebURL& path,
-                                        WebFileSystemCallbacks* callbacks) {
-  FileSystemDispatcher* dispatcher =
-      ChildThread::current()->file_system_dispatcher();
-  dispatcher->Exists(
-      GURL(path), true /* directory */,
-      base::Bind(&FileStatusCallbackAdapter, callbacks));
+void WebFileSystemImpl::fileExists(
+    const WebKit::WebURL& path,
+    WebKit::WebFileSystemCallbacks* callbacks) {
+  CallbacksWrapper* wrapper = new CallbacksWrapper(callbacks, sender_.get());
+  CallDispatcherOnMainThread(
+      main_thread_loop_.get(),
+      &FileSystemDispatcher::Exists,
+      MakeTuple(GURL(path), false /* directory */,
+                base::Bind(&FileStatusCallbackAdapter, wrapper)),
+      wrapper);
 }
 
-void WebFileSystemImpl::readDirectory(const WebURL& path,
-                                      WebFileSystemCallbacks* callbacks) {
-  FileSystemDispatcher* dispatcher =
-      ChildThread::current()->file_system_dispatcher();
-  dispatcher->ReadDirectory(
-      GURL(path),
-      base::Bind(&ReadDirectoryCallbackAdapater, callbacks),
-      base::Bind(&FileStatusCallbackAdapter, callbacks));
+void WebFileSystemImpl::directoryExists(
+    const WebKit::WebURL& path,
+    WebKit::WebFileSystemCallbacks* callbacks) {
+  CallbacksWrapper* wrapper = new CallbacksWrapper(callbacks, sender_.get());
+  CallDispatcherOnMainThread(
+      main_thread_loop_.get(),
+      &FileSystemDispatcher::Exists,
+      MakeTuple(GURL(path), true /* directory */,
+                base::Bind(&FileStatusCallbackAdapter, wrapper)),
+      wrapper);
+}
+
+void WebFileSystemImpl::readDirectory(
+    const WebKit::WebURL& path,
+    WebKit::WebFileSystemCallbacks* callbacks) {
+  CallbacksWrapper* wrapper = new CallbacksWrapper(callbacks, sender_.get());
+  CallDispatcherOnMainThread(
+      main_thread_loop_.get(),
+      &FileSystemDispatcher::ReadDirectory,
+      MakeTuple(GURL(path),
+                base::Bind(&ReadDirectoryCallbackAdapater, wrapper),
+                base::Bind(&FileStatusCallbackAdapter, wrapper)),
+      wrapper);
 }
 
 WebKit::WebFileWriter* WebFileSystemImpl::createFileWriter(
@@ -151,6 +316,8 @@ void WebFileSystemImpl::createFileWriter(
     const WebURL& path,
     WebKit::WebFileWriterClient* client,
     WebKit::WebFileSystemCallbacks* callbacks) {
+  // TODO(kinuko): Convert this method to use bridge model. (crbug.com/257349)
+  DCHECK(main_thread_loop_->RunsTasksOnCurrentThread());
   FileSystemDispatcher* dispatcher =
       ChildThread::current()->file_system_dispatcher();
   dispatcher->ReadMetadata(
@@ -163,12 +330,15 @@ void WebFileSystemImpl::createFileWriter(
 void WebFileSystemImpl::createSnapshotFileAndReadMetadata(
     const WebKit::WebURL& path,
     WebKit::WebFileSystemCallbacks* callbacks) {
-  FileSystemDispatcher* dispatcher =
-      ChildThread::current()->file_system_dispatcher();
-  dispatcher->CreateSnapshotFile(
-      GURL(path),
-      base::Bind(&CreateSnapshotFileCallbackAdapter, callbacks),
-      base::Bind(&FileStatusCallbackAdapter, callbacks));
+  CallbacksWrapper* wrapper = new CallbacksWrapper(callbacks, sender_.get());
+  CallDispatcherOnMainThread(
+      main_thread_loop_.get(),
+      &FileSystemDispatcher::CreateSnapshotFile,
+      MakeTuple(GURL(path),
+                base::Bind(&CallbacksWrapper::DidCreateSnapshotFile,
+                           base::Unretained(wrapper)),
+                base::Bind(&FileStatusCallbackAdapter, wrapper)),
+      wrapper);
 }
 
 }  // namespace content