|
|
Created:
6 years, 6 months ago by sof Modified:
6 years, 6 months ago Reviewers:
haraken, oilpan-reviews, tkent CC:
blink-reviews, blink-reviews-html_chromium.org, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionOilpan: 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
Patch Set 1 #Patch Set 2 : Conditionally define detach() #
Total comments: 13
Patch Set 3 : Have WebPluginContainerImpl be a FrameDestructionObserver #
Created: 6 years, 6 months ago
Messages
Total messages: 16 (0 generated)
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.
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.
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.
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?
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...
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.)
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?
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.
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.
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?
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 :)
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.
LGTM
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/357443005/40001
Message was sent while issue was closed.
Change committed as 176967 |