diff --git a/Source/core/dom/NodeRareData.cpp b/Source/core/dom/NodeRareData.cpp index 1f0489a8ee9..c40e9bb58eb 100644 --- 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 index 7a4d3ff5fd9..302a3e1e132 100644 --- 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 > 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 index c31bb006cb5..4de75697368 100644 --- a/Source/core/frame/Frame.h +++ b/Source/core/frame/Frame.h @@ -59,10 +59,11 @@ class Frame : public RefCountedWillBeGarbageCollectedFinalized { 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 index d7adbc27695..3d388af7e40 100644 --- 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 index 353f6407206..8e5541d6647 100644 --- 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 index efceca02ce5..e4a7a36fc88 100644 --- a/Source/core/frame/FrameHost.h +++ b/Source/core/frame/FrameHost.h @@ -82,15 +82,6 @@ class FrameHost FINAL : public NoBaseWillBeGarbageCollectedFinalized 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 const OwnPtrWillBeMember m_eventHandlerRegistry; AtomicString m_overrideEncoding; - int m_frameCount; }; } diff --git a/Source/core/frame/LocalFrame.cpp b/Source/core/frame/LocalFrame.cpp index 14c5bd11029..34698bdd148 100644 --- 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 >::iterator stop = m_destructionObservers.end(); for (WillBeHeapHashSet >::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 index 3df03109225..46e38899fc1 100644 --- 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 index 5650f9ac19d..28bed4677ac 100644 --- 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 view) diff --git a/Source/core/loader/EmptyClients.h b/Source/core/loader/EmptyClients.h index ccd15039fe2..6b72c5d9448 100644 --- 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 index 8a28b7aab61..37d3db67ca3 100644 --- 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 index 8a6dc82beb1..f6e07c03423 100644 --- 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 = nullptr); diff --git a/Source/core/loader/FrameLoaderClient.h b/Source/core/loader/FrameLoaderClient.h index 68874f63b63..2afebccfb72 100644 --- 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 index 1ee917e6c51..ea017a3640d 100644 --- 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 index 9da896b3381..c0bf5bfe39e 100644 --- a/Source/core/page/Page.h +++ b/Source/core/page/Page.h @@ -134,6 +134,10 @@ class Page FINAL : public NoBaseWillBeGarbageCollectedFinalized, 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, 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, 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, public Wi UseCounter m_useCounter; + int m_subframeCount; bool m_openedByDOM; bool m_tabKeyCyclesThroughElements; diff --git a/Source/web/FrameLoaderClientImpl.cpp b/Source/web/FrameLoaderClientImpl.cpp index 81fa4e669eb..8c2d6826ff5 100644 --- a/Source/web/FrameLoaderClientImpl.cpp +++ b/Source/web/FrameLoaderClientImpl.cpp @@ -290,7 +290,7 @@ Frame* FrameLoaderClientImpl::lastChild() const return toCoreFrame(m_webFrame->lastChild()); } -void FrameLoaderClientImpl::detached() +void FrameLoaderClientImpl::detachedFromParent() { // Alert the client that the frame is being detached. This is the last // chance we have to communicate with the client. diff --git a/Source/web/FrameLoaderClientImpl.h b/Source/web/FrameLoaderClientImpl.h index bea46ecb303..398caf7d03e 100644 --- a/Source/web/FrameLoaderClientImpl.h +++ b/Source/web/FrameLoaderClientImpl.h @@ -75,7 +75,7 @@ class FrameLoaderClientImpl FINAL : public FrameLoaderClient { virtual Frame* nextSibling() const OVERRIDE; virtual Frame* firstChild() const OVERRIDE; virtual Frame* lastChild() const OVERRIDE; - virtual void detached() OVERRIDE; + virtual void detachedFromParent() OVERRIDE; virtual void dispatchWillSendRequest(DocumentLoader*, unsigned long identifier, ResourceRequest&, const ResourceResponse& redirectResponse) OVERRIDE; virtual void dispatchDidReceiveResponse(DocumentLoader*, unsigned long identifier, const ResourceResponse&) OVERRIDE; virtual void dispatchDidChangeResourcePriority(unsigned long identifier, ResourceLoadPriority, int intraPriorityValue) OVERRIDE; diff --git a/Source/web/RemoteFrameClientImpl.cpp b/Source/web/RemoteFrameClientImpl.cpp index 5d14cf5f96f..e13961e9d84 100644 --- a/Source/web/RemoteFrameClientImpl.cpp +++ b/Source/web/RemoteFrameClientImpl.cpp @@ -18,11 +18,6 @@ RemoteFrameClientImpl::RemoteFrameClientImpl(WebRemoteFrameImpl* webFrame) { } -void RemoteFrameClientImpl::detached() -{ - // FIXME: Implement. -} - Frame* RemoteFrameClientImpl::opener() const { return toCoreFrame(m_webFrame->opener()); diff --git a/Source/web/RemoteFrameClientImpl.h b/Source/web/RemoteFrameClientImpl.h index f4c2003d7bb..09ae820f5b2 100644 --- a/Source/web/RemoteFrameClientImpl.h +++ b/Source/web/RemoteFrameClientImpl.h @@ -15,8 +15,6 @@ class RemoteFrameClientImpl : public RemoteFrameClient { explicit RemoteFrameClientImpl(WebRemoteFrameImpl*); // FrameClient overrides: - virtual void detached() OVERRIDE; - virtual Frame* opener() const OVERRIDE; virtual void setOpener(Frame*) OVERRIDE; diff --git a/Source/web/tests/WebFrameTest.cpp b/Source/web/tests/WebFrameTest.cpp index 218c41c38ee..9d4971d0a10 100644 --- a/Source/web/tests/WebFrameTest.cpp +++ b/Source/web/tests/WebFrameTest.cpp @@ -3744,7 +3744,9 @@ TEST_F(WebFrameTest, FindDetachFrameWhileScopingStrings) EXPECT_FALSE(client.findResultsAreReady()); mainFrame->resetMatchCount(); - mainFrame->scopeStringMatches(kFindIdentifier, searchText, options, true); + + for (WebFrame* frame = mainFrame; frame; frame = frame->traverseNext(false)) + frame->scopeStringMatches(kFindIdentifier, searchText, options, true); // The first scopeStringMatches will have reset the state. Detach before it actually scopes. EXPECT_TRUE(mainFrame->document().getElementById("frame").remove());