Oilpan: make shutdown of plugin container objects work better.

Oilpan: make shutdown of plugin container objects work better.

The plugin container object/widget that's kept by the plugin element has
a destructor that accesses its element's document in a couple of ways:

 - Optionally notifying that a touch event handler has been removed.
 - Tries to notify element (and its document) that compositing update
   is needed.
 - Indirectly clears out the script objects registered on the document
   frame's ScriptController when destroying the container's plugin object.

With Oilpan, none of these can be done during finalization as the document
cannot be assumed to be alive.

To address, introduce an explicit detach() step for when the element is
cleanly removed from the document (along with its container.) Otherwise,
the plugin container is assumed to go away at the same time as the
element (and the document.) The container's destructor will then only
notify any off-heap objects.

R=tkent@chromium.org,haraken@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176967

[sof, 2014-06-24 21:24:35]

Please take a look/peek.

Testing is a bit wonky this evening, so I don't have a complete run on the
windows trybots available for verification, but I think this is ready for some
feedback.

[tkent, 2014-06-25 05:05:40]

lgtm.


This looks reasonable.

https://codereview.chromium.org/357443005/diff/20001/Source/platform/Widget.h
File Source/platform/Widget.h (right):

https://codereview.chromium.org/357443005/diff/20001/Source/platform/Widget.h...
Source/platform/Widget.h:42: #if ENABLE(OILPAN)
nit: You don't need to add |#if ENABLE(OILPAN)| to class declarations. It
doesn't affect generated code of enable_oilpan=0.

[haraken, 2014-06-25 05:40:16]

https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:206: plugin->detach();

Is it OK to detach the plugin that is persisted by m_persistedPluginWidget?

https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:207: #endif

if (plugin) {
#if ENABLE(OILPAN)
  plugin->detach();
#endif
  if (plugin->pluginShouldPersist())
    m_persistedPluginWidget = plugin;
}

https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginCont...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.cpp:436:
m_scriptController->cleanupScriptObjectsForPlugin(this);

Maybe should we call cleanupScriptObjectsForPlugin in
WebPluginContainerImpl::detach() instead of in the destructor? I don't quite see
a reason why we have to delay the clean-up to the destructor.

https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.cpp:708:
GraphicsLayer::unregisterContentsLayer(m_webLayer);

Probably can we move the line 704 - 708 to detach()?

In my understanding, in terms of semantics, the WebPluginContainer should die
when the HTMLPluginElement throws away the reference to the WebPluginContainer.
If so, we can do all the clean-up in detach() instead of in the destructor.

[sof, 2014-06-25 05:44:25]

https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginCont...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.cpp:436:
m_scriptController->cleanupScriptObjectsForPlugin(this);
On 2014/06/25 05:40:16, haraken wrote:
> 
> Maybe should we call cleanupScriptObjectsForPlugin in
> WebPluginContainerImpl::detach() instead of in the destructor? I don't quite
see
> a reason why we have to delay the clean-up to the destructor.

Is Node::detach() (from where we call this container's detach()) guaranteed to
be called always?

[haraken, 2014-06-25 05:54:24]

https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginCont...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.cpp:436:
m_scriptController->cleanupScriptObjectsForPlugin(this);
On 2014/06/25 05:44:25, sof wrote:
> On 2014/06/25 05:40:16, haraken wrote:
> > 
> > Maybe should we call cleanupScriptObjectsForPlugin in
> > WebPluginContainerImpl::detach() instead of in the destructor? I don't quite
> see
> > a reason why we have to delay the clean-up to the destructor.
> 
> Is Node::detach() (from where we call this container's detach()) guaranteed to
> be called always?

ah, understood.

Then would it be possible to call cleanupScriptObjectsForPlugin() from
HTMLPluginElement's destructor?

I think it's a responsibility of the element that owns the  WebPluginContainer
to call cleanupScriptObjectsForPlugin(). It looks a bit hacky to pass a bare
pointer of ScriptController to the WebPluginContainer and let the
WebPluginContainer call cleanupScriptObjectsForPlugin().

https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.cpp:717: setWebLayer(0);

Just help me understand: Calling setWebLayer(0) is needed here to prevent what?
I guess it's related to "Tries to notify element (and its document) that
compositing update is needed" in your CL description...

[sof, 2014-06-25 07:09:32]

https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:206: plugin->detach();
On 2014/06/25 05:40:16, haraken wrote:
> 
> Is it OK to detach the plugin that is persisted by m_persistedPluginWidget?

We don't any kind of detachment in the non-Oilpan case, but I can look into
this.

https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginCont...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.cpp:436:
m_scriptController->cleanupScriptObjectsForPlugin(this);
On 2014/06/25 05:54:24, haraken wrote:
> On 2014/06/25 05:44:25, sof wrote:
> > On 2014/06/25 05:40:16, haraken wrote:
> > > 
> > > Maybe should we call cleanupScriptObjectsForPlugin in
> > > WebPluginContainerImpl::detach() instead of in the destructor? I don't
quite
> > see
> > > a reason why we have to delay the clean-up to the destructor.
> > 
> > Is Node::detach() (from where we call this container's detach()) guaranteed
to
> > be called always?
> 
> ah, understood.
> 
> Then would it be possible to call cleanupScriptObjectsForPlugin() from
> HTMLPluginElement's destructor?
> 
> I think it's a responsibility of the element that owns the  WebPluginContainer
> to call cleanupScriptObjectsForPlugin(). It looks a bit hacky to pass a bare
> pointer of ScriptController to the WebPluginContainer and let the
> WebPluginContainer call cleanupScriptObjectsForPlugin().

(cleanupScriptObjectsForPlugin() is something the WebPlugin brings about when
destroyed, calling clearScriptObjects(). We could nullify its effect, of
course.)

Re: ScriptController and keeping a reference here. Yes, I sort of agree that
this isn't increasing tidiness, but as the abstractions are so far apart here
and there are multiple heap objects in between them (the element, document, and
possibly the frame at some point), I don't see an alternative.
(=> if there are alternatives, I'm all ears :) )

Giving the plugin element the responsibility of cleaning up the scripts added by
the WebPlugin is a bit unnatural; it has similar problems -- how do you access
document().frame()->script() from the HTMLPlugInElement's destructor?

We could move code out of the dtor and have a dispose()-like method instead
that's invoked from the HTMLPlugInElement destructor. But we do still need a
detach() method to perform Document-touching operations (when possible.) I saw
less value in having a dispose() also, so didn't do it here.

(Again, this is quite similar to issues we ran into with the WebMediaPlayerImpl
shutdown -- I'm not sure there are really tidy arrangements to be found.)

https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.cpp:717: setWebLayer(0);
On 2014/06/25 05:54:24, haraken wrote:
> 
> Just help me understand: Calling setWebLayer(0) is needed here to prevent
what?
> I guess it's related to "Tries to notify element (and its document) that
> compositing update is needed" in your CL description...
> 

Exactly so; we cannot do it at finalization time as the document isn't valid,
but the WebPlugin does call setWebLayer(0) when being destroyed. The argument
for why it is acceptable not to do it if the HTMLPlugInElement isn't detached is
that everything is going away, hence doing the call to
setNeedsCompositingUpdate() isn't needed.

(This is touching on the same issues we've seen before; e.g., the media player
shutdown steps.)

[haraken, 2014-06-25 09:20:50]

I looked into the code and understood what this CL is doing.

I still think that adding a bare pointer of ScriptController to the
WebPluginContainerImpl looks hacky. Would it be possible to make the
WebPluginContainerImpl derive FrameDestructionObserver and add m_frame to the
WebPluginContainerImpl? (We can clear the m_frame in
FrameDestructionObserver::frameDestroyed().) Then we can use the m_frame to get
the ScriptController.

What do you think?

[sof, 2014-06-25 09:23:50]

https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:206: plugin->detach();
On 2014/06/25 07:09:32, sof wrote:
> On 2014/06/25 05:40:16, haraken wrote:
> > 
> > Is it OK to detach the plugin that is persisted by m_persistedPluginWidget?
> 
> We don't any kind of detachment in the non-Oilpan case, but I can look into
> this.

I don't think we should do this as part of this CL. It seems reasonable to make
use of the detach() step now that we have it for the container and the plugin is
being saved away but no longer displayed, but it does change behavior wrt
non-Oilpan.

[haraken, 2014-06-25 09:31:06]

https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:206: plugin->detach();
On 2014/06/25 09:23:50, sof wrote:
> On 2014/06/25 07:09:32, sof wrote:
> > On 2014/06/25 05:40:16, haraken wrote:
> > > 
> > > Is it OK to detach the plugin that is persisted by
m_persistedPluginWidget?
> > 
> > We don't any kind of detachment in the non-Oilpan case, but I can look into
> > this.
> 
> I don't think we should do this as part of this CL. It seems reasonable to
make
> use of the detach() step now that we have it for the container and the plugin
is
> being saved away but no longer displayed, but it does change behavior wrt
> non-Oilpan.

Makes sense, agreed.

[sof, 2014-06-25 10:31:24]

On 2014/06/25 at 09:20:50, haraken wrote:
> I looked into the code and understood what this CL is doing.
> 
> I still think that adding a bare pointer of ScriptController to the
WebPluginContainerImpl looks hacky. Would it be possible to make the
WebPluginContainerImpl derive FrameDestructionObserver and add m_frame to the
WebPluginContainerImpl? (We can clear the m_frame in
FrameDestructionObserver::frameDestroyed().) Then we can use the m_frame to get
the ScriptController.
> 
> What do you think?

I think that's a most excellent suggestion :-) Much better, thanks - it's what I
was looking for I now realize (i.e., the added worry that the frame and its
ScriptController might disappear out-of-turn is additionally taken care of by
observing the frame.)

In the interest of code simplicity - acceptable to also derive from
FrameDestructionObserver in the non-Oilpan case?

[haraken, 2014-06-25 10:44:26]

On 2014/06/25 10:31:24, sof wrote:
> On 2014/06/25 at 09:20:50, haraken wrote:
> > I looked into the code and understood what this CL is doing.
> > 
> > I still think that adding a bare pointer of ScriptController to the
> WebPluginContainerImpl looks hacky. Would it be possible to make the
> WebPluginContainerImpl derive FrameDestructionObserver and add m_frame to the
> WebPluginContainerImpl? (We can clear the m_frame in
> FrameDestructionObserver::frameDestroyed().) Then we can use the m_frame to
get
> the ScriptController.
> > 
> > What do you think?
> 
> I think that's a most excellent suggestion :-) Much better, thanks - it's what
I
> was looking for I now realize (i.e., the added worry that the frame and its
> ScriptController might disappear out-of-turn is additionally taken care of by
> observing the frame.)
> 
> In the interest of code simplicity - acceptable to also derive from
> FrameDestructionObserver in the non-Oilpan case?

I think it would be acceptable but let's get an approval of experts before
landing :)

[tkent, 2014-06-26 03:53:43]

On 2014/06/25 10:44:26, haraken wrote:
> > In the interest of code simplicity - acceptable to also derive from
> > FrameDestructionObserver in the non-Oilpan case?
> 
> I think it would be acceptable but let's get an approval of experts before
> landing :)

I'm not an expert, but it still lgtm.

[haraken, 2014-06-26 04:10:54]

LGTM

[sof, 2014-06-26 05:20:11]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-06-26 05:21:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/357443005/40001

[commit-bot: I haz the power, 2014-06-26 06:59:52]

Change committed as 176967