Detach all subframes before swapping a frame.

BUG=357747 Review URL: https://codereview.chromium.org/544443002 git-svn-id: svn://svn.chromium.org/blink/trunk@181764 bbb929c8-8fbe-4397-9dbb-9b2b20218538

Detach all subframes before swapping a frame.
BUG=357747 Review URL: https://codereview.chromium.org/544443002 git-svn-id: svn://svn.chromium.org/blink/trunk@181764 bbb929c8-8fbe-4397-9dbb-9b2b20218538
51d2ee33 · nasko@chromium.org · 015956cc · 51d2ee33 · 51d2ee33 · 51d2ee33
Commit 51d2ee33 authored Sep 10, 2014 by nasko@chromium.org
4 changed files
--- a/third_party/WebKit/Source/web/WebFrame.cpp
+++ b/third_party/WebKit/Source/web/WebFrame.cpp
@@ -25,12 +25,21 @@ Frame* toCoreFrame(const WebFrame* frame)
        : toWebRemoteFrameImpl(frame)->frame();
 }

-void WebFrame::swap(WebFrame* frame)
+bool WebFrame::swap(WebFrame* frame)
 {
    using std::swap;
+    RefPtr<Frame> oldFrame = toCoreFrame(this);
+
+    // All child frames must be detached first.
+    oldFrame->detachChildren();
+
+    // If the frame has been detached during detaching its children, return
+    // immediately.
+    // FIXME: There is no unit test for this condition, so one needs to be
+    // written.
+    if (!oldFrame->host())
+        return false;

-    // All child frames must have been detached first.
-    ASSERT(!m_firstChild && !m_lastChild);
    // The frame being swapped in should not have a Frame associated
    // with it yet.
    ASSERT(!toCoreFrame(frame));
@@ -66,7 +75,6 @@ void WebFrame::swap(WebFrame* frame)
    // the type of the passed in WebFrame.
    // FIXME: This is a bit clunky; this results in pointless decrements and
    // increments of connected subframes.
-    Frame* oldFrame = toCoreFrame(this);
    FrameOwner* owner = oldFrame->owner();
    oldFrame->disconnectOwnerElement();
    if (frame->isWebLocalFrame()) {
@@ -74,6 +82,8 @@ void WebFrame::swap(WebFrame* frame)
    } else {
        toWebRemoteFrameImpl(frame)->initializeCoreFrame(oldFrame->host(), owner, oldFrame->tree().name());
    }
+
+    return true;
 }

 v8::Handle<v8::Value> WebFrame::executeScriptAndReturnValueForTests(const WebScriptSource& source)

--- a/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
+++ b/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
@@ -6125,6 +6125,49 @@ TEST_F(WebFrameSwapTest, SwapLastChild)
    remoteFrame->close();
 }

+void swapAndVerifySubframeConsistency(const char* const message, WebFrame* oldFrame, WebFrame* newFrame)
+{
+    SCOPED_TRACE(message);
+
+    EXPECT_TRUE(oldFrame->firstChild());
+    oldFrame->swap(newFrame);
+
+    EXPECT_FALSE(newFrame->firstChild());
+    EXPECT_FALSE(newFrame->lastChild());
+}
+
+TEST_F(WebFrameSwapTest, SwapParentShouldDetachChildren)
+{
+    WebRemoteFrame* remoteFrame = WebRemoteFrame::create(0);
+    WebFrame* targetFrame = mainFrame()->firstChild()->nextSibling();
+    EXPECT_TRUE(targetFrame);
+    swapAndVerifySubframeConsistency("local->remote", targetFrame, remoteFrame);
+
+    targetFrame = mainFrame()->firstChild()->nextSibling();
+    EXPECT_TRUE(targetFrame);
+
+    // Create child frames in the target frame before testing the swap.
+    FrameTestHelpers::TestWebFrameClient remoteFrameClient;
+    remoteFrame->createRemoteChild("", &remoteFrameClient);
+
+    FrameTestHelpers::TestWebFrameClient client;
+    WebFrame* localFrame = WebLocalFrame::create(&client);
+    swapAndVerifySubframeConsistency("remote->local", targetFrame, localFrame);
+
+    // FIXME: This almost certainly fires more load events on the iframe element
+    // than it should.
+    // Finally, make sure an embedder triggered load in the local frame swapped
+    // back in works.
+    FrameTestHelpers::loadFrame(localFrame, m_baseURL + "subframe-hello.html");
+    std::string content = localFrame->contentAsText(1024).utf8();
+    EXPECT_EQ("hello", content);
+
+    // Manually reset to break WebViewHelper's dependency on the stack allocated
+    // TestWebFrameClient.
+    reset();
+    remoteFrame->close();
+}
+
 class MockDocumentThreadableLoaderClient : public DocumentThreadableLoaderClient {
 public:
    MockDocumentThreadableLoaderClient() : m_failed(false) { }

--- a/third_party/WebKit/Source/web/tests/data/subframe-b.html
+++ b/third_party/WebKit/Source/web/tests/data/subframe-b.html
@@ -2,5 +2,6 @@
 <html>
 <body>
 <p>b
+<iframe name="foo" src="subframe-a.html">
 </body>
 </html>
--- a/third_party/WebKit/public/web/WebFrame.h
+++ b/third_party/WebKit/public/web/WebFrame.h
@@ -115,7 +115,7 @@ public:
    virtual bool isWebRemoteFrame() const = 0;
    virtual WebRemoteFrame* toWebRemoteFrame() = 0;

-    BLINK_EXPORT void swap(WebFrame*);
+    BLINK_EXPORT bool swap(WebFrame*);

    // This method closes and deletes the WebFrame.
    virtual void close() = 0;