Mandoline: Move ContentHandlerConnection into separate file

ContentHandlerConnection will serve as the equivalent of SiteInstance in Chromium.

In the future, ContentHandlerConnection will represent an Application instance for a particular origin/site.

As the complexity of this object will grow soon, it makes sense to move it out as a first step.

ContentHandlerConnection now also uses the same lifetime management idiom used in ApplicationConnection and elsewhere to help guard against reentrant calls during destruction and double deletes. This also makes it easier to unit test this class in the future as more functionality is added.

BUG=496937

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

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

7 files changed, 144 insertions, 82 deletions

diff --git a/mojo/shell/BUILD.gn b/mojo/shell/BUILD.gn
index 51f3edf..7cf6a27 100644
--- a/mojo/shell/BUILD.gn
+++ b/mojo/shell/BUILD.gn
@@ -11,6 +11,8 @@ source_set("shell") {
     "application_loader.h",
     "application_manager.cc",
     "application_manager.h",
+    "content_handler_connection.cc",
+    "content_handler_connection.h",
     "data_pipe_peek.cc",
     "data_pipe_peek.h",
     "fetcher.cc",
diff --git a/mojo/shell/application_manager.cc b/mojo/shell/application_manager.cc
index d953a90..4602a68 100644
--- a/mojo/shell/application_manager.cc
+++ b/mojo/shell/application_manager.cc
@@ -14,6 +14,7 @@
 #include "mojo/application/public/interfaces/content_handler.mojom.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/bindings/error_handler.h"
+#include "mojo/shell/content_handler_connection.h"
 #include "mojo/shell/fetcher.h"
 #include "mojo/shell/local_fetcher.h"
 #include "mojo/shell/network_fetcher.h"
@@ -32,45 +33,6 @@ bool has_created_instance = false;
 
 }  // namespace
 
-class ApplicationManager::ContentHandlerConnection : public ErrorHandler {
- public:
-  ContentHandlerConnection(ApplicationManager* manager,
-                           const GURL& content_handler_url,
-                           const GURL& requestor_url,
-                           const std::string& qualifier)
-      : manager_(manager),
-        content_handler_url_(content_handler_url),
-        content_handler_qualifier_(qualifier) {
-    ServiceProviderPtr services;
-    mojo::URLRequestPtr request(mojo::URLRequest::New());
-    request->url = mojo::String::From(content_handler_url.spec());
-    manager->ConnectToApplicationInternal(
-        request.Pass(), qualifier, requestor_url, GetProxy(&services),
-        nullptr, base::Closure());
-    MessagePipe pipe;
-    content_handler_.Bind(
-        InterfacePtrInfo<ContentHandler>(pipe.handle0.Pass(), 0u));
-    services->ConnectToService(ContentHandler::Name_, pipe.handle1.Pass());
-    content_handler_.set_error_handler(this);
-  }
-
-  ContentHandler* content_handler() { return content_handler_.get(); }
-
-  GURL content_handler_url() { return content_handler_url_; }
-  std::string content_handler_qualifier() { return content_handler_qualifier_; }
-
- private:
-  // ErrorHandler implementation:
-  void OnConnectionError() override { manager_->OnContentHandlerError(this); }
-
-  ApplicationManager* manager_;
-  GURL content_handler_url_;
-  std::string content_handler_qualifier_;
-  ContentHandlerPtr content_handler_;
-
-  DISALLOW_COPY_AND_ASSIGN(ContentHandlerConnection);
-};
-
 // static
 ApplicationManager::TestAPI::TestAPI(ApplicationManager* manager)
     : manager_(manager) {
@@ -93,7 +55,9 @@ ApplicationManager::ApplicationManager(Delegate* delegate)
 }
 
 ApplicationManager::~ApplicationManager() {
-  STLDeleteValues(&url_to_content_handler_);
+  URLToContentHandlerMap url_to_content_handler(url_to_content_handler_);
+  for (auto& pair : url_to_content_handler)
+    pair.second->CloseConnection();
   TerminateShellConnections();
   STLDeleteValues(&url_to_loader_);
   STLDeleteValues(&scheme_to_loader_);
@@ -105,17 +69,6 @@ void ApplicationManager::TerminateShellConnections() {
 
 void ApplicationManager::ConnectToApplication(
     mojo::URLRequestPtr requested_url,
-    const GURL& requestor_url,
-    InterfaceRequest<ServiceProvider> services,
-    ServiceProviderPtr exposed_services,
-    const base::Closure& on_application_end) {
-  ConnectToApplicationInternal(
-      requested_url.Pass(), std::string(), requestor_url, services.Pass(),
-      exposed_services.Pass(), on_application_end);
-}
-
-void ApplicationManager::ConnectToApplicationInternal(
-    mojo::URLRequestPtr requested_url,
     const std::string& qualifier,
     const GURL& requestor_url,
     InterfaceRequest<ServiceProvider> services,
@@ -313,9 +266,9 @@ void ApplicationManager::HandleFetchCallback(
     header->name = "Referer";
     header->value = fetcher->GetRedirectReferer().spec();
     request->headers.push_back(header.Pass());
-    ConnectToApplicationInternal(request.Pass(), qualifier, requestor_url,
-                                 services.Pass(), exposed_services.Pass(),
-                                 on_application_end);
+    ConnectToApplication(request.Pass(), qualifier, requestor_url,
+                         services.Pass(), exposed_services.Pass(),
+                         on_application_end);
     return;
   }
 
@@ -523,14 +476,13 @@ void ApplicationManager::OnShellImplError(ShellImpl* shell_impl) {
     on_application_end.Run();
 }
 
-void ApplicationManager::OnContentHandlerError(
+void ApplicationManager::OnContentHandlerConnectionClosed(
     ContentHandlerConnection* content_handler) {
   // Remove the mapping to the content handler.
   auto it = url_to_content_handler_.find(
       std::make_pair(content_handler->content_handler_url(),
                      content_handler->content_handler_qualifier()));
   DCHECK(it != url_to_content_handler_.end());
-  delete it->second;
   url_to_content_handler_.erase(it);
 }
 
@@ -540,8 +492,8 @@ ScopedMessagePipeHandle ApplicationManager::ConnectToServiceByName(
   ServiceProviderPtr services;
   mojo::URLRequestPtr request(mojo::URLRequest::New());
   request->url = mojo::String::From(application_url.spec());
-  ConnectToApplication(request.Pass(), GURL(), GetProxy(&services), nullptr,
-                       base::Closure());
+  ConnectToApplication(request.Pass(), std::string(), GURL(),
+                       GetProxy(&services), nullptr, base::Closure());
   MessagePipe pipe;
   services->ConnectToService(interface_name, pipe.handle1.Pass());
   return pipe.handle0.Pass();
diff --git a/mojo/shell/application_manager.h b/mojo/shell/application_manager.h
index 727b9e6..5733b6b 100644
--- a/mojo/shell/application_manager.h
+++ b/mojo/shell/application_manager.h
@@ -31,6 +31,7 @@ class SequencedWorkerPool;
 namespace mojo {
 namespace shell {
 
+class ContentHandlerConnection;
 class ShellImpl;
 
 class ApplicationManager {
@@ -76,11 +77,14 @@ class ApplicationManager {
   ~ApplicationManager();
 
   // Loads a service if necessary and establishes a new client connection.
-  void ConnectToApplication(mojo::URLRequestPtr application_url,
-                            const GURL& requestor_url,
-                            InterfaceRequest<ServiceProvider> services,
-                            ServiceProviderPtr exposed_services,
-                            const base::Closure& on_application_end);
+  void ConnectToApplication(
+      mojo::URLRequestPtr requested_url,
+      const std::string& qualifier,
+      const GURL& requestor_url,
+      InterfaceRequest<ServiceProvider> services,
+      ServiceProviderPtr exposed_services,
+      const base::Closure& on_application_end);
+
 
   template <typename Interface>
   inline void ConnectToService(const GURL& application_url,
@@ -143,9 +147,11 @@ class ApplicationManager {
   // Removes a ShellImpl when it encounters an error.
   void OnShellImplError(ShellImpl* shell_impl);
 
- private:
-  class ContentHandlerConnection;
+  // Removes a ContentHandler when its connection is closed.
+  void OnContentHandlerConnectionClosed(
+      ContentHandlerConnection* content_handler);
 
+ private:
   using ApplicationPackagedAlias = std::map<GURL, std::pair<GURL, std::string>>;
   using IdentityToShellImplMap = std::map<Identity, ShellImpl*>;
   using MimeTypeToURLMap = std::map<std::string, GURL>;
@@ -155,14 +161,6 @@ class ApplicationManager {
   using URLToLoaderMap = std::map<GURL, ApplicationLoader*>;
   using URLToNativeOptionsMap = std::map<GURL, NativeRunnerFactory::Options>;
 
-  void ConnectToApplicationInternal(
-      mojo::URLRequestPtr requested_url,
-      const std::string& qualifier,
-      const GURL& requestor_url,
-      InterfaceRequest<ServiceProvider> services,
-      ServiceProviderPtr exposed_services,
-      const base::Closure& on_application_end);
-
   bool ConnectToRunningApplication(const GURL& resolved_url,
                                    const std::string& qualifier,
                                    const GURL& requestor_url,
@@ -223,9 +221,6 @@ class ApplicationManager {
   // configured for the URL.
   ApplicationLoader* GetLoaderForURL(const GURL& url);
 
-  // Removes a ContentHandler when it encounters an error.
-  void OnContentHandlerError(ContentHandlerConnection* content_handler);
-
   void CleanupRunner(NativeRunner* runner);
 
   Delegate* const delegate_;
diff --git a/mojo/shell/application_manager_unittest.cc b/mojo/shell/application_manager_unittest.cc
index bc32a39..c5f883e 100644
--- a/mojo/shell/application_manager_unittest.cc
+++ b/mojo/shell/application_manager_unittest.cc
@@ -773,7 +773,7 @@ TEST_F(ApplicationManagerTest, TestEndApplicationClosure) {
   mojo::URLRequestPtr request(mojo::URLRequest::New());
   request->url = mojo::String::From("test:test");
   application_manager_->ConnectToApplication(
-      request.Pass(), GURL(), nullptr, nullptr,
+      request.Pass(), std::string(), GURL(), nullptr, nullptr,
       base::Bind(&QuitClosure, base::Unretained(&called)));
   loop_.Run();
   EXPECT_TRUE(called);
@@ -800,7 +800,7 @@ TEST(ApplicationManagerTest2, ContentHandlerConnectionGetsRequestorURL) {
   mojo::URLRequestPtr request(mojo::URLRequest::New());
   request->url = mojo::String::From("test:test");
   application_manager.ConnectToApplication(
-      request.Pass(), requestor_url, nullptr, nullptr,
+      request.Pass(), std::string(), requestor_url, nullptr, nullptr,
       base::Bind(&QuitClosure, base::Unretained(&called)));
   loop.Run();
   EXPECT_TRUE(called);
diff --git a/mojo/shell/content_handler_connection.cc b/mojo/shell/content_handler_connection.cc
new file mode 100644
index 0000000..2de5797
--- /dev/null
+++ b/mojo/shell/content_handler_connection.cc
@@ -0,0 +1,53 @@
+// Copyright 2015 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/content_handler_connection.h"
+
+#include "mojo/shell/application_manager.h"
+
+namespace mojo {
+namespace shell {
+
+ContentHandlerConnection::ContentHandlerConnection(
+    ApplicationManager* manager,
+    const GURL& content_handler_url,
+    const GURL& requestor_url,
+    const std::string& qualifier)
+    : manager_(manager),
+      content_handler_url_(content_handler_url),
+      content_handler_qualifier_(qualifier),
+      connection_closed_(false) {
+  ServiceProviderPtr services;
+  mojo::URLRequestPtr request(mojo::URLRequest::New());
+  request->url = mojo::String::From(content_handler_url.spec());
+  manager->ConnectToApplication(
+      request.Pass(), qualifier, requestor_url, GetProxy(&services),
+      nullptr, base::Closure());
+  MessagePipe pipe;
+  content_handler_.Bind(
+      InterfacePtrInfo<ContentHandler>(pipe.handle0.Pass(), 0u));
+  services->ConnectToService(ContentHandler::Name_, pipe.handle1.Pass());
+  content_handler_.set_error_handler(this);
+}
+
+void ContentHandlerConnection::CloseConnection() {
+  if (connection_closed_)
+    return;
+  connection_closed_ = true;
+  manager_->OnContentHandlerConnectionClosed(this);
+  delete this;
+}
+
+ContentHandlerConnection::~ContentHandlerConnection() {
+  // If this DCHECK fails then something has tried to delete this object without
+  // calling CloseConnection.
+  DCHECK(connection_closed_);
+}
+
+void ContentHandlerConnection::OnConnectionError() {
+  CloseConnection();
+}
+
+}  // namespace shell
+}  // namespace mojo
diff --git a/mojo/shell/content_handler_connection.h b/mojo/shell/content_handler_connection.h
new file mode 100644
index 0000000..af70493
--- /dev/null
+++ b/mojo/shell/content_handler_connection.h
@@ -0,0 +1,59 @@
+// Copyright 2015 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.
+
+#ifndef MOJO_SHELL_CONTENT_HANDLER_CONNECTION_H_
+#define MOJO_SHELL_CONTENT_HANDLER_CONNECTION_H_
+
+#include <string>
+
+#include "mojo/application/public/interfaces/content_handler.mojom.h"
+#include "mojo/public/cpp/bindings/error_handler.h"
+#include "url/gurl.h"
+
+namespace mojo {
+namespace shell {
+
+class ApplicationManager;
+
+// A ContentHandlerConnection is responsible for creating and maintaining a
+// connection to an app which provides the ContentHandler service.
+// A ContentHandlerConnection can only be destroyed via CloseConnection.
+// A ContentHandlerConnection manages its own lifetime and cannot be used with
+// a scoped_ptr to avoid reentrant calls into ApplicationManager late in
+// destruction.
+class ContentHandlerConnection : public ErrorHandler {
+ public:
+  ContentHandlerConnection(ApplicationManager* manager,
+                           const GURL& content_handler_url,
+                           const GURL& requestor_url,
+                           const std::string& qualifier);
+
+  // Closes the connection and destorys |this| object.
+  void CloseConnection();
+
+  ContentHandler* content_handler() { return content_handler_.get(); }
+  const GURL& content_handler_url() { return content_handler_url_; }
+  const std::string& content_handler_qualifier() {
+    return content_handler_qualifier_;
+  }
+
+ private:
+  ~ContentHandlerConnection() override;
+
+  // ErrorHandler implementation:
+  void OnConnectionError() override;
+
+  ApplicationManager* manager_;
+  GURL content_handler_url_;
+  std::string content_handler_qualifier_;
+  ContentHandlerPtr content_handler_;
+  bool connection_closed_;
+
+  DISALLOW_COPY_AND_ASSIGN(ContentHandlerConnection);
+};
+
+}  // namespace shell
+}  // namespace mojo
+
+#endif  // MOJO_SHELL_CONTENT_HANDLER_CONNECTION_H_
diff --git a/mojo/shell/shell_impl.cc b/mojo/shell/shell_impl.cc
index 3dd5249..4b10471 100644
--- a/mojo/shell/shell_impl.cc
+++ b/mojo/shell/shell_impl.cc
@@ -70,9 +70,9 @@ void ShellImpl::ConnectToApplication(mojo::URLRequestPtr app_request,
     LOG(ERROR) << "Error: invalid URL: " << app_request;
     return;
   }
-  manager_->ConnectToApplication(app_request.Pass(), identity_.url,
-                                 services.Pass(), exposed_services.Pass(),
-                                 base::Closure());
+  manager_->ConnectToApplication(app_request.Pass(), std::string(),
+                                 identity_.url, services.Pass(),
+                                 exposed_services.Pass(), base::Closure());
 }
 
 void ShellImpl::QuitApplication() {
@@ -93,7 +93,8 @@ void ShellImpl::OnConnectionError() {
   for (auto request : queued_client_requests) {
     mojo::URLRequestPtr url(mojo::URLRequest::New());
     url->url = mojo::String::From(request->requested_url.spec());
-    manager->ConnectToApplication(url.Pass(), request->requestor_url,
+    manager->ConnectToApplication(url.Pass(), std::string(),
+                                  request->requestor_url,
                                   request->services.Pass(),
                                   request->exposed_services.Pass(),
                                   base::Closure());