Move Frame 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 94a5d12e52729f9b52217b2c695faf5249daa7ce..c6611df7d1750f264b88b9ef5bd2f2c4d05b4fa5 100644
--- a/Source/core/frame/LocalFrame.cpp
+++ b/Source/core/frame/LocalFrame.cpp
@@ -106,30 +106,57 @@ inline LocalFrame::LocalFrame(FrameLoaderClient* client, FrameHost* host, FrameO
 {
 }
 
-PassRefPtr<LocalFrame> LocalFrame::create(FrameLoaderClient* client, FrameHost* host, FrameOwner* owner)
+PassRefPtrWillBeRawPtr<LocalFrame> LocalFrame::create(FrameLoaderClient* client, FrameHost* host, FrameOwner* owner)
 {
-    RefPtr<LocalFrame> frame = adoptRef(new LocalFrame(client, host, owner));
+    RefPtrWillBeRawPtr<LocalFrame> frame = adoptRefWillBeNoop(new LocalFrame(client, host, owner));
     InspectorInstrumentation::frameAttachedToParent(frame.get());
     return frame.release();
 }
 
 LocalFrame::~LocalFrame()
 {
-    setView(nullptr);
-    loader().clear();
-    setDOMWindow(nullptr);
+    dispose();

[haraken, 2014/09/08 07:25:58]

It looks unsafe to call dispose() methods in LocalFrame's destructor.

Probably can we introduce:

  PersistentHeapHashSet<WeakMember<LocalFrame>, OwnPtr<LocalFrameDisposer>>
m_frames;

and call the dispose() method in the destructor of LocalFrameDisposer?

[sof, 2014/09/08 21:17:46]

Why do you consider it unsafe? loader() (FrameLoader) is a part object of the
LocalFrame.

+}
+
+void LocalFrame::trace(Visitor* visitor)
+{
+#if ENABLE(OILPAN)
+    visitor->trace(m_destructionObservers);
+#endif
+    visitor->trace(m_loader);
+    visitor->trace(m_navigationScheduler);
+    visitor->trace(m_pagePopupOwner);
+    visitor->trace(m_editor);
+    visitor->trace(m_spellChecker);
+    visitor->trace(m_selection);
+    visitor->trace(m_eventHandler);
+    visitor->trace(m_console);
+    visitor->trace(m_inputMethodController);
+    Frame::trace(visitor);
+    WillBeHeapSupplementable<LocalFrame>::trace(visitor);
+}
 
-    // FIXME: What to do here... some of this is redundant with ~Frame.
-    HashSet<FrameDestructionObserver*>::iterator stop = m_destructionObservers.end();
-    for (HashSet<FrameDestructionObserver*>::iterator it = m_destructionObservers.begin(); it != stop; ++it)
+void LocalFrame::dispose()
+{
+#if !ENABLE(OILPAN)
+    setView(nullptr);

[dcheng, 2014/09/07 22:26:34]

Similarly, is it safe to omit this call to setView(nullptr)? This does do a
non-trivial amount of work (in particular, it will detach an active document).

[sof, 2014/09/16 09:08:35]

I now believe it is safe, except that it would be tidier to detach the FrameView
for the Oilpan code path. Updated it do so, along with adding (yet) more
comments.

+    loader().dispose(true);
+#else
+    loader().dispose(false);

[dcheng, 2014/09/07 22:26:34]

Why do we pass different bool values, depending on if Oilpan is enabled? 

[sof, 2014/09/08 21:17:45]

As dispose() is called from the dtor, we have to be careful not to touch other
heap objects when finalizing here (as they might have already been finalized.)
Those objects and their finalizers are instead responsible for carrying out the
needed actions that the FrameLoader::dispose() cannot perform.

For the Frame-accessible Document, as mentioned in the dispose() FIXME and
elsewhere, this needs to be considered carefully again - there's considerable
amount of code that's indirectly triggered here. I don't consider myself done
here -- expanded the FrameLoader::dispose() FIXME just to make that clear.

+#endif
+
+#if !ENABLE(OILPAN)
+    HashSet<RawPtr<FrameDestructionObserver> >::iterator stop = m_destructionObservers.end();
+    for (HashSet<RawPtr<FrameDestructionObserver> >::iterator it = m_destructionObservers.begin(); it != stop; ++it)
         (*it)->frameDestroyed();
+#endif
 }
 
 void LocalFrame::detach()
 {
     // A lot of the following steps can result in the current frame being
     // detached, so protect a reference to it.
-    RefPtr<LocalFrame> protect(this);
+    RefPtrWillBeRawPtr<LocalFrame> protect(this);
     m_loader.stopAllLoaders();
     m_loader.closeURL();
     detachChildren();
@@ -199,7 +226,7 @@ void LocalFrame::setPrinting(bool printing, const FloatSize& pageSize, const Flo
     }
 
     // Subframes of the one we're printing don't lay out to the page size.
-    for (RefPtr<Frame> child = tree().firstChild(); child; child = child->tree().nextSibling()) {
+    for (RefPtrWillBeRawPtr<Frame> child = tree().firstChild(); child; child = child->tree().nextSibling()) {
         if (child->isLocalFrame())
             toLocalFrame(child.get())->setPrinting(printing, FloatSize(), FloatSize(), 0);
     }
@@ -248,7 +275,7 @@ void LocalFrame::didChangeVisibilityState()
     if (document())
         document()->didChangeVisibilityState();
 
-    Vector<RefPtr<LocalFrame> > childFrames;
+    WillBeHeapVector<RefPtrWillBeMember<LocalFrame> > childFrames;
     for (Frame* child = tree().firstChild(); child; child = child->tree().nextSibling()) {
         if (child->isLocalFrame())
             childFrames.append(toLocalFrame(child));
@@ -277,10 +304,14 @@ void LocalFrame::willDetachFrameHost()
     if (parent && parent->isLocalFrame())
         toLocalFrame(parent)->loader().checkLoadComplete();
 
-    HashSet<FrameDestructionObserver*>::iterator stop = m_destructionObservers.end();
-    for (HashSet<FrameDestructionObserver*>::iterator it = m_destructionObservers.begin(); it != stop; ++it)
+    WillBeHeapHashSet<RawPtrWillBeWeakMember<FrameDestructionObserver> >::iterator stop = m_destructionObservers.end();
+    for (WillBeHeapHashSet<RawPtrWillBeWeakMember<FrameDestructionObserver> >::iterator it = m_destructionObservers.begin(); it != stop; ++it)
         (*it)->willDetachFrameHost();
 
+#if ENABLE(OILPAN)
+    m_destructionObservers.clear();

[haraken, 2014/09/08 07:25:58]

This is just for performance optimization and not needed for correctness, right?

[sof, 2014/09/08 21:17:46]

Yes, I just thought it appropriate to release the traced references at the point
we're through with them, but of no real importance. I won't insist we keep it =>
removed.

+#endif
+
     // FIXME: Page should take care of updating focus/scrolling instead of Frame.
     // FIXME: It's unclear as to why this is called more than once, but it is,
     // so page() could be null.
@@ -294,7 +325,7 @@ void LocalFrame::willDetachFrameHost()
 
 void LocalFrame::detachFromFrameHost()
 {
-    // We should never be detatching the page during a Layout.
+    // We should never be detaching the page during a Layout.
     RELEASE_ASSERT(!m_view || !m_view->isInPerformLayout());
     m_host = 0;
 }
@@ -513,7 +544,7 @@ void LocalFrame::setPageAndTextZoomFactors(float pageZoomFactor, float textZoomF
     m_pageZoomFactor = pageZoomFactor;
     m_textZoomFactor = textZoomFactor;
 
-    for (RefPtr<Frame> child = tree().firstChild(); child; child = child->tree().nextSibling()) {
+    for (RefPtrWillBeRawPtr<Frame> child = tree().firstChild(); child; child = child->tree().nextSibling()) {
         if (child->isLocalFrame())
             toLocalFrame(child.get())->setPageAndTextZoomFactors(m_pageZoomFactor, m_textZoomFactor);
     }
@@ -525,7 +556,7 @@ void LocalFrame::setPageAndTextZoomFactors(float pageZoomFactor, float textZoomF
 void LocalFrame::deviceOrPageScaleFactorChanged()
 {
     document()->mediaQueryAffectingValueChanged();
-    for (RefPtr<Frame> child = tree().firstChild(); child; child = child->tree().nextSibling()) {
+    for (RefPtrWillBeRawPtr<Frame> child = tree().firstChild(); child; child = child->tree().nextSibling()) {
         if (child->isLocalFrame())
             toLocalFrame(child.get())->deviceOrPageScaleFactorChanged();
     }