Mojo: Fix two bugs in content handling

mojo/shell/dynamic_application_loader.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "mojo/shell/dynamic_application_loader.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "mojo/common/common_type_converters.h"
 #include "mojo/common/data_pipe_utils.h"
 #include "mojo/services/public/interfaces/network/url_loader.mojom.h"
 #include "mojo/shell/context.h"
 #include "mojo/shell/switches.h"
 #include "net/base/filename_util.h"
 
 namespace mojo {
 namespace shell {
 
+// Encapsulates loading and running one individual application.
+//
+// Loaders are owned by DynamicApplicationLoader, so it's always safe for a
+// loader to poke at |delegate_|.
+//
+// Async operations are done with WeakPtr to protect against
+// DynamicApplicationLoader going away (and taking all the Loaders with it)

[tim (not reviewing), 2014/08/27 17:32:19]

I'm not sure I follow why this isn't enough in terms of ownership (sorry, I
think I'm just missing something). The Loader shouldn't be destroyed until the
app is done, so why do we care about something that might still be reading a
response from the URLLoader at that point?

[Aaron Boodman, 2014/08/27 18:33:43]

I was worried about the case where DynamicApplicationLoader itself is destroyed
while a URLLoader or a file read is happening. I figure this can happen during
system shutdown..?

[tim (not reviewing), 2014/08/27 22:54:14]

Yes but I don't see why it matters. In the system shutdown case, the
DynamicApplicationLoader is owned by the ApplicationManager, and the AM is
destroyed after the shell message loop has quit. If the shell message loop has
quit, then any connected applications should have (or will soon) receive a call
(Terminate) that will kill their message loops. I'd expect the ContentHandler to
go away as well?

+// while the async operation is outstanding.
+class Loader {
+ public:
+  Loader(scoped_refptr<ApplicationLoader::LoadCallbacks> callbacks,
+         internal::LoaderDelegate* delegate)

[darin (slow to review), 2014/08/27 06:34:19]

what was the purpose of introducing LoaderDelegate? was it to avoid having to
make Loader and subclasses friends of DynamicApplicationLoader?

[Aaron Boodman, 2014/08/27 18:26:13]

Yes, I considered using friend, but there sure ends up being a lot of
friendliness going on (there are like 5 or 6 things that Loader and subclasses
need to access from DynamicApplicationLoader).

The other option is to pass Loader's dependencies into its constructor. I've
implemented it like this in the latest patch, so you can take a look.

The downside is that there ends up being a lot of dependencies and they all are
weak pointers back to DynamicApplicationLoader... so I think it's maybe slightly
more complex to understand the lifetime issues.

But the upshot is that it gets rid of the public delegate interface...

Let me know what you think.

[Aaron Boodman, 2014/08/27 18:27:37]

Sorry s/weak pointers/raw pointers/.

+      : callbacks_(callbacks),
+        delegate_(delegate),
+        weak_ptr_factory_(this) {
+  }
+
+  virtual ~Loader() {
+  }
+
+ protected:
+  void RunLibrary(const base::FilePath& path, bool path_exists) {
+    ScopedMessagePipeHandle shell_handle = callbacks_->RegisterApplication();
+    if (!shell_handle.is_valid()) {
+      AppCompleted();
+      return;
+    }
+
+    if (!path_exists) {
+      DVLOG(1) << "Library not started because library path '"
+               << path.value() << "' does not exist.";
+      AppCompleted();
+      return;
+    }
+
+    runner_ = delegate_->CreateRunner();
+    runner_->Start(path,
+                   shell_handle.Pass(),
+                   base::Bind(&Loader::AppCompleted,
+                              weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  void AppCompleted() {
+    delegate_->LoaderComplete(this);
+  }
+
+  scoped_refptr<ApplicationLoader::LoadCallbacks> callbacks_;
+  internal::LoaderDelegate* delegate_;
+
+ private:
+  scoped_ptr<DynamicServiceRunner> runner_;
+  base::WeakPtrFactory<Loader> weak_ptr_factory_;
+};
+
+// A loader for local files.
+class LocalLoader : public Loader {
+ public:
+  LocalLoader(const GURL& url,
+              scoped_refptr<ApplicationLoader::LoadCallbacks> callbacks,
+              internal::LoaderDelegate* delegate)
+      : Loader(callbacks, delegate),
+        weak_ptr_factory_(this) {
+    base::FilePath path;
+    net::FileURLToFilePath(url, &path);
+
+    // Async for consistency with network case.
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&LocalLoader::RunLibrary,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   path,
+                   base::PathExists(path)));
+  }
+
+  virtual ~LocalLoader() {
+  }
+
+ private:
+  base::WeakPtrFactory<LocalLoader> weak_ptr_factory_;
+};
+
+// A loader for network files.
+class NetworkLoader : public Loader {
+ public:
+  NetworkLoader(const GURL& url,
+                scoped_refptr<ApplicationLoader::LoadCallbacks> callbacks,
+                internal::LoaderDelegate* delegate)
+      : Loader(callbacks, delegate),
+        weak_ptr_factory_(this) {
+    URLRequestPtr request(URLRequest::New());
+    request->url = String::From(url);
+    request->auto_follow_redirects = true;
+
+    if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kDisableCache)) {
+      request->bypass_cache = true;
+    }
+
+    delegate_->CreateURLLoader(Get(&url_loader_));
+    url_loader_->Start(request.Pass(),
+                       base::Bind(&NetworkLoader::OnLoadComplete,
+                                  weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  virtual ~NetworkLoader() {
+    if (!file_.empty())
+      base::DeleteFile(file_, false);
+  }
+
+ private:
+  void OnLoadComplete(URLResponsePtr response) {
+    if (response->error) {
+      LOG(ERROR) << "Error (" << response->error->code << ": "
+                 << response->error->description << ") while fetching "
+                 << response->url;
+      AppCompleted();
+      return;
+    }
+
+    const GURL& content_handler_url =
+        delegate_->GetContentHandlerURL(response->mime_type);
+    if (!content_handler_url.is_empty()) {
+      callbacks_->LoadWithContentHandler(content_handler_url,
+                                         response.Pass(),
+                                         url_loader_.Pass());
+      AppCompleted();
+      return;
+    }
+
+    base::CreateTemporaryFile(&file_);
+    common::CopyToFile(
+        response->body.Pass(),
+        file_,
+        delegate_->GetBlockingPool(),
+        base::Bind(&NetworkLoader::RunLibrary,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   file_));
+  }
+
+  URLLoaderPtr url_loader_;
+  base::FilePath file_;
+  base::WeakPtrFactory<NetworkLoader> weak_ptr_factory_;
+};
+
 DynamicApplicationLoader::DynamicApplicationLoader(
     Context* context,
     scoped_ptr<DynamicServiceRunnerFactory> runner_factory)
     : context_(context),
-      runner_factory_(runner_factory.Pass()),
-      weak_ptr_factory_(this) {
+      runner_factory_(runner_factory.Pass()) {
 }
 
 DynamicApplicationLoader::~DynamicApplicationLoader() {
 }
 
 void DynamicApplicationLoader::RegisterContentHandler(
     const std::string& mime_type,
     const GURL& content_handler_url) {
   mime_type_to_url_[mime_type] = content_handler_url;
 }
 
 void DynamicApplicationLoader::Load(ApplicationManager* manager,
                                     const GURL& url,
                                     scoped_refptr<LoadCallbacks> callbacks) {
   GURL resolved_url;
   if (url.SchemeIs("mojo")) {
     resolved_url = context_->mojo_url_resolver()->Resolve(url);
   } else {
     resolved_url = url;
   }
 
   if (resolved_url.SchemeIsFile())
-    LoadLocalService(resolved_url, callbacks);
+    loaders_.push_back(new LocalLoader(resolved_url, callbacks, this));
   else
-    LoadNetworkService(resolved_url, callbacks);
-}
-
-void DynamicApplicationLoader::LoadLocalService(
-    const GURL& resolved_url,
-    scoped_refptr<LoadCallbacks> callbacks) {
-  base::FilePath path;
-  net::FileURLToFilePath(resolved_url, &path);
-  const bool kDeleteFileAfter = false;
-
-  // Async for consistency with network case.
-  base::MessageLoop::current()->PostTask(
-      FROM_HERE,
-      base::Bind(&DynamicApplicationLoader::RunLibrary,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 path,
-                 callbacks,
-                 kDeleteFileAfter,
-                 base::PathExists(path)));
-}
-
-void DynamicApplicationLoader::LoadNetworkService(
-    const GURL& resolved_url,
-    scoped_refptr<LoadCallbacks> callbacks) {
-  if (!network_service_) {
-    context_->application_manager()->ConnectToService(
-        GURL("mojo:mojo_network_service"), &network_service_);
-  }
-  if (!url_loader_)
-    network_service_->CreateURLLoader(Get(&url_loader_));
-
-  URLRequestPtr request(URLRequest::New());
-  request->url = String::From(resolved_url);
-  request->auto_follow_redirects = true;
-
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kDisableCache)) {
-    request->bypass_cache = true;
-  }
-
-  url_loader_->Start(
-      request.Pass(),
-      base::Bind(&DynamicApplicationLoader::OnLoadNetworkServiceComplete,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 callbacks));
-}
-
-void DynamicApplicationLoader::OnLoadNetworkServiceComplete(
-    scoped_refptr<LoadCallbacks> callbacks,
-    URLResponsePtr response) {
-  if (response->error) {
-    LOG(ERROR) << "Error (" << response->error->code << ": "
-               << response->error->description << ") while fetching "
-               << response->url;
-  }
-
-  MimeTypeToURLMap::iterator iter = mime_type_to_url_.find(response->mime_type);
-  if (iter != mime_type_to_url_.end()) {
-    callbacks->LoadWithContentHandler(iter->second, response.Pass());
-    return;
-  }
-
-  base::FilePath file;
-  base::CreateTemporaryFile(&file);
-
-  const bool kDeleteFileAfter = true;
-  common::CopyToFile(response->body.Pass(),
-                     file,
-                     context_->task_runners()->blocking_pool(),
-                     base::Bind(&DynamicApplicationLoader::RunLibrary,
-                                weak_ptr_factory_.GetWeakPtr(),
-                                file,
-                                callbacks,
-                                kDeleteFileAfter));
-}
-
-void DynamicApplicationLoader::RunLibrary(
-    const base::FilePath& path,
-    scoped_refptr<LoadCallbacks> callbacks,
-    bool delete_file_after,
-    bool path_exists) {
-
-  ScopedMessagePipeHandle shell_handle = callbacks->RegisterApplication();
-  if (!shell_handle.is_valid())
-    return;
-
-  if (!path_exists) {
-    DVLOG(1) << "Library not started because library path '"
-             << path.value() << "' does not exist.";
-    return;
-  }
-
-  scoped_ptr<DynamicServiceRunner> runner =
-      runner_factory_->Create(context_).Pass();
-  runner->Start(path,
-                shell_handle.Pass(),
-                base::Bind(&DynamicApplicationLoader::OnRunLibraryComplete,
-                           weak_ptr_factory_.GetWeakPtr(),
-                           base::Unretained(runner.get()),
-                           delete_file_after ? path : base::FilePath()));
-  runners_.push_back(runner.release());
-}
-
-void DynamicApplicationLoader::OnRunLibraryComplete(
-    DynamicServiceRunner* runner,
-    const base::FilePath& temp_file) {
-  for (ScopedVector<DynamicServiceRunner>::iterator it = runners_.begin();
-       it != runners_.end(); ++it) {
-    if (*it == runner) {
-      runners_.erase(it);
-      if (!temp_file.empty())
-        base::DeleteFile(temp_file, false);
-      return;
-    }
-  }
+    loaders_.push_back(new NetworkLoader(resolved_url, callbacks, this));
 }
 
 void DynamicApplicationLoader::OnApplicationError(ApplicationManager* manager,
                                                   const GURL& url) {
   // TODO(darin): What should we do about service errors? This implies that
   // the app closed its handle to the service manager. Maybe we don't care?
 }
 
+void DynamicApplicationLoader::LoaderComplete(Loader* loader) {
+  loaders_.erase(std::find(loaders_.begin(), loaders_.end(), loader));
+}
+
+scoped_ptr<DynamicServiceRunner> DynamicApplicationLoader::CreateRunner() {
+  return runner_factory_->Create(context_);
+}
+
+void DynamicApplicationLoader::CreateURLLoader(
+    InterfaceRequest<URLLoader> loader_request) {
+  if (!network_service_) {
+    context_->application_manager()->ConnectToService(
+        GURL("mojo:mojo_network_service"), &network_service_);
+  }
+  network_service_->CreateURLLoader(loader_request.Pass());
+}
+
+const GURL& DynamicApplicationLoader::GetContentHandlerURL(
+    const std::string& mime_type) {
+  MimeTypeToURLMap::iterator iter = mime_type_to_url_.find(mime_type);
+  if (iter != mime_type_to_url_.end()) {
+    return iter->second;
+  } else {

[darin (slow to review), 2014/08/27 06:34:19]

nit: no need for "else" after "return"

+    return GURL::EmptyGURL();
+  }
+}
+
+base::SequencedWorkerPool* DynamicApplicationLoader::GetBlockingPool() {
+  return context_->task_runners()->blocking_pool();
+}
+
 }  // namespace shell
 }  // namespace mojo