Move Frame to the Oilpan heap.

Source/core/loader/FrameLoader.cpp

Index: Source/core/loader/FrameLoader.cpp
diff --git a/Source/core/loader/FrameLoader.cpp b/Source/core/loader/FrameLoader.cpp
index bf7bcdd39de5b2398d7cec175fec149a7d187f50..7a65b106cac3ced2f40e8e627fb2a421bc7c8705 100644
--- a/Source/core/loader/FrameLoader.cpp
+++ b/Source/core/loader/FrameLoader.cpp
@@ -53,6 +53,7 @@
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/PinchViewport.h"
+#include "core/frame/Settings.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
 #include "core/html/HTMLFormElement.h"
 #include "core/html/HTMLFrameOwnerElement.h"
@@ -77,7 +78,6 @@
 #include "core/page/EventHandler.h"
 #include "core/page/FrameTree.h"
 #include "core/page/Page.h"
-#include "core/frame/Settings.h"
 #include "core/page/WindowFeatures.h"
 #include "core/page/scrolling/ScrollingCoordinator.h"
 #include "core/xml/parser/XMLDocumentParser.h"
@@ -130,6 +130,12 @@ FrameLoader::~FrameLoader()
 {
 }
 
+void FrameLoader::trace(Visitor* visitor)
+{
+    visitor->trace(m_frame);
+    visitor->trace(m_fetchContext);
+}
+
 void FrameLoader::init()
 {
     ResourceRequest initialRequest(KURL(ParsedURLString, emptyString()));
@@ -240,23 +246,34 @@ void FrameLoader::didExplicitOpen()
     m_frame->navigationScheduler().cancel();
 }
 
-void FrameLoader::clear()
+void FrameLoader::dispose(bool clearFrameContents)

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

Nit: don't use a bool parameter here. Use an enum.

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

clearFrameContents => isDuringFrameDestruction ?

enum is better either way :)

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

Done.

 {
+    // dispose() is called during (Local)Frame finalization and when creating
+    // a new Document within it (DocumentLoader::createWriterFor().)
     if (m_stateMachine.creatingInitialEmptyDocument())
         return;
 
-    m_frame->editor().clear();
-    m_frame->document()->cancelParsing();
-    m_frame->document()->prepareForDestruction();
-    m_frame->document()->removeFocusedElementOfSubtree(m_frame->document());
+    if (clearFrameContents) {
+        m_frame->editor().clear();
+        m_frame->document()->cancelParsing();
+        m_frame->document()->prepareForDestruction();
+        m_frame->document()->removeFocusedElementOfSubtree(m_frame->document());
+        // FIXME: Oilpan: the RenderView will not have its selection
+        // cleared when the frame is finalized. Verify that this
+        // is of no particular importance.
+        m_frame->selection().prepareForDestruction();
+        m_frame->eventHandler().clear();
+    }
 
-    m_frame->selection().prepareForDestruction();
-    m_frame->eventHandler().clear();
-    if (m_frame->view())
-        m_frame->view()->clear();
+    if (FrameView* view = m_frame->view())
+        view->clear();
 
     m_frame->script().enableEval();

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

Is it to safe to touch m_frame here if the dispose() is called in LocalFrame's
destructor?

 
+    // Oilpan: this depends on NavigationScheduler being a part object
+    // of FrameLoader, i.e., the part object is still accessible.
+    //
+    // FIXME: Oilpan: verify this assumption.
     m_frame->navigationScheduler().cancel();

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

Ditto. It looks not safe to touch m_frame here.

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

See the FIXME above? :) If NavigationScheduler is a part object of the
FrameLoader (which again is a part object of the LocalFrame), how can m_frame's
allocation have been reclaimed at this stage?

Arguably unsavoury, but is it unsafe?

 
     m_checkTimer.stop();
@@ -285,9 +302,9 @@ void FrameLoader::replaceDocumentWhileExecutingJavaScriptURL(const String& sourc
     init.withNewRegistrationContext();
 
     stopAllLoaders();
-    clear();
+    dispose(true);
 
-    // clear() potentially detaches the frame from the document. The
+    // dispose() potentially detaches the frame from the document. The
     // loading cannot continue in that case.
     if (!m_frame->page())
         return;
@@ -357,7 +374,7 @@ void FrameLoader::receivedFirstData()
 static void didFailContentSecurityPolicyCheck(FrameLoader* loader)
 {
     // load event and stopAllLoaders can detach the LocalFrame, so protect it.
-    RefPtr<LocalFrame> frame(loader->frame());
+    RefPtrWillBeRawPtr<LocalFrame> frame = loader->frame();
 
     // Move the page to a unique origin, and cancel the load.
     frame->document()->enforceSandboxFlags(SandboxOrigin);
@@ -420,7 +437,7 @@ void FrameLoader::finishedParsing()
     // This can be called from the LocalFrame's destructor, in which case we shouldn't protect ourselves
     // because doing so will cause us to re-enter the destructor when protector goes out of scope.
     // Null-checking the FrameView indicates whether or not we're in the destructor.
-    RefPtr<LocalFrame> protector = m_frame->view() ? m_frame : 0;
+    RefPtrWillBeRawPtr<LocalFrame> protect(m_frame->view() ? m_frame.get() : nullptr);
 
     if (client())
         client()->dispatchDidFinishDocumentLoad();
@@ -464,7 +481,7 @@ bool FrameLoader::allAncestorsAreComplete() const
 
 void FrameLoader::checkCompleted()
 {
-    RefPtr<LocalFrame> protect(m_frame);
+    RefPtrWillBeRawPtr<LocalFrame> protect(m_frame.get());
 
     if (m_frame->view())
         m_frame->view()->handleLoadCompleted();
@@ -606,7 +623,7 @@ void FrameLoader::loadInSameDocument(const KURL& url, PassRefPtr<SerializedScrip
 
 void FrameLoader::completed()
 {
-    RefPtr<LocalFrame> protect(m_frame);
+    RefPtrWillBeRawPtr<LocalFrame> protect(m_frame.get());
 
     for (Frame* descendant = m_frame->tree().traverseNext(m_frame); descendant; descendant = descendant->tree().traverseNext(m_frame)) {
         if (descendant->isLocalFrame())
@@ -730,7 +747,7 @@ void FrameLoader::load(const FrameLoadRequest& passedRequest)
 {
     ASSERT(m_frame->document());
 
-    RefPtr<LocalFrame> protect(m_frame);
+    RefPtrWillBeRawPtr<LocalFrame> protect(m_frame.get());
 
     if (m_inStopAllLoaders)
         return;
@@ -739,8 +756,8 @@ void FrameLoader::load(const FrameLoadRequest& passedRequest)
     if (!prepareRequestForThisFrame(request))
         return;
 
-    RefPtr<LocalFrame> targetFrame = request.formState() ? 0 : findFrameForNavigation(AtomicString(request.frameName()), request.formState() ? request.formState()->sourceDocument() : m_frame->document());
-    if (targetFrame && targetFrame != m_frame) {
+    RefPtrWillBeRawPtr<LocalFrame> targetFrame = request.formState() ? 0 : findFrameForNavigation(AtomicString(request.frameName()), request.formState() ? request.formState()->sourceDocument() : m_frame->document());
+    if (targetFrame && targetFrame.get() != m_frame) {
         request.setFrameName("_self");
         targetFrame->loader().load(request);
         if (Page* page = targetFrame->page())
@@ -852,11 +869,11 @@ void FrameLoader::stopAllLoaders()
 
     // Calling stopLoading() on the provisional document loader can blow away
     // the frame from underneath.
-    RefPtr<LocalFrame> protect(m_frame);
+    RefPtrWillBeRawPtr<LocalFrame> protect(m_frame.get());
 
     m_inStopAllLoaders = true;
 
-    for (RefPtr<Frame> child = m_frame->tree().firstChild(); child; child = child->tree().nextSibling()) {
+    for (RefPtrWillBeRawPtr<Frame> child = m_frame->tree().firstChild(); child; child = child->tree().nextSibling()) {
         if (child->isLocalFrame())
             toLocalFrame(child.get())->loader().stopAllLoaders();
     }
@@ -891,7 +908,8 @@ void FrameLoader::didAccessInitialDocument()
 
 void FrameLoader::didAccessInitialDocumentTimerFired(Timer<FrameLoader>*)
 {
-    client()->didAccessInitialDocument();
+    if (client())

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

Why was this check not needed before this CL but is needed after this CL?

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

The FrameClient is cleared upon calling close() on the Frame, but downstream
uses of the Frame aren't cleared as finalization is delayed. Hence we can end up
calling FrameLoader methods where the client isn't available, like here.

(I don't have the failing testcase in front of me.)

+        client()->didAccessInitialDocument();
 }
 
 void FrameLoader::notifyIfInitialDocumentAccessed()
@@ -914,7 +932,7 @@ void FrameLoader::commitProvisionalLoad()
     ASSERT(client()->hasWebView());
     ASSERT(m_state == FrameStateProvisional);
     RefPtr<DocumentLoader> pdl = m_provisionalDocumentLoader;
-    RefPtr<LocalFrame> protect(m_frame);
+    RefPtrWillBeRawPtr<LocalFrame> protect(m_frame.get());
 
     // Check if the destination page is allowed to access the previous page's timing information.
     if (m_frame->document()) {
@@ -989,10 +1007,10 @@ static bool isDocumentDoneLoading(Document* document)
 bool FrameLoader::checkLoadCompleteForThisFrame()
 {
     ASSERT(client()->hasWebView());
-    RefPtr<LocalFrame> protect(m_frame);
+    RefPtrWillBeRawPtr<LocalFrame> protect(m_frame.get());
 
     bool allChildrenAreDoneLoading = true;
-    for (RefPtr<Frame> child = m_frame->tree().firstChild(); child; child = child->tree().nextSibling()) {
+    for (RefPtrWillBeRawPtr<Frame> child = m_frame->tree().firstChild(); child; child = child->tree().nextSibling()) {
         if (child->isLocalFrame())
             allChildrenAreDoneLoading &= toLocalFrame(child.get())->loader().checkLoadCompleteForThisFrame();
     }
@@ -1120,7 +1138,9 @@ String FrameLoader::userAgent(const KURL& url) const
 void FrameLoader::detachFromParent()
 {
     // The caller must protect a reference to m_frame.
+#if !ENABLE(OILPAN)
     ASSERT(m_frame->refCount() > 1);
+#endif
 
     InspectorInstrumentation::frameDetachedFromParent(m_frame);
 
@@ -1174,7 +1194,7 @@ void FrameLoader::detachClient()
 void FrameLoader::receivedMainResourceError(const ResourceError& error)
 {
     // Retain because the stop may release the last reference to it.
-    RefPtr<LocalFrame> protect(m_frame);
+    RefPtrWillBeRawPtr<LocalFrame> protect(m_frame.get());
 
     if (m_frame->document()->parser())
         m_frame->document()->parser()->stopParsing();
@@ -1216,7 +1236,7 @@ void FrameLoader::scrollToFragmentWithParentBoundary(const KURL& url)
         return;
 
     // Leaking scroll position to a cross-origin ancestor would permit the so-called "framesniffing" attack.
-    RefPtr<LocalFrame> boundaryFrame(url.hasFragmentIdentifier() ? m_frame->document()->findUnsafeParentScrollPropagationBoundary() : 0);
+    RefPtrWillBeRawPtr<LocalFrame> boundaryFrame = url.hasFragmentIdentifier() ? m_frame->document()->findUnsafeParentScrollPropagationBoundary() : 0;
 
     if (boundaryFrame)
         boundaryFrame->view()->setSafeToPropagateScrollToParent(false);
@@ -1234,7 +1254,7 @@ bool FrameLoader::shouldClose()
         return true;
 
     // Store all references to each subframe in advance since beforeunload's event handler may modify frame
-    Vector<RefPtr<LocalFrame> > targetFrames;
+    WillBeHeapVector<RefPtrWillBeMember<LocalFrame> > targetFrames;
     targetFrames.append(m_frame);
     for (Frame* child = m_frame->tree().firstChild(); child; child = child->tree().traverseNext(m_frame)) {
         // FIXME: There is not yet any way to dispatch events to out-of-process frames.
@@ -1321,7 +1341,7 @@ void FrameLoader::loadWithNavigationAction(const NavigationAction& action, Frame
         isTransitionNavigation = dispatchNavigationTransitionData();
 
     // stopAllLoaders can detach the LocalFrame, so protect it.
-    RefPtr<LocalFrame> protect(m_frame);
+    RefPtrWillBeRawPtr<LocalFrame> protect(m_frame.get());

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

.get() is needed? The same comment for other parts in this file.

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

Ambiguity results, if not: Member<LocalFrame> can either be implicitly converted
into LocalFrame* or (const RawPtr<LocalFrame>), for which RawPtr<>() has
constructor overloads for.

     if ((!m_policyDocumentLoader->shouldContinueForNavigationPolicy(request, shouldCheckMainWorldContentSecurityPolicy, isTransitionNavigation) || !shouldClose()) && m_policyDocumentLoader) {
         m_policyDocumentLoader->detachFromFrame();
         m_policyDocumentLoader = nullptr;
@@ -1442,7 +1462,7 @@ LocalFrame* FrameLoader::findFrameForNavigation(const AtomicString& name, Docume
 
 void FrameLoader::loadHistoryItem(HistoryItem* item, HistoryLoadType historyLoadType, ResourceRequestCachePolicy cachePolicy)
 {
-    RefPtr<LocalFrame> protect(m_frame);
+    RefPtrWillBeRawPtr<LocalFrame> protect(m_frame.get());
     if (m_frame->page()->defersLoading()) {
         m_deferredHistoryLoad = DeferredHistoryLoad(item, historyLoadType, cachePolicy);
         return;