Move the Widget hierarchy to the Oilpan heap.

Source/core/html/HTMLFrameOwnerElement.cpp

Index: Source/core/html/HTMLFrameOwnerElement.cpp
diff --git a/Source/core/html/HTMLFrameOwnerElement.cpp b/Source/core/html/HTMLFrameOwnerElement.cpp
index 014638445263f52c4767624a3e398f2aebc0084d..f0a9ed8b9f93fa138ce03b7ad7ad924c7af9c30d 100644
--- a/Source/core/html/HTMLFrameOwnerElement.cpp
+++ b/Source/core/html/HTMLFrameOwnerElement.cpp
@@ -30,6 +30,7 @@
 #include "core/frame/LocalFrame.h"
 #include "core/loader/FrameLoader.h"
 #include "core/loader/FrameLoaderClient.h"
+#include "core/plugins/PluginView.h"
 #include "core/rendering/RenderLayer.h"
 #include "core/rendering/RenderPart.h"
 #include "core/rendering/compositing/RenderLayerCompositor.h"
@@ -38,11 +39,11 @@
 
 namespace blink {
 
-typedef HashMap<RefPtr<Widget>, FrameView*> WidgetToParentMap;
+typedef WillBeHeapHashMap<RefPtrWillBeMember<Widget>, RawPtrWillBeMember<FrameView> > WidgetToParentMap;
 static WidgetToParentMap& widgetNewParentMap()
 {
-    DEFINE_STATIC_LOCAL(WidgetToParentMap, map, ());
-    return map;
+    DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<WidgetToParentMap>, map, (adoptPtrWillBeNoop(new WidgetToParentMap())));

[haraken, 2014/09/26 09:19:25]

Just to confirm: This is a persistent map of strong members. Won't it cause
leaks?

[sof, 2014/09/28 15:11:25]

The map contains shortlived (Widget, FrameView) pairs -- put there when widget
hierarchy updates are temporarily suspended -- so this will not retain objects
any longer than it needs to.

+    return *map;
 }
 
 WillBeHeapHashCountedSet<RawPtrWillBeMember<Node> >& SubframeLoadingDisabler::disabledSubtreeRoots()
@@ -72,6 +73,10 @@ void HTMLFrameOwnerElement::UpdateSuspendScope::performDeferredWidgetTreeOperati
                 currentParent->removeChild(child);
             if (newParent)
                 newParent->addChild(child);
+#if ENABLE(OILPAN)
+            if (currentParent && !newParent)
+                HTMLFrameOwnerElement::disposeWidget(child);
+#endif
         }
     }
 }
@@ -87,10 +92,14 @@ HTMLFrameOwnerElement::UpdateSuspendScope::~UpdateSuspendScope()
 static void moveWidgetToParentSoon(Widget* child, FrameView* parent)
 {
     if (!s_updateSuspendCount) {
-        if (parent)
+        if (parent) {
             parent->addChild(child);
-        else if (toScrollView(child->parent()))
+        } else if (toScrollView(child->parent())) {
             toScrollView(child->parent())->removeChild(child);
+#if ENABLE(OILPAN)
+            HTMLFrameOwnerElement::disposeWidget(child);
+#endif
+        }
         return;
     }
     widgetNewParentMap().set(child, parent);
@@ -144,17 +153,12 @@ void HTMLFrameOwnerElement::disconnectContentFrame()
     // see if this behavior is really needed as Gecko does not allow this.
     if (RefPtrWillBeRawPtr<Frame> frame = contentFrame()) {
         frame->detach();
-#if ENABLE(OILPAN)
-        // FIXME: Oilpan: the plugin container is released and finalized here
-        // in order to work around the current inability to make the plugin
-        // container a FrameDestructionObserver (it needs to effectively be on
-        // the heap, and Widget isn't). Hence, release it here while its
-        // frame reference is still valid.
-        if (m_widget && m_widget->isPluginContainer())
-            m_widget = nullptr;
-#endif
         frame->disconnectOwnerElement();
     }
+#if ENABLE(OILPAN)
+    disposeWidget(m_widget.get());
+    m_widget = nullptr;
+#endif
 }
 
 HTMLFrameOwnerElement::~HTMLFrameOwnerElement()
@@ -169,6 +173,18 @@ HTMLFrameOwnerElement::~HTMLFrameOwnerElement()
 #endif
 }
 
+#if ENABLE(OILPAN)
+void HTMLFrameOwnerElement::disposeWidget(Widget* widget)
+{
+    // Oilpan: a plugin container must be disposed of prior to (GC)
+    // finalization of it and all it depends on. Needed to ensure that
+    // the underlying plugin is able to access a fully-functioning
+    // frame while it destructs.
+    if (widget && widget->isPluginView())
+        toPluginView(widget)->dispose();

[haraken, 2014/09/26 09:19:25]

Hmm, the fact that the plugin container needs a prompt disposal adds a lot of
complexity to the code base. Can't we somehow remove the need of the prompt
disposal from the plugin container? (I have no idea though.)

[sof, 2014/09/28 15:11:25]

When the plugin is destroyed, the underlying plugin may perform an arbitrary
amount of work, which may well involve accessing the frame and its document etc.

So, this needs to be done before any of those are finalized. Doing it at a later
stage (e.g., as part of finalizing the plugin container itself) is just not
safe, I came to realize. I agree that it is not simple to handle correctly &
reliably - it's driven me to distraction more than once :) - but the complexity
seems unavoidable in general.

(This issue touches directly on https://codereview.chromium.org/603193005/#msg2
-- the careful use of dispose() over PluginViews feels unnatural and low-level.
But I don't have any better alternatives other than not moving Widget to the
heap. Not what I suggest we do, but it's an option.)

+}
+#endif
+
 Document* HTMLFrameOwnerElement::contentDocument() const
 {
     return (m_contentFrame && m_contentFrame->isLocalFrame()) ? toLocalFrame(m_contentFrame)->document() : 0;
@@ -202,7 +218,7 @@ Document* HTMLFrameOwnerElement::getSVGDocument(ExceptionState& exceptionState)
     return 0;
 }
 
-void HTMLFrameOwnerElement::setWidget(PassRefPtr<Widget> widget)
+void HTMLFrameOwnerElement::setWidget(PassRefPtrWillBeRawPtr<Widget> widget)
 {
     if (widget == m_widget)
         return;
@@ -260,6 +276,7 @@ bool HTMLFrameOwnerElement::loadOrRedirectSubframe(const KURL& url, const Atomic
 void HTMLFrameOwnerElement::trace(Visitor* visitor)
 {
     visitor->trace(m_contentFrame);
+    visitor->trace(m_widget);
     HTMLElement::trace(visitor);
     FrameOwner::trace(visitor);
 }