Move the Widget hierarchy to the Oilpan heap.

Source/core/frame/LocalFrame.cpp

 /*
  * Copyright (C) 1998, 1999 Torben Weis <weis@kde.org>
  *                     1999 Lars Knoll <knoll@kde.org>
  *                     1999 Antti Koivisto <koivisto@kde.org>
  *                     2000 Simon Hausmann <hausmann@kde.org>
  *                     2000 Stefan Schimanski <1Stein@gmx.de>
  *                     2001 George Staikos <staikos@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2005 Alexey Proskuryakov <ap@nypop.com>
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
@@ skipping 30 matching lines @@
 #include "core/events/Event.h"
 #include "core/fetch/ResourceFetcher.h"
 #include "core/frame/EventHandlerRegistry.h"
 #include "core/frame/FrameConsole.h"
 #include "core/frame/FrameDestructionObserver.h"
 #include "core/frame/FrameHost.h"
 #include "core/frame/FrameView.h"
 #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"
 #include "core/page/Chrome.h"
 #include "core/page/EventHandler.h"
 #include "core/page/FocusController.h"
 #include "core/page/Page.h"
 #include "core/page/scrolling/ScrollingCoordinator.h"
 #include "core/rendering/HitTestResult.h"
 #include "core/rendering/RenderLayer.h"
@@ skipping 77 matching lines @@
         (*it)->frameDestroyed();
 #endif
 }
 
 void LocalFrame::trace(Visitor* visitor)
 {
 #if ENABLE(OILPAN)
     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);
     visitor->trace(m_spellChecker);
     visitor->trace(m_selection);
     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);
 }
 
 void LocalFrame::detach()
 {
     // A lot of the following steps can result in the current frame being
     // detached, so protect a reference to it.
     RefPtrWillBeRawPtr<LocalFrame> protect(this);
@@ skipping 24 matching lines @@
 {
     // We detach the FrameView's custom scroll bars as early as
     // possible to prevent m_doc->detach() from messing with the view
     // such that its scroll bars won't be torn down.
     //
     // FIXME: We should revisit this.
     if (m_view)
         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
     // notified. If we wait until the view is destroyed, then things won't be hooked up enough for
     // these calls to work.
     if (!view && document() && document()->isActive()) {
         // FIXME: We don't call willRemove here. Why is that OK?
         document()->prepareForDestruction();
     }
 
@@ skipping 224 matching lines @@
     ASSERT(this);
     ASSERT(page());
 
     bool isLocalRoot = this->isLocalRoot();
 
     if (isLocalRoot && view())
         view()->setParentVisible(false);
 
     setView(nullptr);
 
-    RefPtr<FrameView> frameView;
+    RefPtrWillBeRawPtr<FrameView> frameView = nullptr;
     if (isLocalRoot) {
         frameView = FrameView::create(this, viewportSize);
 
         // The layout size is set by WebViewImpl to support @viewport
         frameView->setLayoutSizeFixedToFrameSize(false);
     } else
         frameView = FrameView::create(this);
 
     frameView->setScrollbarModes(horizontalScrollbarMode, verticalScrollbarMode, horizontalLock, verticalLock);
 
@@ skipping 254 matching lines @@
     ratio *= pageZoomFactor();
     return ratio;
 }
 
 void LocalFrame::disconnectOwnerElement()
 {
     if (owner()) {
         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.
         setView(nullptr);
         loader().clear();
 #endif
     }
     Frame::disconnectOwnerElement();
 }
 
 LocalFrame* LocalFrame::localFrameRoot()
 {
     LocalFrame* curFrame = this;
     while (curFrame && curFrame->tree().parent() && curFrame->tree().parent()->isLocalFrame())
         curFrame = toLocalFrame(curFrame->tree().parent());
 
     return curFrame;
 }
 
 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