Re-land http://codereview.chromium.org/9403039/, r124106

Original description:
"""
PPAPI: Really force-free NPObjects on Instance destruction.

(There still seems to be a memory leak with this patch; I may have to check our NPObject reference counting next.)

BUG=114023
TEST=
"""

BUG=114023
TEST=
TBR=dmichael@chromium.org

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

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

7 files changed, 58 insertions, 18 deletions

diff --git a/webkit/plugins/ppapi/host_var_tracker.cc b/webkit/plugins/ppapi/host_var_tracker.cc
index 51ba0e84..57bc92c 100644
--- a/webkit/plugins/ppapi/host_var_tracker.cc
+++ b/webkit/plugins/ppapi/host_var_tracker.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -42,7 +42,7 @@ void HostVarTracker::AddNPObjectVar(NPObjectVar* object_var) {
   DCHECK(np_object_map->find(object_var->np_object()) ==
          np_object_map->end()) << "NPObjectVar already in map";
   np_object_map->insert(
-      std::make_pair(object_var->np_object(), object_var));
+      std::make_pair(object_var->np_object(), object_var->AsWeakPtr()));
 }
 
 void HostVarTracker::RemoveNPObjectVar(NPObjectVar* object_var) {
@@ -98,14 +98,17 @@ void HostVarTracker::ForceFreeNPObjectsForInstance(PP_Instance instance) {
     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.
+  // Force delete all var references. It's possible that deleting an object "A"
+  // will cause it to delete another object "B" it references, thus removing "B"
+  // from instance_map_. Therefore, we need to make a copy over which we can
+  // iterate safely. Furthermore, the maps contain WeakPtrs so that we can
+  // detect if the object is gone so that we don't dereference invalid memory.
   NPObjectToNPObjectVarMap np_object_map_copy = *np_object_map;
   NPObjectToNPObjectVarMap::iterator cur_var =
       np_object_map_copy.begin();
   while (cur_var != np_object_map_copy.end()) {
     NPObjectToNPObjectVarMap::iterator current = cur_var++;
-    current->second->InstanceDeleted();
+    ForceReleaseNPObject(current->second);
     np_object_map->erase(current->first);
   }
 
@@ -114,5 +117,23 @@ void HostVarTracker::ForceFreeNPObjectsForInstance(PP_Instance instance) {
   instance_map_.erase(found_instance);
 }
 
+void HostVarTracker::ForceReleaseNPObject(
+    const base::WeakPtr< ::ppapi::NPObjectVar>& object) {
+  // There's a chance that the object was already deleted before we got here.
+  // See ForceFreeNPObjectsForInstance for further explanation. If the object
+  // was deleted, the WeakPtr will return NULL.
+  if (!object.get())
+    return;
+  object->InstanceDeleted();
+  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
diff --git a/webkit/plugins/ppapi/host_var_tracker.h b/webkit/plugins/ppapi/host_var_tracker.h
index fb743fc..ece0659 100644
--- a/webkit/plugins/ppapi/host_var_tracker.h
+++ b/webkit/plugins/ppapi/host_var_tracker.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -14,6 +14,7 @@
 #include "base/memory/linked_ptr.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "ppapi/c/pp_instance.h"
 #include "ppapi/c/pp_resource.h"
 #include "ppapi/shared_impl/function_group_base.h"
@@ -66,7 +67,12 @@ class HostVarTracker : public ::ppapi::VarTracker {
   virtual ::ppapi::ArrayBufferVar* CreateArrayBuffer(
       uint32 size_in_bytes) OVERRIDE;
 
-  typedef std::map<NPObject*, ::ppapi::NPObjectVar*> NPObjectToNPObjectVarMap;
+  // Clear the reference count of the given object and remove it from
+  // live_vars_.
+  void ForceReleaseNPObject(const base::WeakPtr< ::ppapi::NPObjectVar>& object);
+
+  typedef std::map<NPObject*, base::WeakPtr< ::ppapi::NPObjectVar> >
+      NPObjectToNPObjectVarMap;
 
   // Lists all known NPObjects, first indexed by the corresponding instance,
   // then by the NPObject*. This allows us to look up an NPObjectVar given
diff --git a/webkit/plugins/ppapi/message_channel.cc b/webkit/plugins/ppapi/message_channel.cc
index 3ef0597..db764e56 100644
--- a/webkit/plugins/ppapi/message_channel.cc
+++ b/webkit/plugins/ppapi/message_channel.cc
@@ -420,7 +420,8 @@ MessageChannel::~MessageChannel() {
 void MessageChannel::SetPassthroughObject(NPObject* passthrough) {
   // Retain the passthrough object; We need to ensure it lives as long as this
   // MessageChannel.
-  WebBindings::retainObject(passthrough);
+  if (passthrough)
+    WebBindings::retainObject(passthrough);
 
   // If we had a passthrough set already, release it. Note that we retain the
   // incoming passthrough object first, so that we behave correctly if anyone
diff --git a/webkit/plugins/ppapi/npobject_var.h b/webkit/plugins/ppapi/npobject_var.h
index b4e4269..1a187ac 100644
--- a/webkit/plugins/ppapi/npobject_var.h
+++ b/webkit/plugins/ppapi/npobject_var.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/compiler_specific.h"
+#include "base/memory/weak_ptr.h"
 #include "ppapi/c/pp_instance.h"
 #include "ppapi/shared_impl/var.h"
 #include "webkit/plugins/webkit_plugins_export.h"
@@ -27,7 +28,8 @@ namespace ppapi {
 // PP_Var IDs) for each module. This allows us to track all references owned by
 // a given module and free them when the plugin exits independently of other
 // plugins that may be running at the same time.
-class NPObjectVar : public Var {
+class NPObjectVar : public Var,
+                    public base::SupportsWeakPtr<NPObjectVar> {
  public:
   // You should always use FromNPObject to create an NPObjectVar. This function
   // guarantees that we maintain the 1:1 mapping between NPObject and
diff --git a/webkit/plugins/ppapi/ppapi_plugin_instance.cc b/webkit/plugins/ppapi/ppapi_plugin_instance.cc
index e4c13c4..3135a3c 100644
--- a/webkit/plugins/ppapi/ppapi_plugin_instance.cc
+++ b/webkit/plugins/ppapi/ppapi_plugin_instance.cc
@@ -358,6 +358,11 @@ PluginInstance::~PluginInstance() {
 void PluginInstance::Delete() {
   // Keep a reference on the stack. See NOTE above.
   scoped_refptr<PluginInstance> ref(this);
+  // Force the MessageChannel to release its "passthrough object" which should
+  // release our last reference to the "InstanceObject" and will probably
+  // destroy it. We want to do this prior to calling DidDestroy in case the
+  // destructor of the instance object tries to use the instance.
+  message_channel_->SetPassthroughObject(NULL);
   instance_interface_->DidDestroy(pp_instance());
 
   if (fullscreen_container_) {
diff --git a/webkit/plugins/ppapi/ppapi_webplugin_impl.cc b/webkit/plugins/ppapi/ppapi_webplugin_impl.cc
index c505741..fb01662 100644
--- a/webkit/plugins/ppapi/ppapi_webplugin_impl.cc
+++ b/webkit/plugins/ppapi/ppapi_webplugin_impl.cc
@@ -8,7 +8,8 @@
 
 #include "base/message_loop.h"
 #include "googleurl/src/gurl.h"
-#include "ppapi/c/pp_var.h"
+#include "ppapi/shared_impl/ppapi_globals.h"
+#include "ppapi/shared_impl/var_tracker.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebBindings.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebDocument.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebElement.h"
@@ -51,7 +52,8 @@ WebPluginImpl::WebPluginImpl(
     const WebPluginParams& params,
     const base::WeakPtr<PluginDelegate>& plugin_delegate)
     : init_data_(new InitData()),
-      full_frame_(params.loadManually) {
+      full_frame_(params.loadManually),
+      instance_object_(PP_MakeUndefined()) {
   DCHECK(plugin_module);
   init_data_->module = plugin_module;
   init_data_->delegate = plugin_delegate;
@@ -91,6 +93,8 @@ bool WebPluginImpl::initialize(WebPluginContainer* container) {
 
 void WebPluginImpl::destroy() {
   if (instance_) {
+    ::ppapi::PpapiGlobals::Get()->GetVarTracker()->ReleaseVar(instance_object_);
+    instance_object_ = PP_MakeUndefined();
     instance_->Delete();
     instance_ = NULL;
   }
@@ -99,17 +103,16 @@ void WebPluginImpl::destroy() {
 }
 
 NPObject* WebPluginImpl::scriptableObject() {
-  // Call through the plugin to get its instance object. Note that we "leak" a
-  // reference here. But we want to keep the instance object alive so long as
-  // the instance is alive, so it's okay. It will get cleaned up when all
-  // NPObjectVars are "force freed" at instance shutdown.
-  scoped_refptr<NPObjectVar> object(
-      NPObjectVar::FromPPVar(instance_->GetInstanceObject()));
+  // Call through the plugin to get its instance object. The plugin should pass
+  // us a reference which we release in destroy().
+  if (instance_object_.type == PP_VARTYPE_UNDEFINED)
+    instance_object_ = instance_->GetInstanceObject();
   // GetInstanceObject talked to the plugin which may have removed the instance
   // from the DOM, in which case instance_ would be NULL now.
   if (!instance_)
     return NULL;
 
+  scoped_refptr<NPObjectVar> object(NPObjectVar::FromPPVar(instance_object_));
   // If there's an InstanceObject, tell the Instance's MessageChannel to pass
   // any non-postMessage calls to it.
   if (object) {
diff --git a/webkit/plugins/ppapi/ppapi_webplugin_impl.h b/webkit/plugins/ppapi/ppapi_webplugin_impl.h
index e853281..ae319a8 100644
--- a/webkit/plugins/ppapi/ppapi_webplugin_impl.h
+++ b/webkit/plugins/ppapi/ppapi_webplugin_impl.h
@@ -11,6 +11,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop_helpers.h"
+#include "ppapi/c/pp_var.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebPlugin.h"
 #include "ui/gfx/rect.h"
 #include "webkit/plugins/webkit_plugins_export.h"
@@ -93,6 +94,7 @@ class WebPluginImpl : public WebKit::WebPlugin {
   scoped_refptr<PluginInstance> instance_;
   scoped_refptr<PPB_URLLoader_Impl> document_loader_;
   gfx::Rect plugin_rect_;
+  PP_Var instance_object_;
 
   DISALLOW_COPY_AND_ASSIGN(WebPluginImpl);
 };