Move BrowserPluginDelegate's lifetime mgmt out of content/

content/renderer/browser_plugin/browser_plugin.cc

 // Copyright 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.
 
 #include "content/renderer/browser_plugin/browser_plugin.h"
 
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 27 matching lines @@
 using blink::WebPoint;
 using blink::WebRect;
 using blink::WebURL;
 using blink::WebVector;
 
 namespace {
 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()) {
+    delete delegate;
+    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 {
 
 // static
 BrowserPlugin* BrowserPlugin::GetFromNode(blink::WebNode& node) {
   blink::WebPluginContainer* container = node.pluginContainer();
   if (!container)
     return nullptr;
 
   PluginContainerMap* browser_plugins = g_plugin_container_map.Pointer();
   PluginContainerMap::iterator it = browser_plugins->find(container);
   return it == browser_plugins->end() ? nullptr : it->second;
 }
 
 BrowserPlugin::BrowserPlugin(RenderFrame* render_frame,
-                             scoped_ptr<BrowserPluginDelegate> delegate)

[Fady Samuel, 2015/06/05 15:10:41]

Why did you make this change?

[lazyboy, 2015/06/05 16:56:55]

We don't want to own BPDelegate by BrowserPlugin, so changed the
scoped_ptr to raw pointer, since BrowserPlugin is only going to
use BPDelegate, I moved the construction of delegate inside
this class.

+                             const std::string& mime_type,
+                             const GURL& original_url)
     : attached_(false),
       render_frame_routing_id_(render_frame->GetRoutingID()),
       container_(nullptr),
       sad_guest_(nullptr),
       guest_crashed_(false),
       plugin_focused_(false),
       visible_(true),
       mouse_locked_(false),
       ready_(false),
       browser_plugin_instance_id_(browser_plugin::kInstanceIDNone),
       contents_opaque_(true),
-      delegate_(delegate.Pass()),
+      delegate_(nullptr),
+      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_);
 }
 
 bool BrowserPlugin::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(BrowserPlugin, message)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_AdvanceFocus, OnAdvanceFocus)
     IPC_MESSAGE_HANDLER_GENERIC(BrowserPluginMsg_CompositorFrameSwapped,
                                 OnCompositorFrameSwapped(message))
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_GuestGone, OnGuestGone)
@@ skipping 193 matching lines @@
     return false;
 
   container_ = container;
   container_->setWantsWheelEvents(true);
 
   g_plugin_container_map.Get().insert(std::make_pair(container_, this));
 
   BrowserPluginManager::Get()->AddBrowserPlugin(
       browser_plugin_instance_id_, this);
 
+  delegate_ =
+      content::GetContentClient()->renderer()->CreateBrowserPluginDelegate(

[Fady Samuel, 2015/06/05 15:10:41]

Why move this code here?

[lazyboy, 2015/06/05 16:56:55]

See ^^^

+          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;
 }
 
 void BrowserPlugin::EnableCompositing(bool enable) {
   bool enabled = !!compositing_helper_.get();
   if (enabled == enable)
     return;
 
   if (enable) {
     DCHECK(!compositing_helper_.get());
     if (!compositing_helper_.get()) {
       compositing_helper_ = ChildFrameCompositingHelper::CreateForBrowserPlugin(
           weak_ptr_factory_.GetWeakPtr());
     }
   }
   compositing_helper_->EnableCompositing(enable);
   compositing_helper_->SetContentsOpaque(contents_opaque_);
 
   if (!enable) {
     DCHECK(compositing_helper_.get());
     compositing_helper_->OnContainerDestroy();
     compositing_helper_ = nullptr;
   }
 }
 
-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
     // call returns, so let's not keep a reference to it around.
     g_plugin_container_map.Get().erase(container_);
   }
 
   container_ = nullptr;
   // Will be a no-op if the mouse is not currently locked.
   auto render_frame = RenderFrameImpl::FromRoutingID(render_frame_routing_id());
@@ skipping 258 matching lines @@
 bool BrowserPlugin::HandleMouseLockedInputEvent(
     const blink::WebMouseEvent& event) {
   BrowserPluginManager::Get()->Send(
       new BrowserPluginHostMsg_HandleInputEvent(browser_plugin_instance_id_,
                                                 view_rect_,
                                                 &event));
   return true;
 }
 
 }  // namespace content