Revert of Streamline frame detach (patchset #8 id:140001 of https://c…

…odereview.chromium.org/551973005/) Reason for revert: This patch breaks SitePerProcessBrowserTest.NavigateRemoteFrame on swarming bots: http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_swarming/builds/21481 Original issue's description: > Streamline frame detach > > Share a bit more code between LocalFrame and RemoteFrame, including moving the detach-time client notification down to FrameClient. > > Move the max frame counter to FrameHost instead of Page. > > Move detach logic from FrameLoader to LocalFrame, unless it manipulates FrameLoader members. > > BUG= > > Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182999 [email protected],[email protected],[email protected] NOTREECHECKS=true NOTRY=true BUG= Review URL: https://codereview.chromium.org/618253002 git-svn-id: svn://svn.chromium.org/blink/trunk@183040 bbb929c8-8fbe-4397-9dbb-9b2b20218538
yoavweiss · Oct 1, 2014 · 6ad9c24 · 6ad9c24
1 parent 6f5897c
commit 6ad9c24
Show file tree

Hide file tree

Showing 20 changed files with 111 additions and 83 deletions.
diff --git a/Source/core/dom/NodeRareData.cpp b/Source/core/dom/NodeRareData.cpp
@@ -32,7 +32,7 @@
 #include "core/dom/NodeRareData.h"
 #include "core/dom/Element.h"
 #include "core/dom/ElementRareData.h"
-#include "core/frame/FrameHost.h"
+#include "core/page/Page.h"
 #include "core/rendering/RenderObject.h"
 #include "platform/heap/Handle.h"
 
@@ -77,6 +77,6 @@ void NodeRareData::finalizeGarbageCollectedObject()
 }
 
 // Ensure the 10 bits reserved for the m_connectedFrameCount cannot overflow
-static_assert(FrameHost::maxNumberOfFrames < (1 << NodeRareData::ConnectedFrameCountBits), "Frame limit should fit in rare data count");
+COMPILE_ASSERT(Page::maxNumberOfFrames < (1 << NodeRareData::ConnectedFrameCountBits), Frame_limit_should_fit_in_rare_data_count);
 
 } // namespace blink
diff --git a/Source/core/frame/Frame.cpp b/Source/core/frame/Frame.cpp
@@ -70,9 +70,8 @@ Frame::Frame(FrameClient* client, FrameHost* host, FrameOwner* owner)
     frameCounter.increment();
 #endif
 
-    m_host->incrementFrameCount();
-
     if (m_owner) {
+        page()->incrementSubframeCount();
         if (m_owner->isLocal())
             toHTMLFrameOwnerElement(m_owner)->setContentFrame(*this);
     } else {
@@ -103,24 +102,6 @@ void Frame::trace(Visitor* visitor)
     visitor->trace(m_domWindow);
 }
 
-void Frame::detach()
-{
-    // client() should never be null because that means we somehow re-entered
-    // the frame detach code... but it is sometimes.
-    // FIXME: Understand why this is happening so we can document this insanity.
-    // http://crbug.com/371084 is a probable explanation.
-    if (!client())
-        return;
-    // FIXME: Should we enforce the invariant that all pointers nulled in this function
-    // get nulled at the same time?
-    m_host->decrementFrameCount();
-    m_host = nullptr;
-    // After this, we must no longer talk to the client since this clears
-    // its owning reference back to our owning LocalFrame.
-    m_client->detached();
-    m_client = nullptr;
-}
-
 void Frame::detachChildren()
 {
     typedef WillBeHeapVector<RefPtrWillBeMember<Frame> > FrameVector;
@@ -225,6 +206,8 @@ void Frame::disconnectOwnerElement()
     if (m_owner) {
         if (m_owner->isLocal())
             toHTMLFrameOwnerElement(m_owner)->clearContentFrame();
+        if (page())
+            page()->decrementSubframeCount();
     }
     m_owner = nullptr;
 }

diff --git a/Source/core/frame/Frame.h b/Source/core/frame/Frame.h
@@ -59,10 +59,11 @@ class Frame : public RefCountedWillBeGarbageCollectedFinalized<Frame> {
     virtual void trace(Visitor*);
 
     virtual void navigate(Document& originDocument, const KURL&, const Referrer&, bool lockBackForwardList) = 0;
-    virtual void detach();
+    virtual void detach() = 0;
     void detachChildren();
 
     FrameClient* client() const;
+    void clearClient();
 
     // NOTE: Page is moving out of Blink up into the browser process as
     // part of the site-isolation (out of process iframes) work.
@@ -120,6 +121,11 @@ inline FrameClient* Frame::client() const
     return m_client;
 }
 
+inline void Frame::clearClient()
+{
+    m_client = 0;
+}
+
 inline LocalDOMWindow* Frame::domWindow() const
 {
     return m_domWindow.get();

diff --git a/Source/core/frame/FrameClient.h b/Source/core/frame/FrameClient.h
@@ -14,8 +14,6 @@ class SecurityOrigin;
 
 class FrameClient {
 public:
-    virtual void detached() = 0;
-
     virtual Frame* opener() const = 0;
     virtual void setOpener(Frame*) = 0;
 

diff --git a/Source/core/frame/FrameHost.cpp b/Source/core/frame/FrameHost.cpp
@@ -47,7 +47,6 @@ FrameHost::FrameHost(Page& page)
     : m_page(&page)
     , m_pinchViewport(adoptPtr(new PinchViewport(*this)))
     , m_eventHandlerRegistry(adoptPtrWillBeNoop(new EventHandlerRegistry(*this)))
-    , m_frameCount(0)
 {
 }
 
@@ -92,25 +91,4 @@ void FrameHost::trace(Visitor* visitor)
     visitor->trace(m_eventHandlerRegistry);
 }
 
-#if ENABLE(ASSERT)
-void checkFrameCountConsistency(int expectedFrameCount, Frame* frame)
-{
-    ASSERT(expectedFrameCount >= 0);
-
-    int actualFrameCount = 0;
-    for (; frame; frame = frame->tree().traverseNext())
-        ++actualFrameCount;
-
-    ASSERT(expectedFrameCount == actualFrameCount);
-}
-#endif
-
-int FrameHost::frameCount() const
-{
-#if ENABLE(ASSERT)
-    checkFrameCountConsistency(m_frameCount, m_page->mainFrame());
-#endif
-    return m_frameCount;
-}
-
 }
diff --git a/Source/core/frame/FrameHost.h b/Source/core/frame/FrameHost.h
@@ -82,15 +82,6 @@ class FrameHost FINAL : public NoBaseWillBeGarbageCollectedFinalized<FrameHost>
 
     void trace(Visitor*);
 
-    // Don't allow more than a certain number of frames in a page.
-    // This seems like a reasonable upper bound, and otherwise mutually
-    // recursive frameset pages can quickly bring the program to its knees
-    // with exponential growth in the number of frames.
-    static const int maxNumberOfFrames = 1000;
-    void incrementFrameCount() { ++m_frameCount; }
-    void decrementFrameCount() { ASSERT(m_frameCount); --m_frameCount; }
-    int frameCount() const;
-
 private:
     explicit FrameHost(Page&);
 
@@ -99,7 +90,6 @@ class FrameHost FINAL : public NoBaseWillBeGarbageCollectedFinalized<FrameHost>
     const OwnPtrWillBeMember<EventHandlerRegistry> m_eventHandlerRegistry;
 
     AtomicString m_overrideEncoding;
-    int m_frameCount;
 };
 
 }

diff --git a/Source/core/frame/LocalFrame.cpp b/Source/core/frame/LocalFrame.cpp
@@ -174,16 +174,7 @@ void LocalFrame::detach()
     // 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();
-    if (!client())
-        return;
-    m_loader.detach();
-    setView(nullptr);
-    willDetachFrameHost();
-    // Notify ScriptController that the frame is closing, since its cleanup ends up calling
-    // back to FrameLoaderClient via WindowProxy.
-    script().clearForClose();
-    InspectorInstrumentation::frameDetachedFromParent(this);
-    Frame::detach();
+    m_loader.detachFromParent();
 }
 
 bool LocalFrame::inScope(TreeScope* scope) const
@@ -324,6 +315,12 @@ void LocalFrame::removeDestructionObserver(FrameDestructionObserver* 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();
 
     WillBeHeapHashSet<RawPtrWillBeWeakMember<FrameDestructionObserver> >::iterator stop = m_destructionObservers.end();
     for (WillBeHeapHashSet<RawPtrWillBeWeakMember<FrameDestructionObserver> >::iterator it = m_destructionObservers.begin(); it != stop; ++it)
@@ -340,6 +337,13 @@ void LocalFrame::willDetachFrameHost()
         page()->scrollingCoordinator()->willDestroyScrollableArea(m_view.get());
 }
 
+void LocalFrame::detachFromFrameHost()
+{
+    // We should never be detaching the page during a Layout.
+    RELEASE_ASSERT(!m_view || !m_view->isInPerformLayout());
+    m_host = nullptr;
+}
+
 String LocalFrame::documentTypeString() const
 {
     if (DocumentType* doctype = document()->doctype())
@@ -576,7 +580,7 @@ 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 (host()->frameCount() >= FrameHost::maxNumberOfFrames)
+    if (page()->subframeCount() >= Page::maxNumberOfFrames)
         return false;
     bool foundSelfReference = false;
     for (const Frame* frame = this; frame; frame = frame->tree().parent()) {

diff --git a/Source/core/frame/LocalFrame.h b/Source/core/frame/LocalFrame.h
@@ -85,6 +85,7 @@ namespace blink {
         void removeDestructionObserver(FrameDestructionObserver*);
 
         void willDetachFrameHost();
+        void detachFromFrameHost();
 
         virtual void disconnectOwnerElement() OVERRIDE;
 

diff --git a/Source/core/frame/RemoteFrame.cpp b/Source/core/frame/RemoteFrame.cpp
@@ -34,7 +34,7 @@ void RemoteFrame::navigate(Document&, const KURL& url, const Referrer& referrer,
 void RemoteFrame::detach()
 {
     detachChildren();
-    Frame::detach();
+    m_host = nullptr;
 }
 
 void RemoteFrame::setView(PassRefPtr<RemoteFrameView> view)

diff --git a/Source/core/loader/EmptyClients.h b/Source/core/loader/EmptyClients.h
@@ -181,7 +181,7 @@ class EmptyFrameLoaderClient : public FrameLoaderClient {
     virtual Frame* nextSibling() const OVERRIDE { return 0; }
     virtual Frame* firstChild() const OVERRIDE { return 0; }
     virtual Frame* lastChild() const OVERRIDE { return 0; }
-    virtual void detached() OVERRIDE { }
+    virtual void detachedFromParent() OVERRIDE { }
 
     virtual void dispatchWillSendRequest(DocumentLoader*, unsigned long, ResourceRequest&, const ResourceResponse&) OVERRIDE { }
     virtual void dispatchDidReceiveResponse(DocumentLoader*, unsigned long, const ResourceResponse&) OVERRIDE { }

diff --git a/Source/core/loader/FrameLoader.cpp b/Source/core/loader/FrameLoader.cpp
@@ -1105,22 +1105,61 @@ String FrameLoader::userAgent(const KURL& url) const
     return userAgent;
 }
 
-void FrameLoader::detach()
+void FrameLoader::detachFromParent()
 {
 #if !ENABLE(OILPAN)
     // The caller must protect a reference to m_frame.
     ASSERT(m_frame->refCount() > 1);
 #endif
+
+    InspectorInstrumentation::frameDetachedFromParent(m_frame);
+
     if (m_documentLoader)
         m_documentLoader->detachFromFrame();
     m_documentLoader = nullptr;
 
+    if (!client())
+        return;
+
+    // FIXME: All this code belongs up in Page.
     Frame* parent = m_frame->tree().parent();
-    if (parent && parent->isLocalFrame())
+    if (parent && parent->isLocalFrame()) {
+        m_frame->setView(nullptr);
+        // FIXME: Shouldn't need to check if page() is null here.
+        if (m_frame->owner() && m_frame->page())
+            m_frame->page()->decrementSubframeCount();
+        m_frame->willDetachFrameHost();
+        detachClient();
         toLocalFrame(parent)->loader().scheduleCheckCompleted();
+    } else {
+        m_frame->setView(nullptr);
+        m_frame->willDetachFrameHost();
+        detachClient();
+    }
+    m_frame->detachFromFrameHost();
+}
+
+void FrameLoader::detachClient()
+{
+    ASSERT(client());
+
+    // Finish all cleanup work that might require talking to the embedder.
     m_progressTracker->dispose();
     m_progressTracker.clear();
     setOpener(0);
+    // Notify ScriptController that the frame is closing, since its cleanup ends up calling
+    // back to FrameLoaderClient via WindowProxy.
+    m_frame->script().clearForClose();
+
+    // client() should never be null because that means we somehow re-entered
+    // the frame detach code... but it is sometimes.
+    // FIXME: Understand why this is happening so we can document this insanity.
+    if (client()) {
+        // After this, we must no longer talk to the client since this clears
+        // its owning reference back to our owning LocalFrame.
+        client()->detachedFromParent();
+        m_frame->clearClient();
+    }
 }
 
 void FrameLoader::receivedMainResourceError(const ResourceError& error)

diff --git a/Source/core/loader/FrameLoader.h b/Source/core/loader/FrameLoader.h
@@ -150,7 +150,7 @@ class FrameLoader FINAL {
     Frame* opener();
     void setOpener(LocalFrame*);
 
-    void detach();
+    void detachFromParent();
 
     void loadDone();
     void finishedParsing();
@@ -208,6 +208,7 @@ class FrameLoader FINAL {
 
     bool validateTransitionNavigationMode();
     bool dispatchNavigationTransitionData();
+    void detachClient();
 
     void setHistoryItemStateForCommit(HistoryCommitType, bool isPushOrReplaceState = false, PassRefPtr<SerializedScriptValue> = nullptr);
 

diff --git a/Source/core/loader/FrameLoaderClient.h b/Source/core/loader/FrameLoaderClient.h
@@ -76,6 +76,8 @@ namespace blink {
 
         virtual bool hasWebView() const = 0; // mainly for assertions
 
+        virtual void detachedFromParent() = 0;
+
         virtual void dispatchWillSendRequest(DocumentLoader*, unsigned long identifier, ResourceRequest&, const ResourceResponse& redirectResponse) = 0;
         virtual void dispatchDidReceiveResponse(DocumentLoader*, unsigned long identifier, const ResourceResponse&) = 0;
         virtual void dispatchDidFinishLoading(DocumentLoader*, unsigned long identifier) = 0;

diff --git a/Source/core/page/Page.cpp b/Source/core/page/Page.cpp
@@ -128,6 +128,7 @@ Page::Page(PageClients& pageClients)
     , m_editorClient(pageClients.editorClient)
     , m_spellCheckerClient(pageClients.spellCheckerClient)
     , m_storageClient(pageClients.storageClient)
+    , m_subframeCount(0)
     , m_openedByDOM(false)
     , m_tabKeyCyclesThroughElements(true)
     , m_defersLoading(false)
@@ -440,6 +441,19 @@ double Page::timerAlignmentInterval() const
     return m_timerAlignmentInterval;
 }
 
+#if ENABLE(ASSERT)
+void Page::checkSubframeCountConsistency() const
+{
+    ASSERT(m_subframeCount >= 0);
+
+    int subframeCount = 0;
+    for (Frame* frame = mainFrame(); frame; frame = frame->tree().traverseNext())
+        ++subframeCount;
+
+    ASSERT(m_subframeCount + 1 == subframeCount);
+}
+#endif
+
 void Page::setVisibilityState(PageVisibilityState visibilityState, bool isInitialState)
 {
     if (m_visibilityState == visibilityState)

diff --git a/Source/core/page/Page.h b/Source/core/page/Page.h
@@ -134,6 +134,10 @@ class Page FINAL : public NoBaseWillBeGarbageCollectedFinalized<Page>, public Wi
     bool openedByDOM() const;
     void setOpenedByDOM();
 
+    void incrementSubframeCount() { ++m_subframeCount; }
+    void decrementSubframeCount() { ASSERT(m_subframeCount); --m_subframeCount; }
+    int subframeCount() const { checkSubframeCountConsistency(); return m_subframeCount; }
+
     PageAnimator& animator() { return m_animator; }
     Chrome& chrome() const { return *m_chrome; }
     AutoscrollController& autoscrollController() const { return *m_autoscrollController; }
@@ -181,6 +185,12 @@ class Page FINAL : public NoBaseWillBeGarbageCollectedFinalized<Page>, public Wi
     StorageNamespace* sessionStorage(bool optionalCreate = true);
     StorageClient& storageClient() const { return *m_storageClient; }
 
+    // Don't allow more than a certain number of frames in a page.
+    // This seems like a reasonable upper bound, and otherwise mutually
+    // recursive frameset pages can quickly bring the program to its knees
+    // with exponential growth in the number of frames.
+    static const int maxNumberOfFrames = 1000;
+
     PageVisibilityState visibilityState() const;
     void setVisibilityState(PageVisibilityState, bool);
 
@@ -218,6 +228,12 @@ class Page FINAL : public NoBaseWillBeGarbageCollectedFinalized<Page>, public Wi
 private:
     void initGroup();
 
+#if ENABLE(ASSERT)
+    void checkSubframeCountConsistency() const;
+#else
+    void checkSubframeCountConsistency() const { }
+#endif
+
     void setTimerAlignmentInterval(double);
 
     void setNeedsLayoutInAllFrames();
@@ -261,6 +277,7 @@ class Page FINAL : public NoBaseWillBeGarbageCollectedFinalized<Page>, public Wi
 
     UseCounter m_useCounter;
 
+    int m_subframeCount;
     bool m_openedByDOM;
 
     bool m_tabKeyCyclesThroughElements;