Move BrowserPluginDelegate's lifetime mgmt out of content/

This CL makes BrowserPlugin not own BPDelegate (GuestViewContainer)
  anymore.
BrowserPluginDelegate's lifetime has been tied to garbage collection
  of the shadow root HTML element that contains the plugin. For
  example, for <webview>, this is the <webview> element.
Before registering BPDelegate to GC, there's a brief period while
  BPDelegate is not owned by anyone directly, this case is covered
  with the static PostTask that deletes the delegate if we did not
  tie it to GC (UpdateInternalInstanceIdAndBindDelegateToGC()).

To make <webview> run without BrowserPlugin, we need to manage
  GuestViewContainer's (which is a BPDelegate) lifetime out of
  content/.

BUG=330264
Test=None, internal change only.

Review URL: https://codereview.chromium.org/1162053003

Cr-Commit-Position: refs/heads/master@{#333552}

components/guest_view/renderer/guest_view_container.cc

@@ -49,12 +49,43 @@ void GuestViewContainer::RenderFrameLifetimeObserver::OnDestruct() {
 GuestViewContainer::GuestViewContainer(content::RenderFrame* render_frame)
     : element_instance_id_(guest_view::kInstanceIDNone),
       render_frame_(render_frame),
-      ready_(false) {
+      ready_(false),
+      in_destruction_(false) {
   render_frame_lifetime_observer_.reset(
       new RenderFrameLifetimeObserver(this, render_frame_));
 }
 
 GuestViewContainer::~GuestViewContainer() {
+  // Note: Cleanups should be done in GuestViewContainer::Destroy(), not here.
+}
+
+// static.
+GuestViewContainer* GuestViewContainer::FromID(int element_instance_id) {
+  GuestViewContainerMap* guest_view_containers =
+      g_guest_view_container_map.Pointer();
+  auto it = guest_view_containers->find(element_instance_id);
+  return it == guest_view_containers->end() ? nullptr : it->second;
+}
+
+// Right now a GuestViewContainer can be destroyed in one of the following
+// ways:
+//
+// 1. If GuestViewContainer is driven by content/, the element (browser plugin)
+//   can destroy GuestViewContainer when the element is destroyed.
+// 2. If GuestViewContainer is managed outside of content/, then the
+//   <webview> element's GC will destroy it.
+// 3. If GuestViewContainer's embedder frame is destroyed, we'd also destroy
+//   GuestViewContainer.
+void GuestViewContainer::Destroy() {
+  if (in_destruction_)
+    return;
+
+  in_destruction_ = true;
+
+  // Give our derived class an opportunity to perform some cleanup prior to
+  // destruction.
+  OnDestroy();
+
   if (element_instance_id() != guest_view::kInstanceIDNone)
     g_guest_view_container_map.Get().erase(element_instance_id());
 
@@ -67,19 +98,14 @@ GuestViewContainer::~GuestViewContainer() {
     // Call the JavaScript callbacks with no arguments which implies an error.
     pending_request->ExecuteCallbackIfAvailable(0 /* argc */, nullptr);
   }
-}
 
-// static.
-GuestViewContainer* GuestViewContainer::FromID(int element_instance_id) {
-  GuestViewContainerMap* guest_view_containers =
-      g_guest_view_container_map.Pointer();
-  auto it = guest_view_containers->find(element_instance_id);
-  return it == guest_view_containers->end() ? nullptr : it->second;
+  delete this;
 }
 
 void GuestViewContainer::RenderFrameDestroyed() {
   OnRenderFrameDestroyed();
   render_frame_ = nullptr;
+  Destroy();
 }
 
 void GuestViewContainer::IssueRequest(linked_ptr<GuestViewRequest> request) {
@@ -146,4 +172,8 @@ void GuestViewContainer::SetElementInstanceID(int element_instance_id) {
       std::make_pair(element_instance_id, this));
 }
 
+void GuestViewContainer::DidDestroyElement() {
+  Destroy();
+}
+
 }  // namespace guest_view

components/guest_view/renderer/guest_view_container.h

@@ -17,7 +17,6 @@ class GuestViewRequest;
 class GuestViewContainer : public content::BrowserPluginDelegate {
  public:
   explicit GuestViewContainer(content::RenderFrame* render_frame);
-  ~GuestViewContainer() override;
 
   static GuestViewContainer* FromID(int element_instance_id);
 
@@ -33,6 +32,9 @@ class GuestViewContainer : public content::BrowserPluginDelegate {
   // container.
   bool OnMessageReceived(const IPC::Message& message);
 
+  // Destroys this GuestViewContainer after performing necessary cleanup.
+  void Destroy();
+
   // Called when the embedding RenderFrame is destroyed.
   virtual void OnRenderFrameDestroyed() {}
 
@@ -43,6 +45,14 @@ class GuestViewContainer : public content::BrowserPluginDelegate {
   // Called to perform actions when a GuestViewContainer gets a geometry.
   virtual void OnReady() {}
 
+  // Called to perform actions when a GuestViewContainer is about to be
+  // destroyed.
+  // Note that this should be called exactly once.
+  virtual void OnDestroy() {}
+
+ protected:
+  ~GuestViewContainer() override;
+
  private:
   class RenderFrameLifetimeObserver;
   friend class RenderFrameLifetimeObserver;
@@ -58,12 +68,14 @@ class GuestViewContainer : public content::BrowserPluginDelegate {
   // BrowserPluginDelegate implementation.
   void Ready() final;
   void SetElementInstanceID(int element_instance_id) final;
+  void DidDestroyElement() final;
 
   int element_instance_id_;
   content::RenderFrame* render_frame_;
   scoped_ptr<RenderFrameLifetimeObserver> render_frame_lifetime_observer_;
 
   bool ready_;
+  bool in_destruction_;
 
   std::deque<linked_ptr<GuestViewRequest> > pending_requests_;
   linked_ptr<GuestViewRequest> pending_response_;

content/public/renderer/browser_plugin_delegate.h

@@ -5,8 +5,6 @@
 #ifndef CONTENT_PUBLIC_RENDERER_BROWSER_PLUGIN_DELEGATE_H_
 #define CONTENT_PUBLIC_RENDERER_BROWSER_PLUGIN_DELEGATE_H_
 
-#include <string>
-
 #include "content/common/content_export.h"
 
 namespace gfx {
@@ -28,8 +26,6 @@ class RenderFrame;
 // behavior of the plugin.
 class CONTENT_EXPORT BrowserPluginDelegate {
  public:
-  virtual ~BrowserPluginDelegate() {}
-
   // Called when the BrowserPlugin's geometry has been computed for the first
   // time.
   virtual void Ready() {}
@@ -47,8 +43,14 @@ class CONTENT_EXPORT BrowserPluginDelegate {
   // Called when the plugin resizes.
   virtual void DidResizeElement(const gfx::Size& new_size) {}
 
+  // Called when the plugin is about to be destroyed.
+  virtual void DidDestroyElement() {}
+
   // Return a scriptable object for the plugin.
   virtual v8::Local<v8::Object> V8ScriptableObject(v8::Isolate* isolate);
+
+ protected:
+  virtual ~BrowserPluginDelegate() {}
 };
 
 }  // namespace content

content/renderer/browser_plugin/browser_plugin.cc

@@ -61,7 +61,7 @@ BrowserPlugin* BrowserPlugin::GetFromNode(blink::WebNode& node) {
 }
 
 BrowserPlugin::BrowserPlugin(RenderFrame* render_frame,
-                             scoped_ptr<BrowserPluginDelegate> delegate)
+                             BrowserPluginDelegate* delegate)
     : attached_(false),
       render_frame_routing_id_(render_frame->GetRoutingID()),
       container_(nullptr),
@@ -73,7 +73,7 @@ BrowserPlugin::BrowserPlugin(RenderFrame* render_frame,
       ready_(false),
       browser_plugin_instance_id_(browser_plugin::kInstanceIDNone),
       contents_opaque_(true),
-      delegate_(delegate.Pass()),
+      delegate_(delegate),
       weak_ptr_factory_(this) {
   browser_plugin_instance_id_ =
       BrowserPluginManager::Get()->GetNextInstanceID();
@@ -86,6 +86,10 @@ BrowserPlugin::~BrowserPlugin() {
   if (compositing_helper_.get())
     compositing_helper_->OnContainerDestroy();
 
+  if (delegate_)
+    delegate_->DidDestroyElement();
+  delegate_ = nullptr;
+
   BrowserPluginManager::Get()->RemoveBrowserPlugin(browser_plugin_instance_id_);
 }
 
@@ -263,6 +267,16 @@ void BrowserPlugin::ShowSadGraphic() {
     container_->invalidate();
 }
 
+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::UpdateGuestFocusState(blink::WebFocusType focus_type) {
   if (!attached())
     return;
@@ -329,16 +343,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
@@ -379,8 +383,7 @@ bool BrowserPlugin::canProcessDrag() const {
 void BrowserPlugin::paint(WebCanvas* canvas, const WebRect& rect) {
   if (guest_crashed_) {
     if (!sad_guest_)  // Lazily initialize bitmap.
-      sad_guest_ = content::GetContentClient()->renderer()->
-          GetSadWebViewBitmap();
+      sad_guest_ = GetContentClient()->renderer()->GetSadWebViewBitmap();
     // content_shell does not have the sad plugin bitmap, so we'll paint black
     // instead to make it clear that something went wrong.
     if (sad_guest_) {

content/renderer/browser_plugin/browser_plugin.h

@@ -55,8 +55,6 @@ class CONTENT_EXPORT BrowserPlugin :
   // A request to enable hardware compositing.
   void EnableCompositing(bool enable);
 
-  void UpdateInternalInstanceId();
-
   // Provided that a guest instance ID has been allocated, this method attaches
   // this BrowserPlugin instance to that guest.
   void Attach();
@@ -146,13 +144,14 @@ class CONTENT_EXPORT BrowserPlugin :
   // uniquely identifies a guest WebContents that's hosted by this
   // BrowserPlugin.
   BrowserPlugin(RenderFrame* render_frame,
-                scoped_ptr<BrowserPluginDelegate> delegate);
+                BrowserPluginDelegate* delegate);
 
   ~BrowserPlugin() override;
 
   gfx::Rect view_rect() const { return view_rect_; }
 
   void ShowSadGraphic();
+  void UpdateInternalInstanceId();
 
   // IPC message handlers.
   // Please keep in alphabetical order.
@@ -201,7 +200,9 @@ class CONTENT_EXPORT BrowserPlugin :
 
   std::vector<EditCommand> edit_commands_;
 
-  scoped_ptr<BrowserPluginDelegate> delegate_;
+  // We call lifetime managing methods on |delegate_|, but we do not directly
+  // own this. The delegate destroys itself.
+  BrowserPluginDelegate* delegate_;
 
   // Weak factory used in v8 |MakeWeak| callback, since the v8 callback might
   // get called after BrowserPlugin has been destroyed.

content/renderer/browser_plugin/browser_plugin_manager.cc

@@ -68,8 +68,8 @@ void BrowserPluginManager::Detach(int browser_plugin_instance_id) {
 
 BrowserPlugin* BrowserPluginManager::CreateBrowserPlugin(
     RenderFrame* render_frame,
-    scoped_ptr<BrowserPluginDelegate> delegate) {
-  return new BrowserPlugin(render_frame, delegate.Pass());
+    BrowserPluginDelegate* delegate) {
+  return new BrowserPlugin(render_frame, delegate);
 }
 
 void BrowserPluginManager::DidCommitCompositorFrame(

content/renderer/browser_plugin/browser_plugin_manager.h

@@ -34,9 +34,11 @@ class CONTENT_EXPORT BrowserPluginManager : public RenderProcessObserver {
   // BrowserPlugin is responsible for associating itself with the
   // BrowserPluginManager via AddBrowserPlugin. When it is destroyed, it is
   // responsible for removing its association via RemoveBrowserPlugin.
-  BrowserPlugin* CreateBrowserPlugin(
-      RenderFrame* render_frame,
-      scoped_ptr<BrowserPluginDelegate> delegate);
+  // The |delegate| is expected to manage its own lifetime.
+  // Generally BrowserPlugin calls DidDestroyElement() on the delegate and
+  // right now the delegate destroys itself once it hears that callback.
+  BrowserPlugin* CreateBrowserPlugin(RenderFrame* render_frame,
+                                     BrowserPluginDelegate* delegate);
 
   void Attach(int browser_plugin_instance_id);
 

content/renderer/render_frame_impl.cc

@@ -1799,11 +1799,10 @@ blink::WebPlugin* RenderFrameImpl::CreatePlugin(
   DCHECK_EQ(frame_, frame);
 #if defined(ENABLE_PLUGINS)
   if (info.type == WebPluginInfo::PLUGIN_TYPE_BROWSER_PLUGIN) {
-    scoped_ptr<BrowserPluginDelegate> browser_plugin_delegate(
+    return BrowserPluginManager::Get()->CreateBrowserPlugin(
+        this,
         GetContentClient()->renderer()->CreateBrowserPluginDelegate(
             this, params.mimeType.utf8(), GURL(params.url)));
-    return BrowserPluginManager::Get()->CreateBrowserPlugin(
-        this, browser_plugin_delegate.Pass());
   }
 
   bool pepper_plugin_was_registered = false;
@@ -1919,11 +1918,10 @@ blink::WebPlugin* RenderFrameImpl::createPlugin(
   }
 
   if (base::UTF16ToUTF8(params.mimeType) == kBrowserPluginMimeType) {
-    scoped_ptr<BrowserPluginDelegate> browser_plugin_delegate(
+    return BrowserPluginManager::Get()->CreateBrowserPlugin(
+        this,
         GetContentClient()->renderer()->CreateBrowserPluginDelegate(this,
             kBrowserPluginMimeType, GURL(params.url)));
-    return BrowserPluginManager::Get()->CreateBrowserPlugin(
-        this, browser_plugin_delegate.Pass());
   }
 
 #if defined(ENABLE_PLUGINS)

extensions/renderer/guest_view/extensions_guest_view_container.cc

@@ -19,6 +19,9 @@ ExtensionsGuestViewContainer::ExtensionsGuestViewContainer(
 }
 
 ExtensionsGuestViewContainer::~ExtensionsGuestViewContainer() {
+}
+
+void ExtensionsGuestViewContainer::OnDestroy() {
   // Call the destruction callback, if one is registered.
   if (!destruction_callback_.IsEmpty()) {
     v8::HandleScope handle_scope(destruction_isolate_);

extensions/renderer/guest_view/extensions_guest_view_container.h

@@ -19,7 +19,6 @@ namespace extensions {
 class ExtensionsGuestViewContainer : public guest_view::GuestViewContainer {
  public:
   explicit ExtensionsGuestViewContainer(content::RenderFrame* render_frame);
-  ~ExtensionsGuestViewContainer() override;
 
   void RegisterDestructionCallback(v8::Local<v8::Function> callback,
                                    v8::Isolate* isolate);
@@ -29,9 +28,15 @@ class ExtensionsGuestViewContainer : public guest_view::GuestViewContainer {
   // BrowserPluginDelegate implementation.
   void DidResizeElement(const gfx::Size& new_size) override;
 
+ protected:
+  ~ExtensionsGuestViewContainer() override;
+
  private:
   void CallElementResizeCallback(const gfx::Size& new_size);
 
+  // GuestViewContainer implementation.
+  void OnDestroy() override;
+
   v8::Global<v8::Function> destruction_callback_;
   v8::Isolate* destruction_isolate_;
 

extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc

@@ -47,6 +47,9 @@ GuestViewInternalCustomBindings::GuestViewInternalCustomBindings(
   RouteFunction("DetachGuest",
                 base::Bind(&GuestViewInternalCustomBindings::DetachGuest,
                            base::Unretained(this)));
+  RouteFunction("DestroyContainer",
+                base::Bind(&GuestViewInternalCustomBindings::DestroyContainer,
+                           base::Unretained(this)));
   RouteFunction("GetContentWindow",
                 base::Bind(&GuestViewInternalCustomBindings::GetContentWindow,
                            base::Unretained(this)));
@@ -174,6 +177,30 @@ void GuestViewInternalCustomBindings::DetachGuest(
   args.GetReturnValue().Set(v8::Boolean::New(context()->isolate(), true));
 }
 
+void GuestViewInternalCustomBindings::DestroyContainer(
+    const v8::FunctionCallbackInfo<v8::Value>& args) {
+  args.GetReturnValue().SetNull();
+
+  if (args.Length() != 1)
+    return;
+
+  // Element Instance ID.
+  if (!args[0]->IsInt32())
+    return;
+
+  int element_instance_id = args[0]->Int32Value();
+  auto* guest_view_container =
+      guest_view::GuestViewContainer::FromID(element_instance_id);
+  if (!guest_view_container)
+    return;
+
+  // Note: |guest_view_container| is deleted.
+  // GuestViewContainer::DidDestroyElement() currently also destroys
+  // a GuestViewContainer. That won't be necessary once GuestViewContainer
+  // always runs w/o plugin.
+  guest_view_container->Destroy();
+}
+
 void GuestViewInternalCustomBindings::GetContentWindow(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   // Default to returning null.