Oilpan: make shutdown of plugin container objects work better.

Source/web/WebPluginContainerImpl.cpp

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 285 matching lines @@
 
     // If either of the layers is null we need to switch between hardware
     // and software compositing.
     bool needsCompositingUpdate = !m_webLayer || !layer;
 
     m_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.
     if (RenderPart* renderer = m_element->renderPart()) {
         ASSERT(renderer->hasLayer());
         renderer->layer()->updateSelfPaintingLayer();
     }
 }
 
 bool WebPluginContainerImpl::supportsPaginatedPrint() const
@@ skipping 103 matching lines @@
         m_scrollbarGroup->setFrameRect(frameRect());
     }
 }
 
 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();
 }
 
 WebString WebPluginContainerImpl::executeScriptURL(const WebURL& url, bool popupsAllowed)
 {
     LocalFrame* frame = m_element->document().frame();
     if (!frame)
         return WebString();
@@ skipping 203 matching lines @@
     context->fillRect(intersection(verticalOverhangArea, dirtyRect));
     context->restore();
     return true;
 }
 
 // Private methods -------------------------------------------------------------
 
 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)
 {
 }
 
 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();
     m_webPlugin->destroy();
     if (m_webLayer)
         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());
 
     if (event->isDragEvent()) {
         if (m_webPlugin->canProcessDrag())
             handleDragEvent(event);
         return;
     }
 
@@ skipping 225 matching lines @@
 
     return clipRect;
 }
 
 bool WebPluginContainerImpl::pluginShouldPersist() const
 {
     return m_webPlugin->shouldPersist();
 }
 
 } // namespace blink