Move BrowserPluginDelegate's lifetime mgmt out of content/

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.

Committed: https://crrev.com/cb6ba5c2ad69bf9daf896e5d3cd0c252c0ce6da3
Cr-Commit-Position: refs/heads/master@{#333552}

[lazyboy, 2015-06-05 14:38:04]

lazyboy@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2015-06-05 15:10:42]

https://chromiumcodereview.appspot.com/1162053003/diff/40001/content/renderer...
File content/renderer/browser_plugin/browser_plugin.cc (left):

https://chromiumcodereview.appspot.com/1162053003/diff/40001/content/renderer...
content/renderer/browser_plugin/browser_plugin.cc:64:
scoped_ptr<BrowserPluginDelegate> delegate)
Why did you make this change?

https://chromiumcodereview.appspot.com/1162053003/diff/40001/content/renderer...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/1162053003/diff/40001/content/renderer...
content/renderer/browser_plugin/browser_plugin.cc:330:
content::GetContentClient()->renderer()->CreateBrowserPluginDelegate(
Why move this code here?

https://chromiumcodereview.appspot.com/1162053003/diff/40001/extensions/rende...
File extensions/renderer/guest_view/extensions_guest_view_container.cc (right):

https://chromiumcodereview.appspot.com/1162053003/diff/40001/extensions/rende...
extensions/renderer/guest_view/extensions_guest_view_container.cc:22:
OnDestroy();
Calling virtual methods from a destructor is bad, please don't do this. It's bad
object oriented practice because a derived class may already have destroyed its
members.

https://chromiumcodereview.appspot.com/1162053003/diff/40001/extensions/rende...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://chromiumcodereview.appspot.com/1162053003/diff/40001/extensions/rende...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:197:
delete guest_view_container;
Let's make sure that GuestViewContainer destruction happens in one place. The
idiom is as follows:

1. Please make GuestViewContainer's destructor private and introduce a Destroy()
method.
2. Make sure that method cannot be called reentrantly by introducing a flag to
check if we're in destruction.
3. Call all virtual methods from there.
4. Clear the weak ptr factory.
5. delete this.

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

https://chromiumcodereview.appspot.com/1162053003/diff/40001/components/guest...
File components/guest_view/renderer/guest_view_container.h (right):

https://chromiumcodereview.appspot.com/1162053003/diff/40001/components/guest...
components/guest_view/renderer/guest_view_container.h:65: void WillDestroy()
final;
The chosen name here is ambiguous, the name should hint
that the "plugin" is about to be destroyed, not the container. Suggestions? I
didn't want to name it WillDestroyPlugin().

https://chromiumcodereview.appspot.com/1162053003/diff/40001/content/renderer...
File content/renderer/browser_plugin/browser_plugin.cc (left):

https://chromiumcodereview.appspot.com/1162053003/diff/40001/content/renderer...
content/renderer/browser_plugin/browser_plugin.cc:64:
scoped_ptr<BrowserPluginDelegate> delegate)
On 2015/06/05 15:10:41, Fady Samuel wrote:
> Why did you make this change?

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.

https://chromiumcodereview.appspot.com/1162053003/diff/40001/content/renderer...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/1162053003/diff/40001/content/renderer...
content/renderer/browser_plugin/browser_plugin.cc:330:
content::GetContentClient()->renderer()->CreateBrowserPluginDelegate(
On 2015/06/05 15:10:41, Fady Samuel wrote:
> Why move this code here?

See ^^^

https://chromiumcodereview.appspot.com/1162053003/diff/40001/extensions/rende...
File extensions/renderer/guest_view/extensions_guest_view_container.cc (right):

https://chromiumcodereview.appspot.com/1162053003/diff/40001/extensions/rende...
extensions/renderer/guest_view/extensions_guest_view_container.cc:22:
OnDestroy();
On 2015/06/05 15:10:41, Fady Samuel wrote:
> Calling virtual methods from a destructor is bad, please don't do this. It's
bad
> object oriented practice because a derived class may already have destroyed
its
> members.

Thanks for catching this.
I've added Destroy() to GuestViewContainer as you've suggested.

https://chromiumcodereview.appspot.com/1162053003/diff/40001/extensions/rende...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://chromiumcodereview.appspot.com/1162053003/diff/40001/extensions/rende...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:197:
delete guest_view_container;
On 2015/06/05 15:10:42, Fady Samuel wrote:
> Let's make sure that GuestViewContainer destruction happens in one place. The
> idiom is as follows:
> 
> 1. Please make GuestViewContainer's destructor private and introduce a
Destroy()
> method.
GuestViewContainer's destructor is now protected.
> 2. Make sure that method cannot be called reentrantly by introducing a flag to
> check if we're in destruction.
Added in_destruction_ bool
> 3. Call all virtual methods from there.
Done.
> 4. Clear the weak ptr factory.
There are no weak ptr factory for these containers, so no-op?
> 5. delete this.
Done.

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

https://codereview.chromium.org/1162053003/diff/60001/components/guest_view/r...
File components/guest_view/renderer/guest_view_container.cc (right):

https://codereview.chromium.org/1162053003/diff/60001/components/guest_view/r...
components/guest_view/renderer/guest_view_container.cc:84: 
set in_destruction_ to true here.

https://codereview.chromium.org/1162053003/diff/60001/content/renderer/browse...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/1162053003/diff/60001/content/renderer/browse...
content/renderer/browser_plugin/browser_plugin.cc:55: delegate->Destroy();
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.

https://codereview.chromium.org/1162053003/diff/60001/content/renderer/browse...
content/renderer/browser_plugin/browser_plugin.cc:100: delegate_(nullptr),
I would suggest restoring this and making the delegate a raw pointer.

https://codereview.chromium.org/1162053003/diff/60001/content/renderer/browse...
content/renderer/browser_plugin/browser_plugin.cc:330:
content::GetContentClient()->renderer()->CreateBrowserPluginDelegate(
I still feel like this should happen outside of BrowserPlugin.

https://codereview.chromium.org/1162053003/diff/60001/content/renderer/browse...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/1162053003/diff/60001/content/renderer/browse...
content/renderer/browser_plugin/browser_plugin.h:147: const std::string&
mime_type,
I still don't understand this change? Why not simply pass it in as a raw pointer
as in BrowserPluginGuest.

https://codereview.chromium.org/1162053003/diff/60001/extensions/renderer/gue...
File extensions/renderer/guest_view/extensions_guest_view_container.cc (right):

https://codereview.chromium.org/1162053003/diff/60001/extensions/renderer/gue...
extensions/renderer/guest_view/extensions_guest_view_container.cc:22:
OnDestroy();
This is still bad. Don't call this here.

https://codereview.chromium.org/1162053003/diff/60001/extensions/renderer/gue...
File extensions/renderer/guest_view/extensions_guest_view_container.h (right):

https://codereview.chromium.org/1162053003/diff/60001/extensions/renderer/gue...
extensions/renderer/guest_view/extensions_guest_view_container.h:37: void
OnDestroy() override;
What is this overriding. Please specify.

https://codereview.chromium.org/1162053003/diff/60001/extensions/renderer/gue...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://codereview.chromium.org/1162053003/diff/60001/extensions/renderer/gue...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:198: //
Note: |guest_view_container| is deleted.
Move this above?

[Fady Samuel, 2015-06-05 18:52:33]

https://codereview.chromium.org/1162053003/diff/80001/components/guest_view/r...
File components/guest_view/renderer/guest_view_container.cc (right):

https://codereview.chromium.org/1162053003/diff/80001/components/guest_view/r...
components/guest_view/renderer/guest_view_container.cc:59: if
(element_instance_id() != guest_view::kInstanceIDNone)
Move all work out of the destructor to avoid future usage of the destructor

https://codereview.chromium.org/1162053003/diff/80001/components/guest_view/r...
components/guest_view/renderer/guest_view_container.cc:86: OnDestroy();
Add a comment here that this gives the derived class an opportunity to perform
some cleanup prior to destruction.

https://codereview.chromium.org/1162053003/diff/80001/components/guest_view/r...
components/guest_view/renderer/guest_view_container.cc:93: render_frame_ =
nullptr;
Call Destroy() here too?

https://codereview.chromium.org/1162053003/diff/80001/components/guest_view/r...
components/guest_view/renderer/guest_view_container.cc:160: void
GuestViewContainer::WillDestroy() {
Call this maybe ElementDestroyed

https://codereview.chromium.org/1162053003/diff/80001/components/guest_view/r...
components/guest_view/renderer/guest_view_container.cc:161: OnDestroy();
Destroy()?

https://codereview.chromium.org/1162053003/diff/80001/content/public/renderer...
File content/public/renderer/browser_plugin_delegate.h (right):

https://codereview.chromium.org/1162053003/diff/80001/content/public/renderer...
content/public/renderer/browser_plugin_delegate.h:47: virtual void WillDestroy()
{}
Rename this to ElementDestroyed.

https://codereview.chromium.org/1162053003/diff/80001/content/public/renderer...
content/public/renderer/browser_plugin_delegate.h:52: virtual void Destroy() =
0;
Let's not expose this to content.

https://codereview.chromium.org/1162053003/diff/80001/extensions/renderer/gue...
File extensions/renderer/guest_view/extensions_guest_view_container.cc (right):

https://codereview.chromium.org/1162053003/diff/80001/extensions/renderer/gue...
extensions/renderer/guest_view/extensions_guest_view_container.cc:39:
destruction_callback_.Reset();
This shouldn't be necessary.

[lazyboy, 2015-06-05 21:24:01]

https://chromiumcodereview.appspot.com/1162053003/diff/60001/components/guest...
File components/guest_view/renderer/guest_view_container.cc (right):

https://chromiumcodereview.appspot.com/1162053003/diff/60001/components/guest...
components/guest_view/renderer/guest_view_container.cc:84: 
On 2015/06/05 17:31:12, Fady Samuel wrote:
> set in_destruction_ to true here.

Incorrect patch, sorry about that.
Done.

https://chromiumcodereview.appspot.com/1162053003/diff/60001/content/renderer...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/1162053003/diff/60001/content/renderer...
content/renderer/browser_plugin/browser_plugin.cc:55: delegate->Destroy();
On 2015/06/05 17:31:13, Fady Samuel wrote:
> 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.

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.

https://chromiumcodereview.appspot.com/1162053003/diff/60001/content/renderer...
content/renderer/browser_plugin/browser_plugin.cc:100: delegate_(nullptr),
On 2015/06/05 17:31:13, Fady Samuel wrote:
> I would suggest restoring this and making the delegate a raw pointer.

Done.

https://chromiumcodereview.appspot.com/1162053003/diff/60001/content/renderer...
content/renderer/browser_plugin/browser_plugin.cc:330:
content::GetContentClient()->renderer()->CreateBrowserPluginDelegate(
On 2015/06/05 17:31:13, Fady Samuel wrote:
> I still feel like this should happen outside of BrowserPlugin.

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.

https://chromiumcodereview.appspot.com/1162053003/diff/60001/content/renderer...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/1162053003/diff/60001/content/renderer...
content/renderer/browser_plugin/browser_plugin.h:147: const std::string&
mime_type,
On 2015/06/05 17:31:13, Fady Samuel wrote:
> I still don't understand this change? Why not simply pass it in as a raw
pointer
> as in BrowserPluginGuest.

Done.

https://chromiumcodereview.appspot.com/1162053003/diff/60001/extensions/rende...
File extensions/renderer/guest_view/extensions_guest_view_container.cc (right):

https://chromiumcodereview.appspot.com/1162053003/diff/60001/extensions/rende...
extensions/renderer/guest_view/extensions_guest_view_container.cc:22:
OnDestroy();
On 2015/06/05 17:31:13, Fady Samuel wrote:
> This is still bad. Don't call this here.

Bad patch upload.
Done.

https://chromiumcodereview.appspot.com/1162053003/diff/60001/extensions/rende...
File extensions/renderer/guest_view/extensions_guest_view_container.h (right):

https://chromiumcodereview.appspot.com/1162053003/diff/60001/extensions/rende...
extensions/renderer/guest_view/extensions_guest_view_container.h:37: void
OnDestroy() override;
On 2015/06/05 17:31:13, Fady Samuel wrote:
> What is this overriding. Please specify.

Done.

https://chromiumcodereview.appspot.com/1162053003/diff/60001/extensions/rende...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://chromiumcodereview.appspot.com/1162053003/diff/60001/extensions/rende...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:198: //
Note: |guest_view_container| is deleted.
On 2015/06/05 17:31:13, Fady Samuel wrote:
> Move this above?

Done.

https://chromiumcodereview.appspot.com/1162053003/diff/80001/components/guest...
File components/guest_view/renderer/guest_view_container.cc (right):

https://chromiumcodereview.appspot.com/1162053003/diff/80001/components/guest...
components/guest_view/renderer/guest_view_container.cc:59: if
(element_instance_id() != guest_view::kInstanceIDNone)
On 2015/06/05 18:52:33, Fady Samuel wrote:
> Move all work out of the destructor to avoid future usage of the destructor

Done.

https://chromiumcodereview.appspot.com/1162053003/diff/80001/components/guest...
components/guest_view/renderer/guest_view_container.cc:86: OnDestroy();
On 2015/06/05 18:52:33, Fady Samuel wrote:
> Add a comment here that this gives the derived class an opportunity to perform
> some cleanup prior to destruction.

Done.

https://chromiumcodereview.appspot.com/1162053003/diff/80001/components/guest...
components/guest_view/renderer/guest_view_container.cc:93: render_frame_ =
nullptr;
On 2015/06/05 18:52:32, Fady Samuel wrote:
> Call Destroy() here too?

Done.

https://chromiumcodereview.appspot.com/1162053003/diff/80001/components/guest...
components/guest_view/renderer/guest_view_container.cc:160: void
GuestViewContainer::WillDestroy() {
On 2015/06/05 18:52:33, Fady Samuel wrote:
> Call this maybe ElementDestroyed

Done.

https://chromiumcodereview.appspot.com/1162053003/diff/80001/components/guest...
components/guest_view/renderer/guest_view_container.cc:161: OnDestroy();
On 2015/06/05 18:52:32, Fady Samuel wrote:
> Destroy()?

Done.

https://chromiumcodereview.appspot.com/1162053003/diff/80001/content/public/r...
File content/public/renderer/browser_plugin_delegate.h (right):

https://chromiumcodereview.appspot.com/1162053003/diff/80001/content/public/r...
content/public/renderer/browser_plugin_delegate.h:47: virtual void WillDestroy()
{}
On 2015/06/05 18:52:33, Fady Samuel wrote:
> Rename this to ElementDestroyed.

DidDestroyElement()

https://chromiumcodereview.appspot.com/1162053003/diff/80001/content/public/r...
content/public/renderer/browser_plugin_delegate.h:52: virtual void Destroy() =
0;
On 2015/06/05 18:52:33, Fady Samuel wrote:
> Let's not expose this to content.

Done.

https://chromiumcodereview.appspot.com/1162053003/diff/80001/extensions/rende...
File extensions/renderer/guest_view/extensions_guest_view_container.cc (right):

https://chromiumcodereview.appspot.com/1162053003/diff/80001/extensions/rende...
extensions/renderer/guest_view/extensions_guest_view_container.cc:39:
destruction_callback_.Reset();
On 2015/06/05 18:52:33, Fady Samuel wrote:
> This shouldn't be necessary.

Done.

[Fady Samuel, 2015-06-05 22:12:47]

lgtm + nit

https://chromiumcodereview.appspot.com/1162053003/diff/140001/components/gues...
File components/guest_view/renderer/guest_view_container.h (right):

https://chromiumcodereview.appspot.com/1162053003/diff/140001/components/gues...
components/guest_view/renderer/guest_view_container.h:38: void
DidDestroyElement() final;
Does this need to be public?

https://chromiumcodereview.appspot.com/1162053003/diff/140001/extensions/rend...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://chromiumcodereview.appspot.com/1162053003/diff/140001/extensions/rend...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:198:
guest_view_container->DidDestroyElement();
nit: Call Destroy directly. DidDestroyElement is going away. Make Destroy
public, and move the comments there. Make DidDestroyElement implementation
private.

[lazyboy, 2015-06-05 22:26:15]

https://chromiumcodereview.appspot.com/1162053003/diff/140001/components/gues...
File components/guest_view/renderer/guest_view_container.h (right):

https://chromiumcodereview.appspot.com/1162053003/diff/140001/components/gues...
components/guest_view/renderer/guest_view_container.h:38: void
DidDestroyElement() final;
On 2015/06/05 22:12:46, Fady Samuel wrote:
> Does this need to be public?

Since Destroy() is now public, made this private.
Done.

https://chromiumcodereview.appspot.com/1162053003/diff/140001/extensions/rend...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://chromiumcodereview.appspot.com/1162053003/diff/140001/extensions/rend...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:198:
guest_view_container->DidDestroyElement();
On 2015/06/05 22:12:46, Fady Samuel wrote:
> nit: Call Destroy directly. DidDestroyElement is going away. Make Destroy
> public, and move the comments there. Make DidDestroyElement implementation
> private.

OK.
Done.

[lazyboy, 2015-06-05 23:09:28]

lazyboy@chromium.org changed reviewers:
+ jochen@chromium.org

[lazyboy, 2015-06-05 23:09:29]

+jochen for ONWERS of:

content/public/renderer/browser_plugin_delegate.h
and
content/renderer/render_frame_impl.cc

[jochen (gone - plz use gerrit), 2015-06-08 09:37:20]

lgtm

btw, it looks like the browser plugin stuff could be entirely moved to a
component? Most code in content appears to invoke hooks from the embedder only
now.

[lazyboy, 2015-06-09 18:05:58]

On 2015/06/08 09:37:20, jochen wrote:
> lgtm
> 
> btw, it looks like the browser plugin stuff could be entirely moved to a
> component? Most code in content appears to invoke hooks from the embedder only
> now.

OK.
Fady started moving guest related stuff to components/, he can probably
chime in about this. We depend on RenderViewImpl:: in browser_plugin.cc though,
e.g. for mouse_lock and focus.
Note that eventually we want to entirely get rid of browser plugin.

[Fady Samuel, 2015-06-09 18:51:50]

On 2015/06/09 18:05:58, lazyboy wrote:
> On 2015/06/08 09:37:20, jochen wrote:
> > lgtm
> > 
> > btw, it looks like the browser plugin stuff could be entirely moved to a
> > component? Most code in content appears to invoke hooks from the embedder
only
> > now.
> 
> OK.
> Fady started moving guest related stuff to components/, he can probably
> chime in about this. We depend on RenderViewImpl:: in browser_plugin.cc
though,
> e.g. for mouse_lock and focus.
> Note that eventually we want to entirely get rid of browser plugin.

Yea, I'd rather us just focus on killing BrowserPlugin and transitions to OOPIF
based embeddings.

[lazyboy, 2015-06-09 19:26:37]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2015-06-09 19:26:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, jochen@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1162053003/#ps180001 (title: "Sync
@tott")

[commit-bot: I haz the power, 2015-06-09 19:27:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1162053003/180001

[commit-bot: I haz the power, 2015-06-09 19:35:12]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-06-09 19:35:51]

Patchset 10 (id:??) landed as
https://crrev.com/cb6ba5c2ad69bf9daf896e5d3cd0c252c0ce6da3
Cr-Commit-Position: refs/heads/master@{#333552}