Cleanup when clients go away.

We delete Service<> implementations when we get errors from the client
connection. When all Services are deleted we sever the connection from the
ServiceFactory<> to the Shell. That allows it to clean up its reference to
the factory.

BUG=None
TEST=ServiceConnectorTest.ClientError
R=darin@chromium.org, darin

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@249422 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 90 insertions, 19 deletions

diff --git a/mojo/shell/service_connector.cc b/mojo/shell/service_connector.cc
index afbb804..c073c6e 100644
--- a/mojo/shell/service_connector.cc
+++ b/mojo/shell/service_connector.cc
@@ -5,19 +5,20 @@
 #include "mojo/shell/service_connector.h"
 
 #include "base/logging.h"
+#include "mojo/public/bindings/error_handler.h"
 #include "mojo/public/bindings/remote_ptr.h"
 #include "mojom/shell.h"
 
 namespace mojo {
 namespace shell {
 
-class ServiceConnector::ServiceFactory : public Shell {
+class ServiceConnector::ServiceFactory : public Shell, public ErrorHandler {
  public:
   ServiceFactory(ServiceConnector* connector, const GURL& url)
       : connector_(connector),
         url_(url) {
     MessagePipe pipe;
-    shell_client_.reset(pipe.handle0.Pass(), this);
+    shell_client_.reset(pipe.handle0.Pass(), this, this);
     connector_->GetLoaderForURL(url)->Load(url, pipe.handle1.Pass());
   }
   virtual ~ServiceFactory() {}
@@ -32,6 +33,12 @@ class ServiceConnector::ServiceFactory : public Shell {
     connector_->Connect(GURL(url.To<std::string>()), client_pipe.Pass());
   }
 
+  virtual void OnError() MOJO_OVERRIDE {
+    connector_->RemoveServiceFactory(this);
+  }
+
+  GURL url() const { return url_; }
+
  private:
   ServiceConnector* connector_;
   GURL url_;
@@ -42,6 +49,11 @@ class ServiceConnector::ServiceFactory : public Shell {
 ServiceConnector::Loader::Loader() {}
 ServiceConnector::Loader::~Loader() {}
 
+bool ServiceConnector::TestAPI::HasFactoryForURL(const GURL& url) const {
+  return connector_->url_to_service_factory_.find(url) !=
+      connector_->url_to_service_factory_.end();
+}
+
 ServiceConnector::ServiceConnector() : default_loader_(NULL) {
 }
 
@@ -80,5 +92,13 @@ void ServiceConnector::Connect(const GURL& url,
   service_factory->ConnectToClient(client_handle.Pass());
 }
 
+void ServiceConnector::RemoveServiceFactory(ServiceFactory* service_factory) {
+  ServiceFactoryMap::iterator it =
+      url_to_service_factory_.find(service_factory->url());
+  DCHECK(it != url_to_service_factory_.end());
+  delete it->second;
+  url_to_service_factory_.erase(it);
+}
+
 }  // namespace shell
 }  // namespace mojo
diff --git a/mojo/shell/service_connector.h b/mojo/shell/service_connector.h
index 725b119..6b338b6 100644
--- a/mojo/shell/service_connector.h
+++ b/mojo/shell/service_connector.h
@@ -28,6 +28,17 @@ class ServiceConnector {
     Loader();
   };
 
+  // API for testing.
+  class TestAPI {
+   private:
+    friend class ServiceConnectorTest;
+    explicit TestAPI(ServiceConnector* connector) : connector_(connector) {}
+    // Returns true if there is a ServiceFactory for this URL.
+    bool HasFactoryForURL(const GURL& url) const;
+
+    ServiceConnector* connector_;
+  };
+
   ServiceConnector();
   ~ServiceConnector();
 
@@ -41,10 +52,12 @@ class ServiceConnector {
   Loader* GetLoaderForURL(const GURL& gurl);
   // Loads a service if necessary and establishes a new client connection.
   void Connect(const GURL& url, ScopedMessagePipeHandle client_handle);
-
  private:
   class ServiceFactory;
 
+  // Removes a ServiceFactory when it no longer has any connections.
+  void RemoveServiceFactory(ServiceFactory* service_factory);
+
   Loader* default_loader_;
   typedef std::map<GURL, ServiceFactory*> ServiceFactoryMap;
   ServiceFactoryMap url_to_service_factory_;
diff --git a/mojo/shell/service_connector_unittest.cc b/mojo/shell/service_connector_unittest.cc
index 6ec7f07..2af4f10 100644
--- a/mojo/shell/service_connector_unittest.cc
+++ b/mojo/shell/service_connector_unittest.cc
@@ -2,10 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/message_loop/message_loop.h"
 #include "mojo/public/bindings/allocation_scope.h"
 #include "mojo/public/bindings/remote_ptr.h"
+#include "mojo/public/environment/environment.h"
 #include "mojo/public/shell/service.h"
+#include "mojo/public/utility/run_loop.h"
 #include "mojo/shell/service_connector.h"
 #include "mojom/shell.h"
 #include "mojom/test.h"
@@ -15,16 +16,34 @@ namespace mojo {
 namespace shell {
 namespace {
 
-struct Context {
+const char kTestURLString[] = "test:testService";
+
+struct TestContext {
+  TestContext() : num_impls(0) {}
   std::string last_test_string;
+  int num_impls;
 };
 
-class TestServiceImpl : public Service<TestService, TestServiceImpl, Context> {
+class TestServiceImpl :
+    public Service<TestService, TestServiceImpl, TestContext> {
  public:
+  TestServiceImpl() {}
+
+  virtual ~TestServiceImpl() {
+    --context()->num_impls;
+  }
+
   virtual void Test(const mojo::String& test_string) OVERRIDE {
     context()->last_test_string = test_string.To<std::string>();
     client()->AckTest();
   }
+
+  void Initialize(ServiceFactory<TestServiceImpl, TestContext>* service_factory,
+                  ScopedMessagePipeHandle client_handle) {
+    Service<TestService, TestServiceImpl, TestContext>::Initialize(
+        service_factory, client_handle.Pass());
+    ++context()->num_impls;
+  }
 };
 
 class TestClientImpl : public TestClient {
@@ -33,32 +52,36 @@ class TestClientImpl : public TestClient {
       : service_(service_handle.Pass(), this),
         quit_after_ack_(false) {
   }
-  virtual ~TestClientImpl() {
-  }
+
+  virtual ~TestClientImpl() {}
+
   virtual void AckTest() OVERRIDE {
     if (quit_after_ack_)
-      base::MessageLoop::current()->QuitNow();
+      mojo::RunLoop::current()->Quit();
   }
+
   void Test(std::string test_string) {
     AllocationScope scope;
     quit_after_ack_ = true;
     service_->Test(mojo::String(test_string));
   }
+
+ private:
   RemotePtr<TestService> service_;
   bool quit_after_ack_;
+  DISALLOW_COPY_AND_ASSIGN(TestClientImpl);
 };
+}  // namespace
 
 class ServiceConnectorTest : public testing::Test,
                              public ServiceConnector::Loader {
  public:
-  ServiceConnectorTest() {
-  }
+  ServiceConnectorTest() {}
 
-  virtual ~ServiceConnectorTest() {
-  }
+  virtual ~ServiceConnectorTest() {}
 
   virtual void SetUp() OVERRIDE {
-    GURL test_url("test:testService");
+    GURL test_url(kTestURLString);
     service_connector_.reset(new ServiceConnector);
     service_connector_->SetLoaderForURL(this, test_url);
     MessagePipe pipe;
@@ -74,14 +97,20 @@ class ServiceConnectorTest : public testing::Test,
 
   virtual void Load(const GURL& url,
                     ScopedMessagePipeHandle shell_handle) OVERRIDE {
-    test_app_.reset(new ServiceFactory<TestServiceImpl, Context>(
+    test_app_.reset(new ServiceFactory<TestServiceImpl, TestContext>(
         shell_handle.Pass(), &context_));
   }
 
+  bool HasFactoryForTestURL() {
+    ServiceConnector::TestAPI connector_test_api(service_connector_.get());
+    return connector_test_api.HasFactoryForURL(GURL(kTestURLString));
+  }
+
  protected:
-  base::MessageLoop loop_;
-  Context context_;
-  scoped_ptr<ServiceFactory<TestServiceImpl, Context> > test_app_;
+  mojo::Environment env_;
+  mojo::RunLoop loop_;
+  TestContext context_;
+  scoped_ptr<ServiceFactory<TestServiceImpl, TestContext> > test_app_;
   scoped_ptr<TestClientImpl> test_client_;
   scoped_ptr<ServiceConnector> service_connector_;
   DISALLOW_COPY_AND_ASSIGN(ServiceConnectorTest);
@@ -93,6 +122,15 @@ TEST_F(ServiceConnectorTest, Basic) {
   EXPECT_EQ(std::string("test"), context_.last_test_string);
 }
 
-}  // namespace
+TEST_F(ServiceConnectorTest, ClientError) {
+  test_client_->Test("test");
+  EXPECT_TRUE(HasFactoryForTestURL());
+  loop_.Run();
+  EXPECT_EQ(1, context_.num_impls);
+  test_client_.reset(NULL);
+  loop_.Run();
+  EXPECT_EQ(0, context_.num_impls);
+  EXPECT_FALSE(HasFactoryForTestURL());
+}
 }  // namespace shell
 }  // namespace mojo