Move BrowserPluginDelegate's lifetime mgmt out of content/

content/renderer/browser_plugin/browser_plugin.cc

Index: content/renderer/browser_plugin/browser_plugin.cc
diff --git a/content/renderer/browser_plugin/browser_plugin.cc b/content/renderer/browser_plugin/browser_plugin.cc
index 6d0c17d56d720780021ec63911ec4499cdfb9cd7..8c371e9a895df03efc45f10d7de463ebc06d3a9a 100644
--- a/content/renderer/browser_plugin/browser_plugin.cc
+++ b/content/renderer/browser_plugin/browser_plugin.cc
@@ -45,6 +45,29 @@ using PluginContainerMap =
     std::map<blink::WebPluginContainer*, content::BrowserPlugin*>;
 static base::LazyInstance<PluginContainerMap> g_plugin_container_map =
     LAZY_INSTANCE_INITIALIZER;
+
+void UpdateInternalInstanceIdAndBindDelegateToGC(
+    int browser_plugin_instance_id,
+    const base::WeakPtr<content::BrowserPlugin>& plugin,
+    content::BrowserPluginDelegate* delegate) {
+  // If the plugin is gone, then delete the delegate to avoid leaking it.
+  if (!plugin.get()) {
+    delegate->Destroy();

[Fady Samuel, 2015/06/05 17:31:13]

Since the delegate manages its own lifetime, why not simply call out to the
delegate to tell it the plugin (element) is gone? It would be nice to move all
lifetime management code to one place to make it easier to reason about.

[lazyboy, 2015/06/05 21:24:00]

As discussed offline, WillDestroy didn't use to destroy
the container, hence the explicit Destroy().

Now I've changed WillDestroy (named DidDestroyElement) to destroy
the container too, so this one is addressed.

+    return;
+  }
+
+  // Updating "internalinstanceid" will bind the |delegate|'s lifetime to
+  // <webview> element's garbage collection.
+  //
+  // This is a way to notify observers of our attributes that this plugin is
+  // available in render tree.
+  // TODO(lazyboy): This should be done through the delegate instead. Perhaps
+  // by firing an event from there.
+  plugin->UpdateDOMAttribute(
+      "internalinstanceid",
+      base::UTF8ToUTF16(base::IntToString(browser_plugin_instance_id)));
+}
+
 }  // namespace
 
 namespace content {
@@ -61,7 +84,8 @@ BrowserPlugin* BrowserPlugin::GetFromNode(blink::WebNode& node) {
 }
 
 BrowserPlugin::BrowserPlugin(RenderFrame* render_frame,
-                             scoped_ptr<BrowserPluginDelegate> delegate)
+                             const std::string& mime_type,
+                             const GURL& original_url)
     : attached_(false),
       render_frame_routing_id_(render_frame->GetRoutingID()),
       container_(nullptr),
@@ -73,19 +97,22 @@ BrowserPlugin::BrowserPlugin(RenderFrame* render_frame,
       ready_(false),
       browser_plugin_instance_id_(browser_plugin::kInstanceIDNone),
       contents_opaque_(true),
-      delegate_(delegate.Pass()),
+      delegate_(nullptr),

[Fady Samuel, 2015/06/05 17:31:13]

I would suggest restoring this and making the delegate a raw pointer.

[lazyboy, 2015/06/05 21:24:00]

Done.

+      mime_type_(mime_type),
+      original_url_(original_url),
       weak_ptr_factory_(this) {
   browser_plugin_instance_id_ =
       BrowserPluginManager::Get()->GetNextInstanceID();
-
-  if (delegate_)
-    delegate_->SetElementInstanceID(browser_plugin_instance_id_);
 }
 
 BrowserPlugin::~BrowserPlugin() {
   if (compositing_helper_.get())
     compositing_helper_->OnContainerDestroy();
 
+  if (delegate_)
+    delegate_->WillDestroy();
+  delegate_ = nullptr;
+
   BrowserPluginManager::Get()->RemoveBrowserPlugin(browser_plugin_instance_id_);
 }
 
@@ -299,11 +326,18 @@ bool BrowserPlugin::initialize(WebPluginContainer* container) {
   BrowserPluginManager::Get()->AddBrowserPlugin(
       browser_plugin_instance_id_, this);
 
+  delegate_ =
+      content::GetContentClient()->renderer()->CreateBrowserPluginDelegate(

[Fady Samuel, 2015/06/05 17:31:13]

I still feel like this should happen outside of BrowserPlugin.

[lazyboy, 2015/06/05 21:24:00]

Done.
I was worried that BrowserPlugin might be deleted before it got a call
to initialize() in some cases. Looking at FrameLoaderClientImpl::createPlugin()
it seems that might not be possible.

+          RenderFrameImpl::FromRoutingID(render_frame_routing_id()), mime_type_,
+          original_url_);
+  delegate_->SetElementInstanceID(browser_plugin_instance_id_);
+
   // Defer attach call so that if there's any pending browser plugin
   // destruction, then it can progress first.
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(&BrowserPlugin::UpdateInternalInstanceId,
-                            weak_ptr_factory_.GetWeakPtr()));
+      FROM_HERE, base::Bind(&UpdateInternalInstanceIdAndBindDelegateToGC,
+                            browser_plugin_instance_id_,
+                            weak_ptr_factory_.GetWeakPtr(), delegate_));
   return true;
 }
 
@@ -329,16 +363,6 @@ void BrowserPlugin::EnableCompositing(bool enable) {
   }
 }
 
-void BrowserPlugin::UpdateInternalInstanceId() {
-  // This is a way to notify observers of our attributes that this plugin is
-  // available in render tree.
-  // TODO(lazyboy): This should be done through the delegate instead. Perhaps
-  // by firing an event from there.
-  UpdateDOMAttribute(
-      "internalinstanceid",
-      base::UTF8ToUTF16(base::IntToString(browser_plugin_instance_id_)));
-}
-
 void BrowserPlugin::destroy() {
   if (container_) {
     // The BrowserPlugin's WebPluginContainer is deleted immediately after this