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

5 files changed, 29 insertions, 17 deletions

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_;