Mojo: Replace RemotePtr with InterfacePtr and InterfaceImpl

mojo/service_manager/service_manager.cc

 // Copyright 2013 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 <stdio.h>
 
 #include "mojo/service_manager/service_manager.h"
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/stl_util.h"
 #include "mojo/public/cpp/bindings/allocation_scope.h"
-#include "mojo/public/cpp/bindings/error_handler.h"
-#include "mojo/public/cpp/bindings/remote_ptr.h"
 #include "mojo/service_manager/service_loader.h"
 
 namespace mojo {
 
 namespace {
 // Used by TestAPI.
 bool has_created_instance = false;
 }
 
-class ServiceManager::ServiceFactory : public Shell, public ErrorHandler {
+class ServiceManager::ServiceFactory : public Shell {
  public:
   ServiceFactory(ServiceManager* manager, const GURL& url)
       : manager_(manager),
-        url_(url) {
-    InterfacePipe<Shell> pipe;
-    shell_client_.reset(pipe.handle_to_peer.Pass(), this, this);
-    manager_->GetLoaderForURL(url)->LoadService(manager_,
-                                                url,
-                                                pipe.handle_to_self.Pass());
+        url_(url),
+        client_(NULL) {
   }
 
-  virtual ~ServiceFactory() {}
+  virtual ~ServiceFactory() {
+    if (manager_)
+      manager_->RemoveServiceFactory(this);
+  }
+
+  void Orphan() {

[DaveMoore, 2014/05/06 21:30:54]

It can be hard to track down bugs when you can either delete the ServiceFactory
directly or through the Manager. I used to have something just like this and it
got very confusing. What now requires it?

[darin (slow to review), 2014/05/06 21:39:11]

The lifetime of the ServiceFactory is now bound to the lifetime of the
MessagePipe used by the client to talk to its Shell instance.

+    manager_ = NULL;
+  }
 
   void ConnectToClient(ScopedMessagePipeHandle handle) {
-    if (handle.is_valid()) {
-      AllocationScope scope;
-      shell_client_->AcceptConnection(url_.spec(), handle.Pass());
+    if (!handle.is_valid()) {
+      assert(false);
+      return;
     }
+    AllocationScope scope;
+    client_->AcceptConnection(url_.spec(), handle.Pass());
   }
 
+  // Shell implementation:
+
+  virtual void SetClient(ShellClient* client) OVERRIDE {
+    client_ = client;
+  }
+
+  virtual void Connect(const String& url,
+                       ScopedMessagePipeHandle client_pipe) OVERRIDE {
+    if (manager_)
+      manager_->Connect(GURL(url.To<std::string>()), client_pipe.Pass());
+  }
+
+  const GURL& url() const { return url_; }
+
+ private:
+  ServiceManager* manager_;
+  const GURL url_;
+  ShellClient* client_;
+
+  DISALLOW_COPY_AND_ASSIGN(ServiceFactory);
+};
+
+class ServiceManager::TestAPI::TestShellConnection : public Shell {
+ public:
+  explicit TestShellConnection(ServiceManager* manager)
+      : manager_(manager),
+        client_(NULL) {
+  }
+  virtual ~TestShellConnection() {}
+
+  // Shell:
+  virtual void SetClient(ShellClient* client) OVERRIDE {
+    client_ = client;
+  }
   virtual void Connect(const String& url,
                        ScopedMessagePipeHandle client_pipe) OVERRIDE {
     manager_->Connect(GURL(url.To<std::string>()), client_pipe.Pass());
   }
 
-  virtual void OnError() OVERRIDE {
-    manager_->OnServiceFactoryError(this);
-  }
-
-  const GURL& url() const { return url_; }
-
- private:
-  ServiceManager* const manager_;
-  const GURL url_;
-  RemotePtr<ShellClient> shell_client_;
-
-  DISALLOW_COPY_AND_ASSIGN(ServiceFactory);
-};
-
-class ServiceManager::TestAPI::TestShellConnection
-    : public Shell,
-      public ErrorHandler {
- public:
-  explicit TestShellConnection(ServiceManager* manager) : manager_(manager) {
-    InterfacePipe<Shell> pipe;
-    shell_client_.reset(pipe.handle_to_peer.Pass(), this, this);
-    shell_handle_ = pipe.handle_to_self.Pass();
-  }
-  virtual ~TestShellConnection() {}
-
-  ScopedShellHandle GetShellHandle() {
-    return shell_handle_.Pass();
-  }
-
-  // Shell:
-  virtual void Connect(const String& url,
-                       ScopedMessagePipeHandle client_pipe) OVERRIDE {
-    manager_->Connect(GURL(url.To<std::string>()), client_pipe.Pass());
-  }
-
-  virtual void OnError() OVERRIDE {
-  }
-
  private:
   ServiceManager* manager_;
-  RemotePtr<ShellClient> shell_client_;
-  ScopedShellHandle shell_handle_;
+  ShellClient* client_;
 
   DISALLOW_COPY_AND_ASSIGN(TestShellConnection);
 };
 
 // static
 ServiceManager::TestAPI::TestAPI(ServiceManager* manager) : manager_(manager) {
 }
 
 ServiceManager::TestAPI::~TestAPI() {
 }
 
 bool ServiceManager::TestAPI::HasCreatedInstance() {
   return has_created_instance;
 }
 
-ScopedShellHandle ServiceManager::TestAPI::GetShellHandle() {
-  if (!shell_connection_.get())
-    shell_connection_.reset(new TestShellConnection(manager_));
-  return shell_connection_->GetShellHandle().Pass();
+ScopedMessagePipeHandle ServiceManager::TestAPI::GetShellHandle() {
+  MessagePipe pipe;
+  shell_.Bind(new TestShellConnection(manager_));
+  shell_.ConfigureStub(pipe.handle0.Pass());
+  return pipe.handle1.Pass();
 }
 
 bool ServiceManager::TestAPI::HasFactoryForURL(const GURL& url) const {
   return manager_->url_to_service_factory_.find(url) !=
-      manager_->url_to_service_factory_.end();
+         manager_->url_to_service_factory_.end();
 }
 
 ServiceManager::ServiceManager()
     : interceptor_(NULL) {
 }
 
 ServiceManager::~ServiceManager() {
-  STLDeleteValues(&url_to_service_factory_);
+  // Make sure any lingering service factories no longer reference us.
+  for (URLToServiceFactoryMap::iterator it = url_to_service_factory_.begin();
+       it != url_to_service_factory_.end(); ++it) {
+    it->second->Orphan();
+  }
+
   STLDeleteValues(&url_to_loader_);
   STLDeleteValues(&scheme_to_loader_);
 }
 
 // static
 ServiceManager* ServiceManager::GetInstance() {
   static base::LazyInstance<ServiceManager> instance =
       LAZY_INSTANCE_INITIALIZER;
   has_created_instance = true;
   return &instance.Get();
 }
 
 void ServiceManager::Connect(const GURL& url,
                              ScopedMessagePipeHandle client_handle) {
   URLToServiceFactoryMap::const_iterator service_it =
       url_to_service_factory_.find(url);
   ServiceFactory* service_factory;
   if (service_it != url_to_service_factory_.end()) {
     service_factory = service_it->second;
   } else {
     service_factory = new ServiceFactory(this, url);
+
+    GetLoaderForURL(url)->LoadService(
+        this, url, BindToPipe<Shell>(service_factory).Pass());
+
+    // NOTE: The lifetime of the service_factory instance is now bound to the
+    // MessagePipe passed to LoadService. We retain a raw pointer to
+    // service_factory so that we can continue communicating with it.
+
     url_to_service_factory_[url] = service_factory;
   }
   if (interceptor_) {
     service_factory->ConnectToClient(
         interceptor_->OnConnectToClient(url, client_handle.Pass()));
   } else {
     service_factory->ConnectToClient(client_handle.Pass());
   }
 }
 
@@ skipping 22 matching lines @@
   if (url_it != url_to_loader_.end())
     return url_it->second;
   SchemeToLoaderMap::const_iterator scheme_it =
       scheme_to_loader_.find(url.scheme());
   if (scheme_it != scheme_to_loader_.end())
     return scheme_it->second;
   DCHECK(default_loader_);
   return default_loader_.get();
 }
 
-void ServiceManager::OnServiceFactoryError(ServiceFactory* service_factory) {
+void ServiceManager::RemoveServiceFactory(ServiceFactory* service_factory) {

[DaveMoore, 2014/05/06 21:30:54]

This is called OnServiceFactoryError() because it sends OnServiceError() to the
Loader. Either that name should be changed as well, or this should remain.

[darin (slow to review), 2014/05/06 21:39:11]

I just had service.h on the mind. Renamed back to OnServiceFactoryError.

   const GURL url = service_factory->url();
   URLToServiceFactoryMap::iterator it = url_to_service_factory_.find(url);
   DCHECK(it != url_to_service_factory_.end());
-  delete it->second;
   url_to_service_factory_.erase(it);
   GetLoaderForURL(url)->OnServiceError(this, url);
 }
 
 }  // namespace mojo