Move Frame 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 88 matching lines @@
     , m_selection(FrameSelection::create(this))
     , m_eventHandler(adoptPtrWillBeNoop(new EventHandler(this)))
     , m_console(FrameConsole::create(*this))
     , m_inputMethodController(InputMethodController::create(*this))
     , m_pageZoomFactor(parentPageZoomFactor(this))
     , m_textZoomFactor(parentTextZoomFactor(this))
     , m_inViewSourceMode(false)
 {
 }
 
-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()
 {
+#if ENABLE(OILPAN)
+    // Verify that the FrameView and FrameLoader have instead been
+    // cleared as part of detaching the frame owner.
+    ASSERT(!m_owner);
+    ASSERT(!m_view);
+    // Oilpan: see setDOMWindow() comment why it is acceptable not to
+    // mirror the non-Oilpan call below.
+    //
+    // Also, FrameDestructionObservers that live longer than this
+    // frame object keep weak references to the frame; those will be
+    // automatically cleared by the garbage collector. Hence, explicit
+    // frameDestroyed() notifications aren't needed.
+#else
+    // FIXME: follow Oilpan and clear the FrameView and FrameLoader
+    // during FrameOwner detachment instead, see LocalFrame::disconnectOwnerElement().
     setView(nullptr);
-    loader().clear();
+    m_loader.clear();
     setDOMWindow(nullptr);
 
-    // 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)
+    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::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);
 }
 
 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();
     // stopAllLoaders() needs to be called after detachChildren(), because detachChildren()
     // will trigger the unload event handlers of any child frames, and those event
     // handlers might start a new subresource load in this frame.
     m_loader.stopAllLoaders();
     m_loader.detachFromParent();
 }
 
 bool LocalFrame::inScope(TreeScope* scope) const
 {
     ASSERT(scope);
     Document* doc = document();
     if (!doc)
         return false;
     // FIXME: This check is broken in for OOPI.
     HTMLFrameOwnerElement* owner = doc->ownerElement();
     if (!owner)
         return false;
     return owner->treeScope() == scope;
 }
 
+void LocalFrame::detachView()
+{
+    // 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)
 {
-    // We the 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();
+    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();
     }
 
     eventHandler().clear();
@@ skipping 19 matching lines @@
 
     document()->styleResolverChanged();
     if (shouldUsePrintingLayout()) {
         view()->forceLayoutForPagination(pageSize, originalPageSize, maximumShrinkRatio);
     } else {
         view()->forceLayout();
         view()->adjustViewSize();
     }
 
     // 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);
     }
 }
 
 bool LocalFrame::shouldUsePrintingLayout() const
 {
     // Only top frame being printed should be fit to page size.
     // Subframes should be constrained by parents only.
     return document()->printing() && (!tree().parent() || !tree().parent()->isLocalFrame() || !toLocalFrame(tree().parent())->document()->printing());
@@ skipping 15 matching lines @@
         float ratio = originalSize.width() / originalSize.height();
         resultSize.setHeight(floorf(expectedSize.height()));
         resultSize.setWidth(floorf(resultSize.height() * ratio));
     }
     return resultSize;
 }
 
 void LocalFrame::setDOMWindow(PassRefPtrWillBeRawPtr<LocalDOMWindow> domWindow)
 {
     if (m_domWindow) {
+        // Oilpan: the assumption is that FrameDestructionObserver::willDetachFrameHost()
+        // on LocalWindow will have signalled these frameWindowDiscarded() notifications.
+        //
+        // It is not invoked when finalizing the LocalFrame, as setDOMWindow() isn't
+        // performed (accessing the m_domWindow heap object is unsafe then.)

[haraken, 2014/09/22 05:35:23]

This comment now looks a bit strange. Now that setDOMWindow() is not called in
any destructor, can we probably just drop this comment?

[sof, 2014/09/22 09:52:51]

I think it makes some sense to document the reason why it is acceptable to leave
out those setDOMWindow() calls during Oilpan finalization. And having it here
next to the details, rather than at the call site(s), seems also reasonable.

I've refreshed and updated the comment a bit.

         console().messageStorage()->frameWindowDiscarded(m_domWindow.get());
         InspectorInstrumentation::frameWindowDiscarded(this, m_domWindow.get());
     }
     if (domWindow)
         script().clearWindowProxy();
     Frame::setDOMWindow(domWindow);
 }
 
 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));
     }
 
     for (size_t i = 0; i < childFrames.size(); ++i)
         childFrames[i]->didChangeVisibilityState();
 }
 
 void LocalFrame::addDestructionObserver(FrameDestructionObserver* observer)
 {
     m_destructionObservers.add(observer);
 }
 
 void LocalFrame::removeDestructionObserver(FrameDestructionObserver* observer)
 {
     m_destructionObservers.remove(observer);
 }
 
 void LocalFrame::willDetachFrameHost()
 {
     // We should never be detatching the page during a Layout.
     RELEASE_ASSERT(!m_view || !m_view->isInPerformLayout());
 
     Frame* parent = tree().parent();
     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();
 
     // 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.
     if (page() && page()->focusController().focusedFrame() == this)
         page()->focusController().setFocusedFrame(nullptr);
     script().clearScriptObjects();
 
     if (page() && page()->scrollingCoordinator() && m_view)
         page()->scrollingCoordinator()->willDestroyScrollableArea(m_view.get());
 }
 
 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;
+    m_host = nullptr;
 }
 
 String LocalFrame::documentTypeString() const
 {
     if (DocumentType* doctype = document()->doctype())
         return createMarkup(doctype);
 
     return String();
 }
 
@@ skipping 196 matching lines @@
             // Update the scroll position when doing a full page zoom, so the content stays in relatively the same position.
             LayoutPoint scrollPosition = view->scrollPosition();
             float percentDifference = (pageZoomFactor / m_pageZoomFactor);
             view->setScrollPosition(IntPoint(scrollPosition.x() * percentDifference, scrollPosition.y() * percentDifference));
         }
     }
 
     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);
     }
 
     document->setNeedsStyleRecalc(SubtreeStyleChange);
     document->updateLayoutIgnorePendingStylesheets();
 }
 
 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();
     }
 }
 
 bool LocalFrame::isURLAllowed(const KURL& url) const
 {
     // We allow one level of self-reference because some sites depend on that,
     // but we don't allow more than one.
     if (page()->subframeCount() >= Page::maxNumberOfFrames)
@@ skipping 119 matching lines @@
         return 0;
 
     double ratio = m_host->deviceScaleFactor();
     ratio *= pageZoomFactor();
     return ratio;
 }
 
 void LocalFrame::disconnectOwnerElement()
 {
     if (owner()) {
-        if (Document* doc = document())
-            doc->topDocument().clearAXObjectCache();
+        if (Document* document = this->document())
+            document->topDocument().clearAXObjectCache();
+#if ENABLE(OILPAN)
+        // 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();

[haraken, 2014/09/22 05:35:23]

Don't we need to call setDOMWindow(nullptr) here?

[sof, 2014/09/22 09:52:51]

That's a bit too strong, as you then will observably have detached frames with
frame->domWindow() being 0. This breaks various detached tests.

[haraken, 2014/09/22 10:07:47]

Makes sense. I now understand that we should let the weak processing clear the
DOMWindow rather than explicitly calling setDOMWindow(nullptr).

+#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;
 }
 
 } // namespace blink