Move the Widget hierarchy to the Oilpan heap.

Source/core/frame/LocalFrame.cpp

Index: Source/core/frame/LocalFrame.cpp
diff --git a/Source/core/frame/LocalFrame.cpp b/Source/core/frame/LocalFrame.cpp
index 365bb0381a686066e5ba2fe36b0bb26730278847..53d994eb092f0dcdf59400663637b997a5edd976 100644
--- a/Source/core/frame/LocalFrame.cpp
+++ b/Source/core/frame/LocalFrame.cpp
@@ -48,6 +48,7 @@
 #include "core/frame/LocalDOMWindow.h"
 #include "core/frame/Settings.h"
 #include "core/html/HTMLFrameElementBase.h"
+#include "core/html/HTMLPlugInElement.h"
 #include "core/inspector/ConsoleMessageStorage.h"
 #include "core/inspector/InspectorInstrumentation.h"
 #include "core/loader/FrameLoaderClient.h"
@@ -145,6 +146,7 @@ void LocalFrame::trace(Visitor* visitor)
     visitor->trace(m_destructionObservers);
     visitor->trace(m_loader);
     visitor->trace(m_navigationScheduler);
+    visitor->trace(m_view);
     visitor->trace(m_pagePopupOwner);
     visitor->trace(m_script);
     visitor->trace(m_editor);
@@ -153,11 +155,23 @@ void LocalFrame::trace(Visitor* visitor)
     visitor->trace(m_eventHandler);
     visitor->trace(m_console);
     visitor->trace(m_inputMethodController);
+    visitor->trace(m_pluginElements);

[haraken, 2014/10/11 17:33:02]

Is it safe to trace m_pluginElements? There is no guarantee about the order of
default weak processing and custom weak processing (i.e., clearWeakMembers).

For clarity, I'd prefer not tracing m_pluginElements and handing everything in
clearWeakMembers.

[sof, 2014/10/12 08:16:22]

It is safe with the weak callback stack that the implementation keeps. i.e.,
clearWeakMembers() will be invoked before the callback for the weak hash set
impl.
Not clear what you're effectively suggesting. I see three possible
interpretations:

 - Stop using Oilpan types (HashSet<HTMLPlugInElement*>) and process that in
   clearWeakMembers() using isAlive() and then later removing the dead entries.

   (A pity to not be able to use Oilpan types to express Oilpan weak reference
    handling! :) )

 - Iterate over a HeapHashSet<WeakMember<HTMLPlugInElement> > checking for
liveness.
   Then mark & register a weak callback for the hash set to be processed in a
later
   callback. Are those operations allowed from within a(nother) weak callback?

 - Somehow emulate what the trace() over weak hash set currently implements,
iterating
   over the elements checking for liveness (and calling shouldDisposePlugin()),
but
   then removing the dead entries. Removing such doesn't appear to be supported
without
   running the risk of a rehash. And how would you mark-but-not-trace the
backing store
   itself. i.e., the functionality that the hash table trace() uses doesn't
appear to
   be supported in "user defined" weak callbacks -- please tell me if I'm
missing some
   details that really do allow this to be expressed.

If the assumption made with what's currently implemented is thought
inappropriate, I can
align with any of the above three -- which one is preferable in that case?

[haraken, 2014/10/12 12:56:04]

I'm suggesting the first one :)

If I'm understanding code correctly, weak processing for built-in hash maps is
executed during a GC phase (while all other threads are paused). Weak processing
for clearWeakMembers is executed in thread-specific weak processing, which
happens after the GC phase.

Thus, I guess isAlive() in clearWeakMembers always returns true.

[sof, 2014/10/12 13:09:12]

It does not always return true; if no one has marked it by that time, it will
return false.

Which you'll see happening on plugins/can-create-without-renderer.html

[haraken, 2014/10/12 13:38:23]

Do you know how it's guaranteed that weak processing for hash tables happens
before custom weak processing?

As far as I see HashTable.h, it looks like the weak processing for hash tables
are registered by registerWeakMember, which means that there is no guarantee in
order in which the weak processing for hash tables and the custom weak
processing are triggered.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

(Sorry, I was wrong in that the weak processing for hash tables runs in a GC
phase.)

[sof, 2014/10/12 16:22:13]

There are no such guarantees, but the weak callbacks are popped from their stack
when performed, so if you register the weak callback _after_ the tracing of a
weak collection (which does the same kind of registration), then that weak
callback will be performed first, assuming they belong to the same thread.

I understand if that is an implementation detail we shouldn't rely on (but it is
kind of handy to have/make-use-of right here.)

[sof, 2014/10/12 18:35:36]

I've switched m_pluginElements to HashSet<HTMLPlugInElement*> and adjusted the
weak pointer handling accordingly.

We can follow up and change it to a "higher level" representation later, if that
ends up the conclusion.

+    visitor->registerWeakMembers<LocalFrame, &LocalFrame::clearWeakMembers>(this);
     HeapSupplementable<LocalFrame>::trace(visitor);
 #endif
     Frame::trace(visitor);
 }
 
+#if ENABLE(OILPAN)
+void LocalFrame::clearWeakMembers(Visitor* visitor)
+{
+    for (HeapHashSet<WeakMember<HTMLPlugInElement> >::const_iterator it = m_pluginElements.begin(); it != m_pluginElements.end(); ++it) {
+        if (!visitor->isAlive(*it))
+            (*it)->shouldDisposePlugin();

[haraken, 2014/10/11 17:33:02]

shouldDisposePlugin() only does simple things and thus can be called in a
pre-finalizer. Can we probably call WebPluginContainerImpl::dispose() in
HTMLPluginElement's pre-finalizer? Then we can remove the weak hash map from
LocalFrame.

[sof, 2014/10/12 08:16:22]

How would you know if the LocalFrame is still alive to allow that to go ahead?

I don't think this suggestion will work -- we trigger it from here when we
_know_ that the Frame has survived (but the plugin element hasn't.)

[haraken, 2014/10/12 12:56:04]

I'm a bit confused. Pre-finalizers are invoked before any object in the thread
heap is swept. So I guess it's guaranteed that the Frame is still alive in the
pre-finalizers.

[sof, 2014/10/12 13:09:12]

It's still alive, yes. But the plugin is allowed to execute scripts & the like
(== allocations.)

[haraken, 2014/10/12 13:38:23]

I agree that dispose() does a bunch of things, but shouldDisposePlugin doesn't
do anything complicated, does it?

[sof, 2014/10/12 16:22:13]

Yes, it does not, but it should only be called/signalled _iff_ the LocalFrame is
still alive (and we've not managed to cleanly dispose of the plugin already.)

That information isn't locally available to the plugin element in a
pre-finalizer (*). (It is a solution I looked at and considered.)

* - I think it would be somewhat useful if the pre-finalizer method took
Visitor* as an argument.

+    }
+}
+#endif
+
 void LocalFrame::navigate(Document& originDocument, const KURL& url, bool lockBackForwardList)
 {
     m_navigationScheduler.scheduleLocationChange(&originDocument, url.string(), lockBackForwardList);
@@ -202,8 +216,9 @@ void LocalFrame::detachView()
         m_view->prepareForDetach();
 }
 
-void LocalFrame::setView(PassRefPtr<FrameView> view)
+void LocalFrame::setView(PassRefPtrWillBeRawPtr<FrameView> view)
 {
+    ASSERT(!m_view || m_view != view);
     detachView();
 
     // Prepare for destruction now, so any unload event handlers get run and the LocalDOMWindow is
@@ -448,7 +463,7 @@ void LocalFrame::createView(const IntSize& viewportSize, const Color& background
 
     setView(nullptr);
 
-    RefPtr<FrameView> frameView;
+    RefPtrWillBeRawPtr<FrameView> frameView = nullptr;
     if (isLocalRoot) {
         frameView = FrameView::create(this, viewportSize);
 
@@ -723,6 +738,12 @@ void LocalFrame::disconnectOwnerElement()
         if (Document* document = this->document())
             document->topDocument().clearAXObjectCache();
 #if ENABLE(OILPAN)
+        // First give the plugin elements holding persisted,
+        // renderer-less plugins the opportunity to dispose of them.
+        for (HeapHashSet<WeakMember<HTMLPlugInElement> >::const_iterator it = m_pluginElements.begin(); it != m_pluginElements.end(); ++it)
+            (*it)->disconnectContentFrame();
+        m_pluginElements.clear();
+
         // Clear the FrameView and FrameLoader right here rather than
         // during finalization. Too late to access various heap objects
         // at that stage.
@@ -747,4 +768,18 @@ void LocalFrame::setPagePopupOwner(Element& owner)
     m_pagePopupOwner = &owner;
 }
 
+#if ENABLE(OILPAN)
+void LocalFrame::registerPluginElement(HTMLPlugInElement* plugin)
+{
+    m_pluginElements.add(plugin);
+}
+
+void LocalFrame::unregisterPluginElement(HTMLPlugInElement* plugin)
+{
+    ASSERT(!ThreadState::current()->isSweepInProgress());

[haraken, 2014/10/11 17:33:02]

I wanted to add this check since it would cause an issue if
m_pluginElements.remove runs during the sweeping phase. However, there will be
no issue, so the check is meaningless. Sorry, let's remove the check.

[sof, 2014/10/12 08:16:22]

Gone :)

+    ASSERT(m_pluginElements.contains(plugin));
+    m_pluginElements.remove(plugin);
+}
+#endif
+
 } // namespace blink