Mojo: Fix two bugs in content handling

1. URLLoaders can't be reused, so we need to construct one
   per-load. This became too gnarly to handle with callbacks
   so I put it back to having a loader object that owns the
   URLLoader.

2. The relevant URLLoader needs to be passed to
   ContentHandler::OnConnect() so that it stays alive long
   enough to read the entire response.

BUG=

Review URL: https://codereview.chromium.org/513573002

Cr-Commit-Position: refs/heads/master@{#292460}

mojo/application_manager/application_loader.cc

@@ -23,7 +23,7 @@ ApplicationLoader::SimpleLoadCallbacks::RegisterApplication() {
 
 void ApplicationLoader::SimpleLoadCallbacks::LoadWithContentHandler(
     const GURL& content_handle_url,
-    URLResponsePtr content) {
+    URLResponsePtr url_response) {
   NOTREACHED();
 }
 

mojo/application_manager/application_loader.h

@@ -34,7 +34,7 @@ class MOJO_APPLICATION_MANAGER_EXPORT ApplicationLoader {
 
     // Load the requested application with a content handler.
     virtual void LoadWithContentHandler(const GURL& content_handler_url,
-                                        URLResponsePtr response) = 0;
+                                        URLResponsePtr url_response) = 0;
 
    protected:
     friend base::RefCounted<LoadCallbacks>;

mojo/application_manager/application_manager.cc

@@ -66,12 +66,12 @@ class ApplicationManager::LoadCallbacksImpl
   }
 
   virtual void LoadWithContentHandler(const GURL& content_handler_url,
-                                      URLResponsePtr content) OVERRIDE {
+                                      URLResponsePtr url_response) OVERRIDE {
     if (manager_) {
       manager_->LoadWithContentHandler(requested_url_,
                                        requestor_url_,
                                        content_handler_url,
-                                       content.Pass(),
+                                       url_response.Pass(),
                                        service_provider_.Pass());
     }
   }
@@ -232,7 +232,7 @@ void ApplicationManager::LoadWithContentHandler(
     const GURL& content_url,
     const GURL& requestor_url,
     const GURL& content_handler_url,
-    URLResponsePtr content,
+    URLResponsePtr url_response,
     ServiceProviderPtr service_provider) {
   ContentHandlerConnection* connection = NULL;
   URLToContentHandlerMap::iterator iter =
@@ -243,8 +243,11 @@ void ApplicationManager::LoadWithContentHandler(
     connection = new ContentHandlerConnection(this, content_handler_url);
     url_to_content_handler_[content_handler_url] = connection;
   }
+
+  InterfaceRequest<ServiceProvider> spir;
+  spir.Bind(service_provider.PassMessagePipe());
   connection->content_handler->OnConnect(
-      content_url.spec(), content.Pass(), service_provider.Pass());
+      content_url.spec(), url_response.Pass(), spir.Pass());
 }
 
 void ApplicationManager::SetLoaderForURL(scoped_ptr<ApplicationLoader> loader,

mojo/application_manager/application_manager.h

@@ -123,7 +123,7 @@ class MOJO_APPLICATION_MANAGER_EXPORT ApplicationManager {
   void LoadWithContentHandler(const GURL& content_url,
                               const GURL& requestor_url,
                               const GURL& content_handler_url,
-                              URLResponsePtr content,
+                              URLResponsePtr url_response,
                               ServiceProviderPtr service_provider);
 
   // Returns the Loader to use for a url (using default if not overridden.)

mojo/examples/content_handler_demo/content_handler_demo.cc

@@ -23,8 +23,9 @@ class ContentHandlerImpl : public InterfaceImpl<ContentHandler> {
 
  private:
   virtual void OnConnect(const mojo::String& url,
-                         URLResponsePtr content,
-                         ServiceProviderPtr service_provider) MOJO_OVERRIDE;
+                         URLResponsePtr response,
+                         InterfaceRequest<ServiceProvider> service_provider)
+      MOJO_OVERRIDE;
 
   ContentHandlerApp* content_handler_app_;
 };
@@ -71,12 +72,13 @@ class ContentHandlerApp : public ApplicationDelegate {
                                   ContentHandlerApp> content_handler_factory_;
 };
 
-void ContentHandlerImpl::OnConnect(const mojo::String& url,
-                                   URLResponsePtr content,
-                                   ServiceProviderPtr service_provider) {
+void ContentHandlerImpl::OnConnect(
+    const mojo::String& url,
+    URLResponsePtr response,
+    InterfaceRequest<ServiceProvider> service_provider) {
   printf("ContentHandler::OnConnect - url:%s - body follows\n\n",
          url.To<std::string>().c_str());
-  content_handler_app_->PrintResponse(content->body.Pass());
+  content_handler_app_->PrintResponse(response->body.Pass());
 }
 
 }  // namespace examples

mojo/services/public/interfaces/content_handler/content_handler.mojom

@@ -8,9 +8,9 @@ import "mojo/services/public/interfaces/network/url_loader.mojom"
 module mojo {
 
 interface ContentHandler {
-  OnConnect(string? url,
-            URLResponse? url_response,
-            ServiceProvider? service_provider);
+  OnConnect(string url,
+            URLResponse response,
+            ServiceProvider&? service_provider);
 };
 
 }

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,175 @@
 namespace mojo {
 namespace shell {
 
+// Encapsulates loading and running one individual application.
+//
+// Loaders are owned by DynamicApplicationLoader. DynamicApplicationLoader must
+// ensure that all the parameters passed to Loader subclasses stay valid through
+// Loader's lifetime.
+//
+// Async operations are done with WeakPtr to protect against
+// DynamicApplicationLoader going away (and taking all the Loaders with it)
+// while the async operation is outstanding.
+class DynamicApplicationLoader::Loader {
+ public:
+  Loader(Context* context,
+         DynamicServiceRunnerFactory* runner_factory,
+         scoped_refptr<ApplicationLoader::LoadCallbacks> load_callbacks,
+         const LoaderCompleteCallback& loader_complete_callback)
+      : load_callbacks_(load_callbacks),
+        loader_complete_callback_(loader_complete_callback),
+        context_(context),
+        runner_factory_(runner_factory),
+        weak_ptr_factory_(this) {}
+
+  virtual ~Loader() {}
+
+ protected:
+  void RunLibrary(const base::FilePath& path, bool path_exists) {
+    ScopedMessagePipeHandle shell_handle =
+        load_callbacks_->RegisterApplication();
+    if (!shell_handle.is_valid()) {
+      LoaderComplete();
+      return;
+    }
+
+    if (!path_exists) {
+      DVLOG(1) << "Library not started because library path '" << path.value()
+               << "' does not exist.";
+      LoaderComplete();
+      return;
+    }
+
+    runner_ = runner_factory_->Create(context_);
+    runner_->Start(
+        path,
+        shell_handle.Pass(),
+        base::Bind(&Loader::LoaderComplete, weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  void LoaderComplete() { loader_complete_callback_.Run(this); }
+
+  scoped_refptr<ApplicationLoader::LoadCallbacks> load_callbacks_;
+  LoaderCompleteCallback loader_complete_callback_;
+  Context* context_;
+
+ private:
+  DynamicServiceRunnerFactory* runner_factory_;
+  scoped_ptr<DynamicServiceRunner> runner_;
+  base::WeakPtrFactory<Loader> weak_ptr_factory_;
+};
+
+// A loader for local files.
+class DynamicApplicationLoader::LocalLoader : public Loader {
+ public:
+  LocalLoader(const GURL& url,
+              Context* context,
+              DynamicServiceRunnerFactory* runner_factory,
+              scoped_refptr<ApplicationLoader::LoadCallbacks> load_callbacks,
+              const LoaderCompleteCallback& loader_complete_callback)
+      : Loader(context,
+               runner_factory,
+               load_callbacks,
+               loader_complete_callback),
+        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 DynamicApplicationLoader::NetworkLoader : public Loader {
+ public:
+  NetworkLoader(const GURL& url,
+                MimeTypeToURLMap* mime_type_to_url,
+                Context* context,
+                DynamicServiceRunnerFactory* runner_factory,
+                NetworkService* network_service,
+                scoped_refptr<ApplicationLoader::LoadCallbacks> load_callbacks,
+                const LoaderCompleteCallback& loader_complete_callback)
+      : Loader(context,
+               runner_factory,
+               load_callbacks,
+               loader_complete_callback),
+        mime_type_to_url_(mime_type_to_url),
+        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;
+    }
+
+    network_service->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;
+      LoaderComplete();
+      return;
+    }
+
+    MimeTypeToURLMap::iterator iter =
+        mime_type_to_url_->find(response->mime_type);
+    if (iter != mime_type_to_url_->end()) {
+      load_callbacks_->LoadWithContentHandler(iter->second, response.Pass());
+      LoaderComplete();
+      return;
+    }
+
+    base::CreateTemporaryFile(&file_);
+    common::CopyToFile(
+        response->body.Pass(),
+        file_,
+        context_->task_runners()->blocking_pool(),
+        base::Bind(
+            &NetworkLoader::RunLibrary, weak_ptr_factory_.GetWeakPtr(), file_));
+  }
+
+  MimeTypeToURLMap* mime_type_to_url_;
+  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) {
+
+      // Unretained() is correct here because DynamicApplicationLoader owns the
+      // loaders that we pass this callback to.
+      loader_complete_callback_(
+          base::Bind(&DynamicApplicationLoader::LoaderComplete,
+                     base::Unretained(this))) {
 }
 
 DynamicApplicationLoader::~DynamicApplicationLoader() {
@@ -37,9 +201,10 @@ void DynamicApplicationLoader::RegisterContentHandler(
   mime_type_to_url_[mime_type] = content_handler_url;
 }
 
-void DynamicApplicationLoader::Load(ApplicationManager* manager,
-                                    const GURL& url,
-                                    scoped_refptr<LoadCallbacks> callbacks) {
+void DynamicApplicationLoader::Load(
+    ApplicationManager* manager,
+    const GURL& url,
+    scoped_refptr<LoadCallbacks> load_callbacks) {
   GURL resolved_url;
   if (url.SchemeIs("mojo")) {
     resolved_url = context_->mojo_url_resolver()->Resolve(url);
@@ -47,124 +212,27 @@ void DynamicApplicationLoader::Load(ApplicationManager* manager,
     resolved_url = url;
   }
 
-  if (resolved_url.SchemeIsFile())
-    LoadLocalService(resolved_url, callbacks);
-  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)));
-}
+  if (resolved_url.SchemeIsFile()) {
+    loaders_.push_back(new LocalLoader(resolved_url,
+                                       context_,
+                                       runner_factory_.get(),
+                                       load_callbacks,
+                                       loader_complete_callback_));
+    return;
+  }
 
-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,
+                                       &mime_type_to_url_,
+                                       context_,
+                                       runner_factory_.get(),
+                                       network_service_.get(),
+                                       load_callbacks,
+                                       loader_complete_callback_));
 }
 
 void DynamicApplicationLoader::OnApplicationError(ApplicationManager* manager,
@@ -173,5 +241,9 @@ void DynamicApplicationLoader::OnApplicationError(ApplicationManager* manager,
   // 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));
+}
+
 }  // namespace shell
 }  // namespace mojo

mojo/shell/dynamic_application_loader.h

@@ -7,6 +7,7 @@
 
 #include <map>
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
@@ -44,28 +45,21 @@ class DynamicApplicationLoader : public ApplicationLoader {
                                   const GURL& url) OVERRIDE;
 
  private:
+  class Loader;
+  class LocalLoader;
+  class NetworkLoader;
+
   typedef std::map<std::string, GURL> MimeTypeToURLMap;
+  typedef base::Callback<void(Loader*)> LoaderCompleteCallback;
 
-  void LoadLocalService(const GURL& resolved_url,
-                        scoped_refptr<LoadCallbacks> callbacks);
-  void LoadNetworkService(const GURL& resolved_url,
-                          scoped_refptr<LoadCallbacks> callbacks);
-  void OnLoadNetworkServiceComplete(scoped_refptr<LoadCallbacks> callbacks,
-                                    URLResponsePtr url_response);
-  void RunLibrary(const base::FilePath& response_file,
-                  scoped_refptr<LoadCallbacks> callbacks,
-                  bool delete_file_after,
-                  bool response_path_exists);
-  void OnRunLibraryComplete(DynamicServiceRunner* runner,
-                            const base::FilePath& temp_file);
+  void LoaderComplete(Loader* loader);
 
   Context* const context_;
   scoped_ptr<DynamicServiceRunnerFactory> runner_factory_;
-  ScopedVector<DynamicServiceRunner> runners_;
   NetworkServicePtr network_service_;
-  URLLoaderPtr url_loader_;
   MimeTypeToURLMap mime_type_to_url_;
-  base::WeakPtrFactory<DynamicApplicationLoader> weak_ptr_factory_;
+  ScopedVector<Loader> loaders_;
+  LoaderCompleteCallback loader_complete_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(DynamicApplicationLoader);
 };