Mojo: Fix two bugs in content handling

mojo/shell/dynamic_application_loader.cc

Index: mojo/shell/dynamic_application_loader.cc
diff --git a/mojo/shell/dynamic_application_loader.cc b/mojo/shell/dynamic_application_loader.cc
index 718ece4263714b9f81fd388f3b683eb554efaf84..d03ec3b85dca27e0be61f25a8b602a0c49d6c0b6 100644
--- a/mojo/shell/dynamic_application_loader.cc
+++ b/mojo/shell/dynamic_application_loader.cc
@@ -9,6 +9,7 @@
 #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"
@@ -20,12 +21,159 @@
 namespace mojo {
 namespace shell {
 
+namespace {
+
+// Encapsulates loading and running one individual application.
+//
+// Loaders are owned by DynamicApplicationLoader, so they can always poke into
+// DynamicApplicationLoader (via LoaderDelegate).
+//
+// Async operations are done with WeakPtr to protect against
+// DynamicApplicationLoader going away (and taking all the Loaders with it)
+// while the async operation is oustanding.

[darin (slow to review), 2014/08/27 03:22:04]

nit: oustanding -> outstanding

[Aaron Boodman, 2014/08/27 05:43:51]

Done.

+class Loader {
+ public:
+  Loader(scoped_refptr<ApplicationLoader::LoadCallbacks> callbacks,
+         LoaderDelegate* delegate)
+      : 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_;
+  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,
+              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,
+                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_;
+};
+
+}  // namespace
+
 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() {
@@ -48,129 +196,46 @@ void DynamicApplicationLoader::Load(ApplicationManager* manager,
   }
 
   if (resolved_url.SchemeIsFile())
-    LoadLocalService(resolved_url, callbacks);
+    loaders_.push_back(new LocalLoader(resolved_url, callbacks, this));
   else
-    LoadNetworkService(resolved_url, callbacks);
+    loaders_.push_back(new NetworkLoader(resolved_url, callbacks, this));
 }
 
-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::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::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::LoaderComplete(Loader* loader) {
+  loaders_.erase(std::find(loaders_.begin(), loaders_.end(), loader));
 }
 
-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));
+scoped_ptr<DynamicServiceRunner> DynamicApplicationLoader::CreateRunner() {
+  return runner_factory_->Create(context_);
 }
 
-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;
+void DynamicApplicationLoader::CreateURLLoader(
+    InterfaceRequest<URLLoader> loader_request) {
+  if (!network_service_) {
+    context_->application_manager()->ConnectToService(
+        GURL("mojo:mojo_network_service"), &network_service_);
   }
-
-  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());
+  network_service_->CreateURLLoader(loader_request.Pass());
 }
 
-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;
-    }
+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 {
+    return GURL::EmptyGURL();
   }
 }
 
-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?
+base::SequencedWorkerPool* DynamicApplicationLoader::GetBlockingPool() {
+  return context_->task_runners()->blocking_pool();
 }
 
 }  // namespace shell