Merge 73319 - Release all Object PP_Vars when an instance goes away. This prevents a crash

when the instance is deleted but the reference to the var is kept in the
ResourceTracker. When the ResourceTracker singleton is cleared on shutdown,
the resource will attempt to dereference the invalid PluginInstance and crash.

Properly delete the PluginObjects associated with an instance when the instance
goes out of scope. A simple error in the PluginInstance made this not happen,
and it could potentially dereference PluginObjects for instances that were
deleted.

BUG=http://crosbug.com/11438
BUG=http://crbug.com/71357
Review URL: http://codereview.chromium.org/6312038

TBR=brettw@chromium.org
Review URL: http://codereview.chromium.org/6286104

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

7 files changed, 226 insertions, 90 deletions

diff --git a/webkit/plugins/ppapi/ppapi_plugin_instance.cc b/webkit/plugins/ppapi/ppapi_plugin_instance.cc
index 474c2dd..7ab4713 100644
--- a/webkit/plugins/ppapi/ppapi_plugin_instance.cc
+++ b/webkit/plugins/ppapi/ppapi_plugin_instance.cc
@@ -327,8 +327,8 @@ PluginInstance::~PluginInstance() {
   // unregister themselves inside the delete call).
   PluginObjectSet plugin_object_copy;
   live_plugin_objects_.swap(plugin_object_copy);
-  for (PluginObjectSet::iterator i = live_plugin_objects_.begin();
-       i != live_plugin_objects_.end(); ++i)
+  for (PluginObjectSet::iterator i = plugin_object_copy.begin();
+       i != plugin_object_copy.end(); ++i)
     delete *i;
 
   delegate_->InstanceDeleted(this);
diff --git a/webkit/plugins/ppapi/ppapi_unittest.cc b/webkit/plugins/ppapi/ppapi_unittest.cc
index bc3039d..7ad58c3 100644
--- a/webkit/plugins/ppapi/ppapi_unittest.cc
+++ b/webkit/plugins/ppapi/ppapi_unittest.cc
@@ -100,6 +100,8 @@ void PpapiUnittest::SetUp() {
 }
 
 void PpapiUnittest::TearDown() {
+  instance_ = NULL;
+  module_ = NULL;
 }
 
 const void* PpapiUnittest::GetMockInterface(const char* interface_name) const {
diff --git a/webkit/plugins/ppapi/resource_tracker.cc b/webkit/plugins/ppapi/resource_tracker.cc
index 6507e2d..4874c79 100644
--- a/webkit/plugins/ppapi/resource_tracker.cc
+++ b/webkit/plugins/ppapi/resource_tracker.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -11,6 +11,7 @@
 #include "base/logging.h"
 #include "base/rand_util.h"
 #include "ppapi/c/pp_resource.h"
+#include "ppapi/c/pp_var.h"
 #include "webkit/plugins/ppapi/ppapi_plugin_instance.h"
 #include "webkit/plugins/ppapi/resource.h"
 #include "webkit/plugins/ppapi/var.h"
@@ -82,18 +83,31 @@ PP_Resource ResourceTracker::AddResource(Resource* resource) {
   // Add the resource with plugin use-count 1.
   PP_Resource new_id = MakeTypedId(++last_resource_id_, PP_ID_TYPE_RESOURCE);
   live_resources_.insert(std::make_pair(new_id, std::make_pair(resource, 1)));
-  instance_to_resources_[resource->instance()->pp_instance()].insert(new_id);
+
+  // Track associated with the instance.
+  PP_Instance pp_instance = resource->instance()->pp_instance();
+  DCHECK(instance_map_.find(pp_instance) != instance_map_.end());
+  instance_map_[pp_instance].resources.insert(new_id);
   return new_id;
 }
 
 int32 ResourceTracker::AddVar(Var* var) {
   // If the plugin manages to create 1B strings...
-  if (last_var_id_ == std::numeric_limits<int32>::max() >> kPPIdTypeBits) {
+  if (last_var_id_ == std::numeric_limits<int32>::max() >> kPPIdTypeBits)
     return 0;
-  }
+
   // Add the resource with plugin use-count 1.
   int32 new_id = MakeTypedId(++last_var_id_, PP_ID_TYPE_VAR);
   live_vars_.insert(std::make_pair(new_id, std::make_pair(var, 1)));
+
+  // Object vars must be associated with the instance.
+  ObjectVar* object_var = var->AsObjectVar();
+  if (object_var) {
+    PP_Instance instance = object_var->instance()->pp_instance();
+    DCHECK(instance_map_.find(instance) != instance_map_.end());
+    instance_map_[instance].object_vars.insert(new_id);
+  }
+
   return new_id;
 }
 
@@ -119,13 +133,12 @@ bool ResourceTracker::UnrefResource(PP_Resource res) {
   if (i != live_resources_.end()) {
     if (!--i->second.second) {
       Resource* to_release = i->second.first;
+      // LastPluginRefWasDeleted will clear the instance pointer, so save it
+      // first.
+      PP_Instance instance = to_release->instance()->pp_instance();
       to_release->LastPluginRefWasDeleted(false);
 
-      ResourceSet& instance_resource_set =
-          instance_to_resources_[to_release->instance()->pp_instance()];
-      DCHECK(instance_resource_set.find(res) != instance_resource_set.end());
-      instance_resource_set.erase(res);
-
+      instance_map_[instance].resources.erase(res);
       live_resources_.erase(i);
     }
     return true;
@@ -134,43 +147,21 @@ bool ResourceTracker::UnrefResource(PP_Resource res) {
   }
 }
 
-void ResourceTracker::ForceDeletePluginResourceRefs(PP_Resource res) {
-  DLOG_IF(ERROR, !CheckIdType(res, PP_ID_TYPE_RESOURCE))
-      << res << " is not a PP_Resource.";
-  ResourceMap::iterator i = live_resources_.find(res);
-  if (i == live_resources_.end())
-    return;  // Nothing to do.
-
-  i->second.second = 0;
-  Resource* resource = i->second.first;
-
-  // Must delete from the resource set first since the resource's instance
-  // pointer will get zeroed out in LastPluginRefWasDeleted.
-  ResourceSet& resource_set = instance_to_resources_[
-      resource->instance()->pp_instance()];
-  DCHECK(resource_set.find(res) != resource_set.end());
-  resource_set.erase(res);
-
-  resource->LastPluginRefWasDeleted(true);
-  live_resources_.erase(i);
-}
-
 uint32 ResourceTracker::GetLiveObjectsForInstance(
     PP_Instance instance) const {
-  InstanceToResourceMap::const_iterator found =
-      instance_to_resources_.find(instance);
-  if (found == instance_to_resources_.end())
+  InstanceMap::const_iterator found = instance_map_.find(instance);
+  if (found == instance_map_.end())
     return 0;
-  return static_cast<uint32>(found->second.size());
+  return static_cast<uint32>(found->second.resources.size() +
+                             found->second.object_vars.size());
 }
 
 scoped_refptr<Var> ResourceTracker::GetVar(int32 var_id) const {
   DLOG_IF(ERROR, !CheckIdType(var_id, PP_ID_TYPE_VAR))
       << var_id << " is not a PP_Var ID.";
   VarMap::const_iterator result = live_vars_.find(var_id);
-  if (result == live_vars_.end()) {
+  if (result == live_vars_.end())
     return scoped_refptr<Var>();
-  }
   return result->second.first;
 }
 
@@ -193,26 +184,24 @@ bool ResourceTracker::UnrefVar(int32 var_id) {
       << var_id << " is not a PP_Var ID.";
   VarMap::iterator i = live_vars_.find(var_id);
   if (i != live_vars_.end()) {
-    if (!--i->second.second)
+    if (!--i->second.second) {
+      ObjectVar* object_var = i->second.first->AsObjectVar();
+      if (object_var) {
+        instance_map_[object_var->instance()->pp_instance()].object_vars.erase(
+            var_id);
+      }
       live_vars_.erase(i);
+    }
     return true;
   }
   return false;
 }
 
 PP_Instance ResourceTracker::AddInstance(PluginInstance* instance) {
-#ifndef NDEBUG
-  // Make sure we're not adding one more than once.
-  for (InstanceMap::const_iterator i = instance_map_.begin();
-       i != instance_map_.end(); ++i)
-    DCHECK(i->second != instance);
-#endif
+  DCHECK(instance_map_.find(instance->pp_instance()) == instance_map_.end());
 
-  // Use a random 64-bit number for the instance ID. This helps prevent some
-  // mischeif where you could misallocate resources if you gave a different
-  // instance ID.
-  //
-  // See also AddModule below.
+  // Use a random number for the instance ID. This helps prevent some
+  // accidents. See also AddModule below.
   //
   // Need to make sure the random number isn't a duplicate or 0.
   PP_Instance new_instance;
@@ -221,32 +210,64 @@ PP_Instance ResourceTracker::AddInstance(PluginInstance* instance) {
                                PP_ID_TYPE_INSTANCE);
   } while (!new_instance ||
            instance_map_.find(new_instance) != instance_map_.end());
-  instance_map_[new_instance] = instance;
+
+  instance_map_[new_instance].instance = instance;
   return new_instance;
 }
 
 void ResourceTracker::InstanceDeleted(PP_Instance instance) {
   DLOG_IF(ERROR, !CheckIdType(instance, PP_ID_TYPE_INSTANCE))
       << instance << " is not a PP_Instance.";
+  InstanceMap::iterator found = instance_map_.find(instance);
+  if (found == instance_map_.end()) {
+    NOTREACHED();
+    return;
+  }
+  InstanceData& data = found->second;
+
   // Force release all plugin references to resources associated with the
   // deleted instance.
-  ResourceSet& resource_set = instance_to_resources_[instance];
-  ResourceSet::iterator i = resource_set.begin();
-  while (i != resource_set.end()) {
+  ResourceSet::iterator cur_res = data.resources.begin();
+  while (cur_res != data.resources.end()) {
+    ResourceMap::iterator found_resource = live_resources_.find(*cur_res);
+    if (found_resource == live_resources_.end()) {
+      NOTREACHED();
+    } else {
+      Resource* resource = found_resource->second.first;
+
+      // Must delete from the resource set first since the resource's instance
+      // pointer will get zeroed out in LastPluginRefWasDeleted.
+      resource->LastPluginRefWasDeleted(true);
+      live_resources_.erase(*cur_res);
+    }
+
     // Iterators to a set are stable so we can iterate the set while the items
     // are being deleted as long as we're careful not to delete the item we're
     // holding an iterator to.
-    ResourceSet::iterator current = i++;
-    ForceDeletePluginResourceRefs(*current);
+    ResourceSet::iterator current = cur_res++;
+    data.resources.erase(current);
   }
-  DCHECK(resource_set.empty());
-  instance_to_resources_.erase(instance);
-
-  InstanceMap::iterator found = instance_map_.find(instance);
-  if (found == instance_map_.end()) {
-    NOTREACHED();
-    return;
+  DCHECK(data.resources.empty());
+
+  // Force delete all var references.
+  VarSet::iterator cur_var = data.object_vars.begin();
+  while (cur_var != data.object_vars.end()) {
+    VarSet::iterator current = cur_var++;
+
+    // Tell the corresponding ObjectVar that the instance is gone.
+    PP_Var object_pp_var;
+    object_pp_var.type = PP_VARTYPE_OBJECT;
+    object_pp_var.value.as_id = *current;
+    scoped_refptr<ObjectVar> object_var(ObjectVar::FromPPVar(object_pp_var));
+    if (object_var.get())
+      object_var->InstanceDeleted();
+
+    // Clear the object from the var mapping and the live instance object list.
+    live_vars_.erase(*current);
+    data.object_vars.erase(*current);
   }
+  DCHECK(data.object_vars.empty());
+
   instance_map_.erase(found);
 }
 
@@ -256,7 +277,7 @@ PluginInstance* ResourceTracker::GetInstance(PP_Instance instance) {
   InstanceMap::iterator found = instance_map_.find(instance);
   if (found == instance_map_.end())
     return NULL;
-  return found->second;
+  return found->second.instance;
 }
 
 PP_Module ResourceTracker::AddModule(PluginModule* module) {
diff --git a/webkit/plugins/ppapi/resource_tracker.h b/webkit/plugins/ppapi/resource_tracker.h
index ce577c3..32a4c7b 100644
--- a/webkit/plugins/ppapi/resource_tracker.h
+++ b/webkit/plugins/ppapi/resource_tracker.h
@@ -53,15 +53,6 @@ class ResourceTracker {
   bool AddRefResource(PP_Resource res);
   bool UnrefResource(PP_Resource res);
 
-  // Forces the plugin refcount of the given resource to 0. This is used when
-  // the instance is destroyed and we want to free all resources.
-  //
-  // Note that this may not necessarily delete the resource object since the
-  // regular refcount is maintained separately from the plugin refcount and
-  // random components in the Pepper implementation could still have
-  // references to it.
-  void ForceDeletePluginResourceRefs(PP_Resource res);
-
   // Returns the number of resources associated with this module.
   uint32 GetLiveObjectsForInstance(PP_Instance instance) const;
 
@@ -106,6 +97,24 @@ class ResourceTracker {
   friend class ResourceTrackerTest;
   friend class Var;
 
+  typedef std::set<PP_Resource> ResourceSet;
+
+  // Indexed by the var ID.
+  typedef std::set<int32> VarSet;
+
+  // Per-instance data we track.
+  struct InstanceData {
+    InstanceData() : instance(0) {}
+
+    // Non-owning pointer to the instance object. When a PluginInstance is
+    // destroyed, it will notify us and we'll delete all associated data.
+    PluginInstance* instance;
+
+    // Resources and object vars associated with the instance.
+    ResourceSet resources;
+    VarSet object_vars;
+  };
+
   // Prohibit creation other then by the Singleton class.
   ResourceTracker();
   ~ResourceTracker();
@@ -146,15 +155,8 @@ class ResourceTracker {
   typedef base::hash_map<int32, VarAndRefCount> VarMap;
   VarMap live_vars_;
 
-  // Tracks all resources associated with each instance. This is used to
-  // delete resources when the instance has been destroyed to avoid leaks.
-  typedef std::set<PP_Resource> ResourceSet;
-  typedef std::map<PP_Instance, ResourceSet> InstanceToResourceMap;
-  InstanceToResourceMap instance_to_resources_;
-
-  // Tracks all live instances. The pointers are non-owning, the PluginInstance
-  // destructor will notify us when the instance is deleted.
-  typedef std::map<PP_Instance, PluginInstance*> InstanceMap;
+  // Tracks all live instances and their associated data.
+  typedef std::map<PP_Instance, InstanceData> InstanceMap;
   InstanceMap instance_map_;
 
   // Tracks all live modules. The pointers are non-owning, the PluginModule
diff --git a/webkit/plugins/ppapi/resource_tracker_unittest.cc b/webkit/plugins/ppapi/resource_tracker_unittest.cc
index 6e7b2a4..ead6b57 100644
--- a/webkit/plugins/ppapi/resource_tracker_unittest.cc
+++ b/webkit/plugins/ppapi/resource_tracker_unittest.cc
@@ -1,20 +1,26 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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 "webkit/plugins/ppapi/ppapi_unittest.h"
 
+#include "base/scoped_ptr.h"
+#include "ppapi/c/pp_var.h"
 #include "ppapi/c/ppp_instance.h"
+#include "third_party/npapi/bindings/npruntime.h"
 #include "webkit/plugins/ppapi/mock_plugin_delegate.h"
 #include "webkit/plugins/ppapi/mock_resource.h"
 #include "webkit/plugins/ppapi/ppapi_plugin_instance.h"
 #include "webkit/plugins/ppapi/resource_tracker.h"
+#include "webkit/plugins/ppapi/var.h"
 
 namespace webkit {
 namespace ppapi {
 
 namespace {
 
+// Tracked Resources -----------------------------------------------------------
+
 class TrackedMockResource : public MockResource {
  public:
   static int tracked_objects_alive;
@@ -29,20 +35,59 @@ class TrackedMockResource : public MockResource {
 
 int TrackedMockResource::tracked_objects_alive = 0;
 
+// Tracked NPObjects -----------------------------------------------------------
+
+int g_npobjects_alive = 0;
+
+void TrackedClassDeallocate(NPObject* npobject) {
+  g_npobjects_alive--;
+}
+
+NPClass g_tracked_npclass = {
+  NP_CLASS_STRUCT_VERSION,
+  NULL,
+  &TrackedClassDeallocate,
+  NULL,
+  NULL,
+  NULL,
+  NULL,
+  NULL,
+  NULL,
+  NULL,
+  NULL,
+  NULL,
+};
+
+// Returns a new tracked NPObject with a refcount of 1.
+NPObject* NewTrackedNPObject() {
+  NPObject* object = new NPObject;
+  object->_class = &g_tracked_npclass;
+  object->referenceCount = 1;
+
+  g_npobjects_alive++;
+  return object;
+}
+
 }  // namespace
 
+// ResourceTrackerTest ---------------------------------------------------------
+
 class ResourceTrackerTest : public PpapiUnittest {
  public:
   ResourceTrackerTest() {
   }
 
   virtual void SetUp() {
-    PpapiUnittest::SetUp();
+    // The singleton override must be installed before the generic setup because
+    // that creates an instance, etc. which uses the tracker.
     ResourceTracker::SetSingletonOverride(&tracker_);
+    PpapiUnittest::SetUp();
   }
   virtual void TearDown() {
-    ResourceTracker::ClearSingletonOverride();
+    // Must do normal tear down before clearing the override for the same rason
+    // as the SetUp.
     PpapiUnittest::TearDown();
+    ResourceTracker::ClearSingletonOverride();
   }
 
   ResourceTracker& tracker() { return tracker_; }
@@ -93,7 +138,7 @@ TEST_F(ResourceTrackerTest, Ref) {
   ASSERT_EQ(0, TrackedMockResource::tracked_objects_alive);
 }
 
-TEST_F(ResourceTrackerTest, ForceDeleteWithInstance) {
+TEST_F(ResourceTrackerTest, DeleteResourceWithInstance) {
   // Make a second instance (the test harness already creates & manages one).
   scoped_refptr<PluginInstance> instance2(
       new PluginInstance(delegate(), module(),
@@ -128,5 +173,58 @@ TEST_F(ResourceTrackerTest, ForceDeleteWithInstance) {
   ASSERT_EQ(0, TrackedMockResource::tracked_objects_alive);
 }
 
+TEST_F(ResourceTrackerTest, DeleteObjectVarWithInstance) {
+  // Make a second instance (the test harness already creates & manages one).
+  scoped_refptr<PluginInstance> instance2(
+      new PluginInstance(delegate(), module(),
+                         static_cast<const PPP_Instance*>(
+                             GetMockInterface(PPP_INSTANCE_INTERFACE))));
+  PP_Instance pp_instance2 = instance2->pp_instance();
+
+  // Make an object var.
+  scoped_ptr<NPObject> npobject(NewTrackedNPObject());
+  PP_Var pp_object = ObjectVar::NPObjectToPPVar(instance2.get(),
+                                                npobject.get());
+
+  EXPECT_EQ(1, g_npobjects_alive);
+  EXPECT_EQ(1u, tracker().GetLiveObjectsForInstance(pp_instance2));
+
+  // Free the instance, this should release the ObjectVar.
+  instance2 = NULL;
+  EXPECT_EQ(0u, tracker().GetLiveObjectsForInstance(pp_instance2));
+}
+
+// Make sure that using the same NPObject should give the same PP_Var
+// each time.
+TEST_F(ResourceTrackerTest, ReuseVar) {
+  scoped_ptr<NPObject> npobject(NewTrackedNPObject());
+
+  PP_Var pp_object1 = ObjectVar::NPObjectToPPVar(instance(), npobject.get());
+  PP_Var pp_object2 = ObjectVar::NPObjectToPPVar(instance(), npobject.get());
+
+  // The two results should be the same.
+  EXPECT_EQ(pp_object1.value.as_id, pp_object2.value.as_id);
+
+  // The objects should be able to get us back to the associated NPObject.
+  // This ObjectVar must be released before we do NPObjectToPPVar again
+  // below so it gets freed and we get a new identifier.
+  {
+    scoped_refptr<ObjectVar> check_object(ObjectVar::FromPPVar(pp_object1));
+    ASSERT_TRUE(check_object.get());
+    EXPECT_EQ(instance(), check_object->instance());
+    EXPECT_EQ(npobject.get(), check_object->np_object());
+  }
+
+  // Remove both of the refs we made above.
+  tracker().UnrefVar(static_cast<int32_t>(pp_object2.value.as_id));
+  tracker().UnrefVar(static_cast<int32_t>(pp_object1.value.as_id));
+
+  // Releasing the resource should free the internal ref, and so making a new
+  // one now should generate a new ID.
+  PP_Var pp_object3 = ObjectVar::NPObjectToPPVar(instance(), npobject.get());
+  EXPECT_NE(pp_object1.value.as_id, pp_object3.value.as_id);
+  tracker().UnrefVar(static_cast<int32_t>(pp_object3.value.as_id));
+}
+
 }  // namespace ppapi
 }  // namespace webkit
diff --git a/webkit/plugins/ppapi/var.cc b/webkit/plugins/ppapi/var.cc
index f5cae25..b8071ed 100644
--- a/webkit/plugins/ppapi/var.cc
+++ b/webkit/plugins/ppapi/var.cc
@@ -825,7 +825,8 @@ ObjectVar::ObjectVar(PluginInstance* instance, NPObject* np_object)
 }
 
 ObjectVar::~ObjectVar() {
-  instance_->RemoveNPObjectVar(this);
+  if (instance_)
+    instance_->RemoveNPObjectVar(this);
   WebBindings::releaseObject(np_object_);
 }
 
@@ -833,8 +834,14 @@ ObjectVar* ObjectVar::AsObjectVar() {
   return this;
 }
 
+void ObjectVar::InstanceDeleted() {
+  DCHECK(instance_);
+  instance_ = NULL;
+}
+
 // static
 PP_Var ObjectVar::NPObjectToPPVar(PluginInstance* instance, NPObject* object) {
+  DCHECK(object);
   scoped_refptr<ObjectVar> object_var(instance->ObjectVarForNPObject(object));
   if (!object_var)  // No object for this module yet, make a new one.
     object_var = new ObjectVar(instance, object);
@@ -842,7 +849,7 @@ PP_Var ObjectVar::NPObjectToPPVar(PluginInstance* instance, NPObject* object) {
   if (!object_var)
     return PP_MakeUndefined();
 
-  // Convert to a PP_Var, GetReference will AddRef for us.
+  // Convert to a PP_Var, GetID will AddRef for us.
   PP_Var result;
   result.type = PP_VARTYPE_OBJECT;
   result.value.as_id = object_var->GetID();
diff --git a/webkit/plugins/ppapi/var.h b/webkit/plugins/ppapi/var.h
index ef3613f..4571698 100644
--- a/webkit/plugins/ppapi/var.h
+++ b/webkit/plugins/ppapi/var.h
@@ -179,6 +179,13 @@ class ObjectVar : public Var {
   // Guaranteed non-NULL.
   NPObject* np_object() const { return np_object_; }
 
+  // Notification that the instance was deleted, the internal pointer will be
+  // NULLed out.
+  void InstanceDeleted();
+
+  // Possibly NULL if the object has outlived its instance.
+  PluginInstance* instance() const { return instance_; }
+
   // Helper function to create a PP_Var of type object that contains the given
   // NPObject for use byt he given module. Calling this function multiple times
   // given the same module + NPObject results in the same PP_Var, assuming that
@@ -196,8 +203,6 @@ class ObjectVar : public Var {
   // if the PP_Var is not of object type or the object is invalid.
   static scoped_refptr<ObjectVar> FromPPVar(PP_Var var);
 
-  PluginInstance* instance() const { return instance_; }
-
  protected:
   // You should always use FromNPObject to create an ObjectVar. This function
   // guarantees that we maintain the 1:1 mapping between NPObject and
@@ -205,6 +210,7 @@ class ObjectVar : public Var {
   ObjectVar(PluginInstance* instance, NPObject* np_object);
 
  private:
+  // Possibly NULL if the object has outlived its instance.
   PluginInstance* instance_;
 
   // Guaranteed non-NULL, this is the underlying object used by WebKit. We