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..013715d72f31503b2d1bfdbf8f589536279b47e3 100644
--- a/content/child/fileapi/webfilesystem_impl.cc
+++ b/content/child/fileapi/webfilesystem_impl.cc
@@ -5,6 +5,8 @@
 #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"
@@ -13,6 +15,8 @@
 #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 "url/gurl.h"
+#include "webkit/child/worker_task_runner.h"
 #include "webkit/glue/webkit_glue.h"
 
 using WebKit::WebFileInfo;
@@ -21,6 +25,7 @@ using WebKit::WebFileSystemEntry;
 using WebKit::WebString;
 using WebKit::WebURL;
 using WebKit::WebVector;
+using webkit_glue::WorkerTaskRunner;
 
 namespace content {
 
@@ -39,107 +44,254 @@ void DidReadMetadataForCreateFileWriter(
                                  file_info.size);
 }
 
+int CurrentWorkerId() {
+  return WorkerTaskRunner::Instance()->CurrentWorkerId();
+}
+
+class CallbacksWrapper
+    : public WebFileSystemCallbacks,
+      public WorkerTaskRunner::Observer {
+ public:
+  explicit CallbacksWrapper(WebFileSystemCallbacks* original_callbacks)
+      : callbacks_(original_callbacks),
+        thread_id_(CurrentWorkerId()) {
+    if (thread_id_)
+      WorkerTaskRunner::Instance()->AddStopObserver(this);
+  }
+
+  virtual ~CallbacksWrapper() {
+    if (CurrentWorkerId())
+      WorkerTaskRunner::Instance()->RemoveStopObserver(this);
+  }
+
+  // WorkerTaskRunner::Observer overrides.
+  virtual void OnWorkerRunLoopStopped() OVERRIDE {

[michaeln, 2013/07/24 00:54:08]

At this point the worker run loop is wound down and the script execution
context is all but gone. Is there any reason to complete/finish the call in this
case, or would it work to just stop processing here with an early return?

If the call to didFail() results in webcore logic trying to create new v8
objects (exceptions) or invoke methods of v8 objects, i think that's an
opportunity for odd/unexpected things happening.

[michaeln, 2013/07/24 01:38:58]

Or i guess what i meant to say was, maybe just {
  delete this;
}

[kinuko, 2013/07/24 09:29:09]

Current API is designed as 1. the callback is self-destruct and 2. one of the
methods must be called by the caller.
Deleting callback_ is API violation, but leaking callback_ could leak more
objects in the blink side. In the end the callback does nothing after the script
context's shutdown, so calling this should be safe. (Or at least that's what
we've been doing in Blink)

I'm wondering if I should change this API contract (to let the caller explicitly
delete the callback), but let me keep this code as is for now.

[kinuko, 2013/07/24 11:03:29]

Well... on the second look the current API seems to expect callbacks to be
called or deleted (though it's not documented well).  Ok, let's just delete the
callbacks here.

[kinuko, 2013/07/24 11:15:41]

Oops, callbacks can't be deleted from outside since dtor is protected. Well...
let me keep these didFail(ABORT) calls as is for now in this CL. I'll revisit
this issue later

+    callbacks_->didFail(WebKit::WebFileErrorAbort);
+    delete this;

[michaeln, 2013/07/24 06:19:13]

Oh, I have another question about this method?

Pointers to CallbackWrapper instances are given to the main
FileSystemDispatcher. When that main dispatcher receives a response from the
browser process, it invokes the CallbackWrapper's appropriate
WebFileSystemCallbacks override. But if OnWorkerRunLoopStopped happens prior to
having received that response (and delete this has happened), i think we'll hit
a UAF bug when the FileSystemDispatcher tries to use it.

[kinuko, 2013/07/24 09:29:09]

Ah, in the main thread side-- yes, good catch.  Fixed (I think).

+  }
+
+  // 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,
+                             const base::Closure& did_receive_snapshot) {
+    if (thread_id_ != CurrentWorkerId()) {
+      WorkerTaskRunner::Instance()->PostTask(
+          thread_id_,
+          base::Bind(&CallbacksWrapper::DidCreateSnapshotFile,
+                     base::Unretained(this), file_info, platform_path,
+                     did_receive_snapshot));
+      return;
+    }
+    scoped_ptr<CallbacksWrapper> deleter(this);
+    WebFileInfo web_file_info;
+    webkit_glue::PlatformFileInfoToWebFileInfo(file_info, &web_file_info);
+    web_file_info.platformPath = platform_path.AsUTF16Unsafe();
+    callbacks_->didCreateSnapshotFile(web_file_info);
+    did_receive_snapshot.Run();
+  }
+
+ private:
+  void RunCallback(const base::Closure& callback) {
+    if (thread_id_ != CurrentWorkerId()) {
+      WorkerTaskRunner::Instance()->PostTask(
+          thread_id_,
+          base::Bind(&CallbacksWrapper::RunCallback,
+                    base::Unretained(this), callback));
+      return;
+    }
+    scoped_ptr<CallbacksWrapper> deleter(this);
+    callback.Run();
+  }
+
+  WebFileSystemCallbacks* callbacks_;
+  int thread_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);

[michaeln, 2013/07/24 00:54:08]

It looks like didFail() can be called on the main thread even if the callbacks
have worker thread affinity.

I think in cases where there is no ChildThread or fs_dispatcher instance, that's
indicative of being in the midst of shut down. Is that right? Would it work to
just early return here and stop executing more code in this case.

[kinuko, 2013/07/24 09:29:09]

Done.

+    return;
+  }
+  DispatchToMethod(ChildThread::current()->file_system_dispatcher(),
+                   method, params);
+}
+
 }  // namespace
 
-WebFileSystemImpl::WebFileSystemImpl() {
+WebFileSystemImpl::WebFileSystemImpl(
+    base::MessageLoopProxy* main_thread_loop)
+    : main_thread_loop_(main_thread_loop) {
+  // 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);
+  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);
+  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);
+  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);
+  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);
+  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);
+  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);
+  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);
+  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);
+  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);
+  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 +303,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 +317,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);
+  CallDispatcherOnMainThread(
+      main_thread_loop_.get(),
+      &FileSystemDispatcher::CreateSnapshotFile,
+      MakeTuple(GURL(path),
+                base::Bind(&CallbacksWrapper::DidCreateSnapshotFile,
+                           base::Unretained(wrapper)),
+                base::Bind(&FileStatusCallbackAdapter, wrapper)),
+      wrapper);
 }
 
 }  // namespace content