Make sure PluginResource doesn't live forever because of holding references to itself.

BUG=None
TEST=None


Review URL: https://chromiumcodereview.appspot.com/11387003

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

7 files changed, 64 insertions, 13 deletions

diff --git a/ppapi/proxy/plugin_resource.cc b/ppapi/proxy/plugin_resource.cc
index c568df5..22eba26 100644
--- a/ppapi/proxy/plugin_resource.cc
+++ b/ppapi/proxy/plugin_resource.cc
@@ -42,6 +42,30 @@ void PluginResource::OnReplyReceived(
   }
 }
 
+void PluginResource::NotifyLastPluginRefWasDeleted() {
+  Resource::NotifyLastPluginRefWasDeleted();
+
+  // The callbacks may hold referrences to this object. Normally, we will get
+  // reply messages from the host side and remove them. However, it is possible
+  // that some replies from the host never arrive, e.g., the corresponding
+  // renderer crashes. In that case, we have to clean up the callbacks,
+  // otherwise this object will live forever.
+  callbacks_.clear();
+}
+
+void PluginResource::NotifyInstanceWasDeleted() {
+  Resource::NotifyInstanceWasDeleted();
+
+  // Please see comments in NotifyLastPluginRefWasDeleted() about why we must
+  // clean up the callbacks.
+  // It is possible that NotifyLastPluginRefWasDeleted() is never called for a
+  // resource. For example, those singleton-style resources such as
+  // GamepadResource never expose references to the plugin and thus won't
+  // receive a NotifyLastPluginRefWasDeleted() call. For those resources, we
+  // need to clean up callbacks when the instance goes away.
+  callbacks_.clear();
+}
+
 void PluginResource::SendCreate(Destination dest, const IPC::Message& msg) {
   if (dest == RENDERER) {
     DCHECK(!sent_create_to_renderer_);
diff --git a/ppapi/proxy/plugin_resource.h b/ppapi/proxy/plugin_resource.h
index b763d1a..3b564b4 100644
--- a/ppapi/proxy/plugin_resource.h
+++ b/ppapi/proxy/plugin_resource.h
@@ -40,6 +40,14 @@ class PPAPI_PROXY_EXPORT PluginResource : public Resource {
   // and calling it with |params| and |msg|.
   virtual void OnReplyReceived(const proxy::ResourceMessageReplyParams& params,
                                const IPC::Message& msg) OVERRIDE;
+
+  // Resource overrides.
+  // Note: Subclasses shouldn't override these methods directly. Instead, they
+  // should implement LastPluginRefWasDeleted() or InstanceWasDeleted() to get
+  // notified.
+  virtual void NotifyLastPluginRefWasDeleted() OVERRIDE;
+  virtual void NotifyInstanceWasDeleted() OVERRIDE;
+
  protected:
   enum Destination {
     RENDERER = 0,
@@ -66,7 +74,7 @@ class PPAPI_PROXY_EXPORT PluginResource : public Resource {
   // Call<PpapiPluginMsg_MyResourceType_MyReplyMessage>(
   //     BROWSER,
   //     PpapiHostMsg_MyResourceType_MyRequestMessage(),
-  //     base::Bind(&MyPluginResource::ReplyHandler, this));
+  //     base::Bind(&MyPluginResource::ReplyHandler, base::Unretained(this)));
   //
   // If a reply message to this call is received whose type does not match
   // |ReplyMsgClass| (for example, in the case of an error), the callback will
@@ -75,7 +83,15 @@ class PPAPI_PROXY_EXPORT PluginResource : public Resource {
   // Returns the new request's sequence number which can be used to identify
   // the callback.
   //
-  // Note that all integers (including 0 and -1) are valid request IDs.
+  // Note: 1) all integers (including 0 and -1) are valid request IDs.
+  //       2) when all plugin references to this resource are gone or the
+  //          corresponding plugin instance is deleted, all pending callbacks
+  //          are abandoned.
+  //       3) it is *not* recommended to let |callback| hold any reference to
+  //          |this|, in which it will be stored. Otherwise, this object will
+  //          live forever if we fail to clean up the callback. It is safe to
+  //          use base::Unretained(this) or a weak pointer, because this object
+  //          will outlive the callback.
   template<typename ReplyMsgClass, typename CallbackType>
   int32_t Call(Destination dest,
                const IPC::Message& msg,
diff --git a/ppapi/shared_impl/resource.cc b/ppapi/shared_impl/resource.cc
index 5c9f738..88f908b 100644
--- a/ppapi/shared_impl/resource.cc
+++ b/ppapi/shared_impl/resource.cc
@@ -53,10 +53,15 @@ PP_Resource Resource::GetReference() {
   return pp_resource();
 }
 
-void Resource::LastPluginRefWasDeleted() {
+void Resource::NotifyLastPluginRefWasDeleted() {
+  // Notify subclasses.
+  LastPluginRefWasDeleted();
 }
 
-void Resource::InstanceWasDeleted() {
+void Resource::NotifyInstanceWasDeleted() {
+  // Notify subclasses.
+  InstanceWasDeleted();
+
   host_resource_ = HostResource();
 }
 
diff --git a/ppapi/shared_impl/resource.h b/ppapi/shared_impl/resource.h
index c8b7d1f..c1bf0cb 100644
--- a/ppapi/shared_impl/resource.h
+++ b/ppapi/shared_impl/resource.h
@@ -156,7 +156,10 @@ class PPAPI_SHARED_EXPORT Resource : public base::RefCounted<Resource> {
   // was released. For a few types of resources, the resource could still
   // stay alive if there are other references held by the PPAPI implementation
   // (possibly for callbacks and things).
-  virtual void LastPluginRefWasDeleted();
+  //
+  // Note that subclasses except PluginResource should override
+  // LastPluginRefWasDeleted() to be notified.
+  virtual void NotifyLastPluginRefWasDeleted();
 
   // Called by the resource tracker when the instance is going away but the
   // object is still alive (this is not the common case, since it requires
@@ -167,8 +170,9 @@ class PPAPI_SHARED_EXPORT Resource : public base::RefCounted<Resource> {
   // background processing (like maybe network loads) on behalf of the plugin
   // and you want to stop that when the plugin is deleted.
   //
-  // Be sure to call this version which clears the instance ID.
-  virtual void InstanceWasDeleted();
+  // Note that subclasses except PluginResource should override
+  // InstanceWasDeleted() to be notified.
+  virtual void NotifyInstanceWasDeleted();
 
   // Dynamic casting for this object. Returns the pointer to the given type if
   // it's supported. Derived classes override the functions they support to
@@ -198,6 +202,10 @@ class PPAPI_SHARED_EXPORT Resource : public base::RefCounted<Resource> {
   // Logs a message to the console from this resource.
   void Log(PP_LogLevel_Dev level, const std::string& message);
 
+  // Notifications for subclasses.
+  virtual void LastPluginRefWasDeleted() {}
+  virtual void InstanceWasDeleted() {}
+
  private:
   // See the getters above.
   PP_Resource pp_resource_;
diff --git a/ppapi/shared_impl/resource_tracker.cc b/ppapi/shared_impl/resource_tracker.cc
index da89774..45f127a 100644
--- a/ppapi/shared_impl/resource_tracker.cc
+++ b/ppapi/shared_impl/resource_tracker.cc
@@ -141,7 +141,7 @@ void ResourceTracker::DidDeleteInstance(PP_Instance instance) {
   while (cur != to_delete.end()) {
     ResourceMap::iterator found_resource = live_resources_.find(*cur);
     if (found_resource != live_resources_.end())
-      found_resource->second.first->InstanceWasDeleted();
+      found_resource->second.first->NotifyInstanceWasDeleted();
     cur++;
   }
 
@@ -216,7 +216,7 @@ void ResourceTracker::LastPluginRefWasDeleted(Resource* object) {
   CHECK(callback_tracker || is_message_loop);
   if (callback_tracker)
     callback_tracker->PostAbortForResource(object->pp_resource());
-  object->LastPluginRefWasDeleted();
+  object->NotifyLastPluginRefWasDeleted();
 }
 
 }  // namespace ppapi
diff --git a/ppapi/shared_impl/resource_tracker.h b/ppapi/shared_impl/resource_tracker.h
index 2f1c54e..f5f790c 100644
--- a/ppapi/shared_impl/resource_tracker.h
+++ b/ppapi/shared_impl/resource_tracker.h
@@ -66,8 +66,8 @@ class PPAPI_SHARED_EXPORT ResourceTracker {
   virtual void RemoveResource(Resource* object);
 
  private:
-  // Calls LastPluginRefWasDeleted on the given resource object and cancels
-  // pending callbacks for the resource.
+  // Calls NotifyLastPluginRefWasDeleted on the given resource object and
+  // cancels pending callbacks for the resource.
   void LastPluginRefWasDeleted(Resource* object);
 
   typedef std::set<PP_Resource> ResourceSet;
diff --git a/ppapi/shared_impl/resource_tracker_unittest.cc b/ppapi/shared_impl/resource_tracker_unittest.cc
index 26d02c3..fb00d76 100644
--- a/ppapi/shared_impl/resource_tracker_unittest.cc
+++ b/ppapi/shared_impl/resource_tracker_unittest.cc
@@ -27,11 +27,9 @@ class MyMockResource : public Resource {
   }
 
   virtual void LastPluginRefWasDeleted() OVERRIDE {
-    Resource::LastPluginRefWasDeleted();
     last_plugin_ref_was_deleted_count++;
   }
   virtual void InstanceWasDeleted() OVERRIDE {
-    Resource::InstanceWasDeleted();
     instance_was_deleted_count++;
   }
 };