Merge 82366 - PpapiBrokerImpl manages its own lifetime based on Connect and Disconnect calls.

Previously, the PluginModule still had a reference to the PpapiBrokerImpl, so the broker channel was not destroyed until the module was. Now, it is destroyed when the instance is or a plugin releases all references to PPB_BrokerTrusted.

This change also makes the PpapiBrokerImpl's reference to PPB_Broker_Impl a WeakPtr so that the PPB_Broker_Impl can be destroyed even if the PpapiBrokerImpl is not. This prevents a scenario where neither would be destroyed if the broker channel was never established and pending_connects_ was never cleared.

BUG=81335
TEST=Add the following to the PPB_BrokerTrusted::Connect callback: pp::Module::Get()->core()->ReleaseResource(broker_resource);. The broker process should exit in 30 seconds while the plugin process keeps running.

Review URL: http://codereview.chromium.org/6881033

TBR=ddorwin@chromium.org

git-svn-id: svn://svn.chromium.org/chrome/branches/742/src@83956 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 114 insertions, 38 deletions

diff --git a/content/renderer/pepper_plugin_delegate_impl.cc b/content/renderer/pepper_plugin_delegate_impl.cc
index b137536..2f0a99b 100644
--- a/content/renderer/pepper_plugin_delegate_impl.cc
+++ b/content/renderer/pepper_plugin_delegate_impl.cc
@@ -409,16 +409,43 @@ int32_t BrokerDispatcherWrapper::SendHandleToBroker(
   return PP_OK;
 }
 
-PpapiBrokerImpl::PpapiBrokerImpl() {
+PpapiBrokerImpl::PpapiBrokerImpl(webkit::ppapi::PluginModule* plugin_module)
+    : plugin_module_(plugin_module) {
+  plugin_module->SetBroker(this);
 }
 
 PpapiBrokerImpl::~PpapiBrokerImpl() {
+  // Report failure to all clients that had pending operations.
+  for (ClientMap::iterator i = pending_connects_.begin();
+       i != pending_connects_.end(); ++i) {
+    base::WeakPtr<webkit::ppapi::PPB_Broker_Impl>& weak_ptr = i->second;
+    if (weak_ptr) {
+      weak_ptr->BrokerConnected(
+          PlatformFileToInt(base::kInvalidPlatformFileValue), PP_ERROR_ABORTED);
+    }
+  }
+  pending_connects_.clear();
+
+  plugin_module_->SetBroker(NULL);
+  plugin_module_ = NULL;
 }
 
 // If the channel is not ready, queue the connection.
 void PpapiBrokerImpl::Connect(webkit::ppapi::PPB_Broker_Impl* client) {
+  DCHECK(pending_connects_.find(client) == pending_connects_.end())
+      << "Connect was already called for this client";
+
+  // Ensure this object and the associated broker exist as long as the
+  // client exists. There is a corresponding Release() call in Disconnect(),
+  // which is called when the PPB_Broker_Impl is destroyed. The only other
+  // possible reference is in pending_connect_broker_, which only holds a
+  // transient reference. This ensures the broker is available as long as the
+  // plugin needs it and allows the plugin to release the broker when it is no
+  // longer using it.
+  AddRef();
+
   if (!dispatcher_.get()) {
-    pending_connects_.push_back(client);
+    pending_connects_[client] = client->AsWeakPtr();
     return;
   }
   DCHECK(pending_connects_.empty());
@@ -427,8 +454,15 @@ void PpapiBrokerImpl::Connect(webkit::ppapi::PPB_Broker_Impl* client) {
 }
 
 void PpapiBrokerImpl::Disconnect(webkit::ppapi::PPB_Broker_Impl* client) {
+  // Remove the pending connect if one exists. This class will not call client's
+  // callback.
+  pending_connects_.erase(client);
+
   // TODO(ddorwin): Send message using dispatcher_ and clean up any pending
   // connects or pipes.
+
+  // Release the reference added in Connect().
+  Release();
 }
 
 void PpapiBrokerImpl::OnBrokerChannelConnected(
@@ -439,13 +473,22 @@ void PpapiBrokerImpl::OnBrokerChannelConnected(
     dispatcher_.reset(dispatcher.release());
 
     // Process all pending channel requests from the renderers.
-    for (size_t i = 0; i < pending_connects_.size(); i++)
-      ConnectPluginToBroker(pending_connects_[i]);
+    for (ClientMap::iterator i = pending_connects_.begin();
+         i != pending_connects_.end(); ++i) {
+      base::WeakPtr<webkit::ppapi::PPB_Broker_Impl>& weak_ptr = i->second;
+      if (weak_ptr)
+        ConnectPluginToBroker(weak_ptr);
+    }
   } else {
     // Report failure to all clients.
-    for (size_t i = 0; i < pending_connects_.size(); i++) {
-      pending_connects_[i]->BrokerConnected(
-          PlatformFileToInt(base::kInvalidPlatformFileValue), PP_ERROR_FAILED);
+    for (ClientMap::iterator i = pending_connects_.begin();
+         i != pending_connects_.end(); ++i) {
+      base::WeakPtr<webkit::ppapi::PPB_Broker_Impl>& weak_ptr = i->second;
+      if (weak_ptr) {
+        weak_ptr->BrokerConnected(
+            PlatformFileToInt(base::kInvalidPlatformFileValue),
+            PP_ERROR_FAILED);
+      }
     }
   }
   pending_connects_.clear();
@@ -543,8 +586,12 @@ PepperPluginDelegateImpl::CreatePepperPlugin(
   return module;
 }
 
-scoped_refptr<webkit::ppapi::PluginDelegate::PpapiBroker>
-PepperPluginDelegateImpl::CreatePpapiBroker(
+// If the IPC response never comes and OnPpapiBrokerChannelCreated is not called
+// the broker will not be deleted even if there are no more clients because
+// pending_connect_broker_ will still hold a reference.
+// TODO(ddorwin): Consider fixing this by cleaning up pending_connect_broker_
+// on the last Disconnect.
+scoped_refptr<PpapiBrokerImpl> PepperPluginDelegateImpl::CreatePpapiBroker(
     webkit::ppapi::PluginModule* plugin_module) {
   DCHECK(plugin_module);
   DCHECK(!plugin_module->GetBroker());
@@ -552,8 +599,7 @@ PepperPluginDelegateImpl::CreatePpapiBroker(
   // The broker path is the same as the plugin.
   const FilePath& broker_path = plugin_module->path();
 
-  scoped_refptr<PpapiBrokerImpl> broker = new PpapiBrokerImpl;
-  plugin_module->SetBroker(broker);
+  scoped_refptr<PpapiBrokerImpl> broker = new PpapiBrokerImpl(plugin_module);
 
   int request_id =
       pending_connect_broker_.Add(new scoped_refptr<PpapiBrokerImpl>(broker));
@@ -566,7 +612,7 @@ PepperPluginDelegateImpl::CreatePpapiBroker(
                                                broker_path);
   if (!render_view_->Send(msg)) {
     pending_connect_broker_.Remove(request_id);
-    return scoped_refptr<webkit::ppapi::PluginDelegate::PpapiBroker>();
+    return scoped_refptr<PpapiBrokerImpl>();
   }
 
   return broker;
@@ -758,15 +804,21 @@ PepperPluginDelegateImpl::ConnectToPpapiBroker(
     webkit::ppapi::PPB_Broker_Impl* client) {
   CHECK(client);
 
+  // If a broker needs to be created, this will ensure it does not get deleted
+  // before Connect() adds a reference.
+  scoped_refptr<PpapiBrokerImpl> broker_impl;
+
   webkit::ppapi::PluginModule* plugin_module = client->instance()->module();
-  scoped_refptr<webkit::ppapi::PluginDelegate::PpapiBroker> broker =
+  PpapiBroker* broker =
       plugin_module->GetBroker();
   if (!broker) {
-    broker = CreatePpapiBroker(plugin_module);
-    if (!broker)
+    broker_impl = CreatePpapiBroker(plugin_module);
+    if (!broker_impl.get())
       return NULL;
+    broker = broker_impl;
   }
 
+  // Adds a reference, ensuring not deleted when broker_impl goes out of scope.
   broker->Connect(client);
   return broker;
 }
diff --git a/content/renderer/pepper_plugin_delegate_impl.h b/content/renderer/pepper_plugin_delegate_impl.h
index 50f0824..ddce25d 100644
--- a/content/renderer/pepper_plugin_delegate_impl.h
+++ b/content/renderer/pepper_plugin_delegate_impl.h
@@ -7,8 +7,8 @@
 #pragma once
 
 #include <set>
+#include <map>
 #include <string>
-#include <vector>
 
 #include "base/basictypes.h"
 #include "base/id_map.h"
@@ -51,6 +51,7 @@ struct CustomContextMenuContext;
 
 class TransportDIB;
 
+// This object is NOT thread-safe.
 class BrokerDispatcherWrapper {
  public:
   BrokerDispatcherWrapper();
@@ -66,9 +67,11 @@ class BrokerDispatcherWrapper {
   scoped_ptr<pp::proxy::BrokerDispatcher> dispatcher_;
 };
 
-class PpapiBrokerImpl : public webkit::ppapi::PluginDelegate::PpapiBroker {
+// This object is NOT thread-safe.
+class PpapiBrokerImpl : public webkit::ppapi::PluginDelegate::PpapiBroker,
+                        public base::RefCountedThreadSafe<PpapiBrokerImpl>{
  public:
-  PpapiBrokerImpl();
+  explicit PpapiBrokerImpl(webkit::ppapi::PluginModule* plugin_module);
 
   // PpapiBroker implementation.
   virtual void Connect(webkit::ppapi::PPB_Broker_Impl* client);
@@ -86,11 +89,20 @@ class PpapiBrokerImpl : public webkit::ppapi::PluginDelegate::PpapiBroker {
                              base::SyncSocket::Handle handle);
 
  protected:
+  friend class base::RefCountedThreadSafe<PpapiBrokerImpl>;
   virtual ~PpapiBrokerImpl();
   scoped_ptr<BrokerDispatcherWrapper> dispatcher_;
 
-  std::vector<scoped_refptr<webkit::ppapi::PPB_Broker_Impl> > pending_connects_;
-  std::vector<scoped_refptr<webkit::ppapi::PPB_Broker_Impl> > pending_pipes_;
+  // A map of pointers to objects that have requested a connection to the weak
+  // pointer we can use to reference them. The mapping is needed so we can clean
+  // up entries for objects that may have been deleted.
+  typedef std::map<webkit::ppapi::PPB_Broker_Impl*,
+                   base::WeakPtr<webkit::ppapi::PPB_Broker_Impl> > ClientMap;
+  ClientMap pending_connects_;
+
+  // Pointer to the associated plugin module.
+  // Always set and cleared at the same time as the module's pointer to this.
+  webkit::ppapi::PluginModule* plugin_module_;
 
   DISALLOW_COPY_AND_ASSIGN(PpapiBrokerImpl);
 };
@@ -256,7 +268,7 @@ class PepperPluginDelegateImpl
 
  private:
   // Asynchronously attempts to create a PPAPI broker for the given plugin.
-  scoped_refptr<webkit::ppapi::PluginDelegate::PpapiBroker> CreatePpapiBroker(
+  scoped_refptr<PpapiBrokerImpl> CreatePpapiBroker(
       webkit::ppapi::PluginModule* plugin_module);
 
   // Pointer to the RenderView that owns us.
diff --git a/webkit/plugins/ppapi/plugin_delegate.h b/webkit/plugins/ppapi/plugin_delegate.h
index 761a808..3cb1fea 100644
--- a/webkit/plugins/ppapi/plugin_delegate.h
+++ b/webkit/plugins/ppapi/plugin_delegate.h
@@ -212,17 +212,17 @@ class PluginDelegate {
   };
 
   // Provides access to the ppapi broker.
-  class PpapiBroker : public base::RefCountedThreadSafe<PpapiBroker> {
+  class PpapiBroker {
    public:
     virtual void Connect(webkit::ppapi::PPB_Broker_Impl* client) = 0;
 
     // Decrements the references to the broker.
     // When there are no more references, this renderer's dispatcher is
     // destroyed, allowing the broker to shutdown if appropriate.
+    // Callers should not reference this object after calling Disconnect.
     virtual void Disconnect(webkit::ppapi::PPB_Broker_Impl* client) = 0;
 
    protected:
-    friend class base::RefCountedThreadSafe<PpapiBroker>;
     virtual ~PpapiBroker() {}
   };
 
diff --git a/webkit/plugins/ppapi/plugin_module.cc b/webkit/plugins/ppapi/plugin_module.cc
index a7a7dd2..2cae785 100644
--- a/webkit/plugins/ppapi/plugin_module.cc
+++ b/webkit/plugins/ppapi/plugin_module.cc
@@ -538,13 +538,12 @@ bool PluginModule::ReserveInstanceID(PP_Instance instance) {
   return true;  // Instance ID is usable.
 }
 
-void PluginModule::SetBroker(
-    scoped_refptr<PluginDelegate::PpapiBroker> broker) {
-  DCHECK(!broker_.get());
+void PluginModule::SetBroker(PluginDelegate::PpapiBroker* broker) {
+  DCHECK(!broker_ || !broker);
   broker_ = broker;
 }
 
-scoped_refptr<PluginDelegate::PpapiBroker> PluginModule::GetBroker(){
+PluginDelegate::PpapiBroker* PluginModule::GetBroker(){
   return broker_;
 }
 
diff --git a/webkit/plugins/ppapi/plugin_module.h b/webkit/plugins/ppapi/plugin_module.h
index e462f3a..1ade5a6 100644
--- a/webkit/plugins/ppapi/plugin_module.h
+++ b/webkit/plugins/ppapi/plugin_module.h
@@ -145,8 +145,9 @@ class PluginModule : public base::RefCounted<PluginModule>,
       PP_Bool (*reserve)(PP_Module, PP_Instance));
   bool ReserveInstanceID(PP_Instance instance);
 
-  void SetBroker(scoped_refptr<PluginDelegate::PpapiBroker> broker);
-  scoped_refptr<PluginDelegate::PpapiBroker> GetBroker();
+  // These should only be called from the main thread.
+  void SetBroker(PluginDelegate::PpapiBroker* broker);
+  PluginDelegate::PpapiBroker* GetBroker();
 
  private:
   // Calls the InitializeModule entrypoint. The entrypoint must have been
@@ -170,8 +171,9 @@ class PluginModule : public base::RefCounted<PluginModule>,
   // entry_points_ aren't valid.
   scoped_ptr<PluginDelegate::OutOfProcessProxy> out_of_process_proxy_;
 
-  // Trusted broker for this plugin module.
-  scoped_refptr<PluginDelegate::PpapiBroker> broker_;
+  // Non-owning pointer to the broker for this plugin module, if one exists.
+  // It is populated and cleared in the main thread.
+  PluginDelegate::PpapiBroker* broker_;
 
   // Holds a reference to the base::NativeLibrary handle if this PluginModule
   // instance wraps functions loaded from a library.  Can be NULL.  If
diff --git a/webkit/plugins/ppapi/ppb_broker_impl.cc b/webkit/plugins/ppapi/ppb_broker_impl.cc
index 16aa316..d188ac7 100644
--- a/webkit/plugins/ppapi/ppb_broker_impl.cc
+++ b/webkit/plugins/ppapi/ppb_broker_impl.cc
@@ -106,15 +106,24 @@ int32_t PPB_Broker_Impl::Connect(
     return PP_ERROR_FAILED;
   }
 
-  broker_ = plugin_delegate->ConnectToPpapiBroker(this);
-  if (!broker_)
-    return PP_ERROR_FAILED;
-
+  // The callback must be populated now in case we are connected to the broker
+  // and BrokerConnected is called before ConnectToPpapiBroker returns.
+  // Because it must be created now, it must be aborted and cleared if
+  // ConnectToPpapiBroker fails.
   PP_Resource resource_id = GetReferenceNoAddRef();
   CHECK(resource_id);
   connect_callback_ = new TrackedCompletionCallback(
       instance()->module()->GetCallbackTracker(), resource_id,
       connect_callback);
+
+  broker_ = plugin_delegate->ConnectToPpapiBroker(this);
+  if (!broker_) {
+    scoped_refptr<TrackedCompletionCallback> callback;
+    callback.swap(connect_callback_);
+    callback->Abort();
+    return PP_ERROR_FAILED;
+  }
+
   return PP_OK_COMPLETIONPENDING;
 }
 
diff --git a/webkit/plugins/ppapi/ppb_broker_impl.h b/webkit/plugins/ppapi/ppb_broker_impl.h
index 5bba19a..111fba9 100644
--- a/webkit/plugins/ppapi/ppb_broker_impl.h
+++ b/webkit/plugins/ppapi/ppb_broker_impl.h
@@ -6,6 +6,7 @@
 #define WEBKIT_PLUGINS_PPAPI_PPB_BROKER_IMPL_H_
 
 #include "base/basictypes.h"
+#include "base/memory/weak_ptr.h"
 #include "ppapi/c/pp_completion_callback.h"
 #include "ppapi/c/trusted/ppb_broker_trusted.h"
 #include "webkit/plugins/ppapi/plugin_delegate.h"
@@ -18,7 +19,8 @@ namespace ppapi {
 
 class PluginInstance;
 
-class PPB_Broker_Impl : public Resource {
+class PPB_Broker_Impl : public Resource,
+                        public base::SupportsWeakPtr<PPB_Broker_Impl> {
  public:
   explicit PPB_Broker_Impl(PluginInstance* instance);
   virtual ~PPB_Broker_Impl();
@@ -37,8 +39,8 @@ class PPB_Broker_Impl : public Resource {
 
  private:
   // PluginDelegate ppapi broker object.
-  // We don't own this pointer but are responsible for calling Release on it.
-  scoped_refptr<PluginDelegate::PpapiBroker> broker_;
+  // We don't own this pointer but are responsible for calling Disconnect on it.
+  PluginDelegate::PpapiBroker* broker_;
 
   // Callback invoked from BrokerConnected.
   scoped_refptr<TrackedCompletionCallback> connect_callback_;