PPAPI: Really force-free NPObjects on Instance destruction.

webkit/plugins/ppapi/host_var_tracker.cc

 // 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/host_var_tracker.h"
 
 #include "base/logging.h"
 #include "ppapi/c/pp_var.h"
 #include "webkit/plugins/ppapi/host_array_buffer_var.h"
 #include "webkit/plugins/ppapi/npobject_var.h"
@@ skipping 80 matching lines @@
     return 0;
   return static_cast<int>(found->second->size());
 }
 
 void HostVarTracker::ForceFreeNPObjectsForInstance(PP_Instance instance) {
   InstanceMap::iterator found_instance = instance_map_.find(instance);
   if (found_instance == instance_map_.end())
     return;  // Nothing to do.
   NPObjectToNPObjectVarMap* np_object_map = found_instance->second.get();
 
-  // Force delete all var references. Need to make a copy so we can iterate over
-  // the map while deleting stuff from it.
-  NPObjectToNPObjectVarMap np_object_map_copy = *np_object_map;

[dmichael (off chromium), 2012/02/17 22:45:10]

I realized this copy really *is* necessary (now). Consider an in-process,
trusted plugin. When we force-release an object, its PPP_Class_Deallocate
function might force-release another object and thus delete it from our map
while we're iterating.

I fixed this case, added a test, and improved the existing comment.

-  NPObjectToNPObjectVarMap::iterator cur_var =
-      np_object_map_copy.begin();
-  while (cur_var != np_object_map_copy.end()) {
+  NPObjectToNPObjectVarMap::iterator cur_var = np_object_map->begin();
+  while (cur_var != np_object_map->end()) {
     NPObjectToNPObjectVarMap::iterator current = cur_var++;
     current->second->InstanceDeleted();
-    np_object_map->erase(current->first);
+    ForceReleaseNPObject(current->second);
+    np_object_map->erase(current);
   }
 
   // Remove the record for this instance since it should be empty.
   DCHECK(np_object_map->empty());

[dmichael (off chromium), 2012/02/17 22:45:10]

Note that as-is, PPP_Class_Deallocate could create a new object for our instance
and thus leave one or more objects in the map (violating this assertion). To
protect against that, we could disallow creating any more objects for this
instance while we're in the process of cleaning out the instance.

Should I be that paranoid, or should we assume (since we control the trusted
plugins) that we just won't do that?

   instance_map_.erase(found_instance);
 }
 
+void HostVarTracker::ForceReleaseNPObject(::ppapi::NPObjectVar* object) {
+  VarMap::iterator iter = live_vars_.find(object->GetExistingVarID());
+  if (iter == live_vars_.end()) {
+    NOTREACHED();
+    return;
+  }
+  iter->second.ref_count = 0;
+  DCHECK(iter->second.track_with_no_reference_count == 0);
+  DeleteObjectInfoIfNecessary(iter);
+}
+
 }  // namespace ppapi
 }  // namespace webkit