Oilpan: make shutdown of plugin container objects work better.

Source/web/WebPluginContainerImpl.cpp

Index: Source/web/WebPluginContainerImpl.cpp
diff --git a/Source/web/WebPluginContainerImpl.cpp b/Source/web/WebPluginContainerImpl.cpp
index 5f5bd41aefe882ae08f1652521b96cdec90ed94a..1c3ef88099e24c0d411c216e89518d8a48d38348 100644
--- a/Source/web/WebPluginContainerImpl.cpp
+++ b/Source/web/WebPluginContainerImpl.cpp
@@ -303,6 +303,11 @@ void WebPluginContainerImpl::setWebLayer(WebLayer* layer)
     if (!needsCompositingUpdate)
         return;
 
+#if ENABLE(OILPAN)
+    if (!m_element)
+        return;
+#endif
+
     m_element->setNeedsCompositingUpdate();
     // Being composited or not affects the self painting layer bit
     // on the RenderLayer.
@@ -426,12 +431,26 @@ void WebPluginContainerImpl::allowScriptObjects()
 
 void WebPluginContainerImpl::clearScriptObjects()
 {
+#if ENABLE(OILPAN)
+    if (m_scriptController) {
+        m_scriptController->cleanupScriptObjectsForPlugin(this);

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

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.

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

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

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

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().

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

(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.)

+        m_scriptController = nullptr;
+    }
+#else
     LocalFrame* frame = m_element->document().frame();
     if (!frame)
         return;
     frame->script().cleanupScriptObjectsForPlugin(this);
+#endif
 }
 
+#if ENABLE(OILPAN)
+void WebPluginContainerImpl::setScriptController(WebCore::ScriptController* scriptController)
+{
+    m_scriptController = scriptController;
+}
+#endif
+
 NPObject* WebPluginContainerImpl::scriptableObjectForElement()
 {
     return m_element->getNPObject();
@@ -655,6 +674,9 @@ bool WebPluginContainerImpl::paintCustomOverhangArea(GraphicsContext* context, c
 WebPluginContainerImpl::WebPluginContainerImpl(WebCore::HTMLPlugInElement* element, WebPlugin* webPlugin)
     : m_element(element)
     , m_webPlugin(webPlugin)
+#if ENABLE(OILPAN)
+    , m_scriptController(0)
+#endif
     , m_webLayer(0)
     , m_touchEventRequestType(TouchEventRequestTypeNone)
     , m_wantsWheelEvents(false)
@@ -663,8 +685,21 @@ WebPluginContainerImpl::WebPluginContainerImpl(WebCore::HTMLPlugInElement* eleme
 
 WebPluginContainerImpl::~WebPluginContainerImpl()
 {
+#if ENABLE(OILPAN)
+    // The element (and its document) are heap allocated and may
+    // have been finalized by now; unsafe to unregister the touch
+    // event handler at this stage.
+    //
+    // This is acceptable, as the widget will unregister itself if it
+    // is cleanly detached. If an explicit detach doesn't happen, this
+    // container is assumed to have died with the plugin element (and
+    // its document), hence no unregistration step is needed.
+    //
+    m_element = 0;
+#else
     if (m_touchEventRequestType != TouchEventRequestTypeNone)
         m_element->document().didRemoveTouchEventHandler(m_element);
+#endif
 
     for (size_t i = 0; i < m_pluginLoadObservers.size(); ++i)
         m_pluginLoadObservers[i]->clearPluginContainer();
@@ -673,6 +708,16 @@ WebPluginContainerImpl::~WebPluginContainerImpl()
         GraphicsLayer::unregisterContentsLayer(m_webLayer);

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

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.

 }
 
+#if ENABLE(OILPAN)
+void WebPluginContainerImpl::detach()
+{
+    if (m_touchEventRequestType != TouchEventRequestTypeNone)
+        m_element->document().didRemoveTouchEventHandler(m_element);
+
+    setWebLayer(0);

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

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]

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.)

+}
+#endif
+
 void WebPluginContainerImpl::handleMouseEvent(MouseEvent* event)
 {
     ASSERT(parent()->isFrameView());