Move the Widget hierarchy to the Oilpan heap.

Source/core/html/HTMLFrameOwnerElement.cpp

 /*
  * Copyright (C) 2006, 2007, 2009 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 12 matching lines @@
 
 #include "bindings/core/v8/ExceptionMessages.h"
 #include "bindings/core/v8/ExceptionState.h"
 #include "core/accessibility/AXObjectCache.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/events/Event.h"
 #include "core/frame/FrameView.h"
 #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"
 #include "platform/weborigin/SecurityOrigin.h"
 #include "platform/weborigin/SecurityPolicy.h"
 
 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()
 {
     DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<WillBeHeapHashCountedSet<RawPtrWillBeMember<Node> > >, nodes, (adoptPtrWillBeNoop(new WillBeHeapHashCountedSet<RawPtrWillBeMember<Node> >())));
     return *nodes;
 }
 
 static unsigned s_updateSuspendCount = 0;
 
 HTMLFrameOwnerElement::UpdateSuspendScope::UpdateSuspendScope()
 {
     ++s_updateSuspendCount;
 }
 
 void HTMLFrameOwnerElement::UpdateSuspendScope::performDeferredWidgetTreeOperations()
 {
     WidgetToParentMap map;
     widgetNewParentMap().swap(map);
     WidgetToParentMap::iterator end = map.end();
     for (WidgetToParentMap::iterator it = map.begin(); it != end; ++it) {
         Widget* child = it->key.get();
         ScrollView* currentParent = toScrollView(child->parent());
         FrameView* newParent = it->value;
         if (newParent != currentParent) {
             if (currentParent)
                 currentParent->removeChild(child);
             if (newParent)
                 newParent->addChild(child);
+#if ENABLE(OILPAN)
+            if (currentParent && !newParent)
+                HTMLFrameOwnerElement::disposeWidget(child);
+#endif
         }
     }
 }
 
 HTMLFrameOwnerElement::UpdateSuspendScope::~UpdateSuspendScope()
 {
     ASSERT(s_updateSuspendCount > 0);
     if (s_updateSuspendCount == 1)
         performDeferredWidgetTreeOperations();
     --s_updateSuspendCount;
 }
 
 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);
 }
 
 HTMLFrameOwnerElement::HTMLFrameOwnerElement(const QualifiedName& tagName, Document& document)
     : HTMLElement(tagName, document)
     , m_contentFrame(nullptr)
     , m_widget(nullptr)
     , m_sandboxFlags(SandboxNone)
@@ skipping 33 matching lines @@
 }
 
 void HTMLFrameOwnerElement::disconnectContentFrame()
 {
     // FIXME: Currently we don't do this in removedFrom because this causes an
     // unload event in the subframe which could execute script that could then
     // reach up into this document and then attempt to look back down. We should
     // 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()
 {
 #if ENABLE(OILPAN)
     // An owner must by now have been informed of detachment
     // when the frame was closed.
     ASSERT(!m_contentFrame);
 #else
     if (m_contentFrame)
         m_contentFrame->disconnectOwnerElement();
 #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;
 }
 
 LocalDOMWindow* HTMLFrameOwnerElement::contentWindow() const
 {
     return m_contentFrame ? m_contentFrame->domWindow() : 0;
 }
 
@@ skipping 13 matching lines @@
 }
 
 Document* HTMLFrameOwnerElement::getSVGDocument(ExceptionState& exceptionState) const
 {
     Document* doc = contentDocument();
     if (doc && doc->isSVGDocument())
         return doc;
     return 0;
 }
 
-void HTMLFrameOwnerElement::setWidget(PassRefPtr<Widget> widget)
+void HTMLFrameOwnerElement::setWidget(PassRefPtrWillBeRawPtr<Widget> widget)
 {
     if (widget == m_widget)
         return;
 
     if (m_widget) {
         if (m_widget->parent())
             moveWidgetToParentSoon(m_widget.get(), 0);
         m_widget = nullptr;
     }
 
@@ skipping 37 matching lines @@
     if (!SubframeLoadingDisabler::canLoadFrame(*this))
         return false;
 
     String referrer = SecurityPolicy::generateReferrerHeader(document().referrerPolicy(), url, document().outgoingReferrer());
     return parentFrame->loader().client()->createFrame(url, frameName, Referrer(referrer, document().referrerPolicy()), this);
 }
 
 void HTMLFrameOwnerElement::trace(Visitor* visitor)
 {
     visitor->trace(m_contentFrame);
+    visitor->trace(m_widget);
     HTMLElement::trace(visitor);
     FrameOwner::trace(visitor);
 }
 
 
 } // namespace blink