2011-01-08 Charlie Reis <creis@chromium.org>
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 9 Jan 2011 01:27:19 +0000 (01:27 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 9 Jan 2011 01:27:19 +0000 (01:27 +0000)
        Reviewed by Mihai Parparita.

        Canceled frame loads can corrupt back forward list
        https://bugs.webkit.org/show_bug.cgi?id=50254

        http/tests/navigation/forward-and-cancel.html aborts a slowly loading
        subframe in a frame tree and ensures the history items are updated properly.

        * LayoutTests/http/tests/navigation/forward-and-cancel-expected.txt: Added.
        * LayoutTests/http/tests/navigation/forward-and-cancel.html: Added.
        * LayoutTests/http/tests/navigation/resources/forward-and-cancel-frames.html: Added.
        * LayoutTests/http/tests/navigation/resources/forward-and-cancel-frames-container.html: Added.
2011-01-08  Charlie Reis  <creis@chromium.org>

        Reviewed by Mihai Parparita.

        Canceled frame loads can corrupt back forward list
        https://bugs.webkit.org/show_bug.cgi?id=50254

        Avoids changing m_currentItem until the navigation commits.
        Also resets top-level history items if a subframe navigation is canceled.

        * WebCore/loader/FrameLoader.cpp:
        (WebCore::FrameLoader::checkLoadCompleteForThisFrame):
        * WebCore/loader/HistoryController.cpp:
        * WebCore/loader/HistoryController.h:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@75336 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/http/tests/navigation/forward-and-cancel-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/navigation/forward-and-cancel.html [new file with mode: 0644]
LayoutTests/http/tests/navigation/resources/forward-and-cancel-frames-container.html [new file with mode: 0644]
LayoutTests/http/tests/navigation/resources/forward-and-cancel-frames.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/HistoryController.cpp
Source/WebCore/loader/HistoryController.h

index 6a7743e3660d76679ea761661c85ba455fdf7fab..9cb1e4fe6b58df227396872c932e8dab9375ac01 100644 (file)
@@ -1,3 +1,18 @@
+2011-01-08  Charlie Reis  <creis@chromium.org>
+
+        Reviewed by Mihai Parparita.
+
+        Canceled frame loads can corrupt back forward list
+        https://bugs.webkit.org/show_bug.cgi?id=50254
+
+        http/tests/navigation/forward-and-cancel.html aborts a slowly loading
+        subframe in a frame tree and ensures the history items are updated properly.
+
+        * LayoutTests/http/tests/navigation/forward-and-cancel-expected.txt: Added.
+        * LayoutTests/http/tests/navigation/forward-and-cancel.html: Added.
+        * LayoutTests/http/tests/navigation/resources/forward-and-cancel-frames.html: Added.
+        * LayoutTests/http/tests/navigation/resources/forward-and-cancel-frames-container.html: Added.
+
 2011-01-08  Chang Shu  <chang.shu@nokia.com>
 
         Reviewed by Kenneth Rohde Christiansen.
diff --git a/LayoutTests/http/tests/navigation/forward-and-cancel-expected.txt b/LayoutTests/http/tests/navigation/forward-and-cancel-expected.txt
new file mode 100644 (file)
index 0000000..caedb4c
--- /dev/null
@@ -0,0 +1,15 @@
+This test checks that the backForward list is not corrupted when a frame load is canceled.
+
+If testing manually, click here.
+
+============== Back Forward List ==============
+curr->  http://127.0.0.1:8000/navigation/forward-and-cancel.html  **nav target**
+        http://127.0.0.1:8000/navigation/resources/forward-and-cancel-frames-container.html  **nav target**
+            http://127.0.0.1:8000/navigation/resources/forward-and-cancel-frames.html (in frame "<!--framePath //<!--frame0-->-->")
+                about:blank (in frame "frame1")
+            http://127.0.0.1:8000/navigation/resources/otherpage.html (in frame "<!--framePath //<!--frame1-->-->")
+        http://127.0.0.1:8000/navigation/resources/forward-and-cancel-frames-container.html
+            http://127.0.0.1:8000/navigation/resources/forward-and-cancel-frames.html (in frame "<!--framePath //<!--frame0-->-->")
+                http://127.0.0.1:8000/navigation/resources/slow-resource-1-sec.pl (in frame "frame1")  **nav target**
+            http://127.0.0.1:8000/navigation/resources/otherpage.html (in frame "<!--framePath //<!--frame1-->-->")
+===============================================
diff --git a/LayoutTests/http/tests/navigation/forward-and-cancel.html b/LayoutTests/http/tests/navigation/forward-and-cancel.html
new file mode 100644 (file)
index 0000000..2e777af
--- /dev/null
@@ -0,0 +1,43 @@
+<script>
+// Test steps:
+// 1. Start on a page with no frames (this page).
+// 2. Navigate to a page with a frame tree (Grandparent, Parent, Child, Uncle).
+// 3. Navigate child frame to a slowly loading URL.
+// 4. Go back to about:blank in child frame.
+//    Important to use about:blank, which can commit immediately while walking the tree.
+// 5. Go forward to slow URL, but stop before the navigation commits.
+//    Important to cancel the load and ensure the history is not corrupted.
+// 6. Go back to start page with no frames.
+//    Important for testing that subframes can be removed.
+if (window.layoutTestController) {
+    layoutTestController.dumpBackForwardList();
+    layoutTestController.dumpAsText();
+    layoutTestController.queueLoad("resources/forward-and-cancel-frames-container.html");
+    layoutTestController.queueLoadingScript("frames[0].clickLink();");
+    layoutTestController.queueBackNavigation(1);
+
+    // Go forward to slow URL in child frame, but stop right away.  This should
+    // reset the backForward list to the previous entry.
+    layoutTestController.queueNonLoadingScript("setTimeout('history.forward();',0); setTimeout('window.stop();',10);");
+
+    // Now go back to make sure the backForwardList is not corrupted.
+    layoutTestController.queueNonLoadingScript("setTimeout('history.back();',50);");
+
+    // Wait until we get back to this page.
+    layoutTestController.queueLoadingScript("layoutTestController.waitUntilDone();");
+}
+</script>
+<p>This test checks that the backForward list is not corrupted when a frame load is canceled.
+<p>If testing manually, <a href="resources/forward-and-cancel-frames-container.html">click here</a>.
+
+<script>
+if (window.layoutTestController) {
+    // Only notify done when we return to this page a second time.
+    if (!window.localStorage.started) {
+        window.localStorage.started = true;
+    } else {
+        delete window.localStorage.started;
+        layoutTestController.notifyDone();
+    }
+}
+</script>
diff --git a/LayoutTests/http/tests/navigation/resources/forward-and-cancel-frames-container.html b/LayoutTests/http/tests/navigation/resources/forward-and-cancel-frames-container.html
new file mode 100644 (file)
index 0000000..0ea96c7
--- /dev/null
@@ -0,0 +1,7 @@
+<!-- Grandparent page -->
+
+<!-- Parent iframe -->
+<iframe src="forward-and-cancel-frames.html" width=500 height=400></iframe>
+
+<!-- Uncle iframe -->
+<iframe src="otherpage.html"></iframe>
diff --git a/LayoutTests/http/tests/navigation/resources/forward-and-cancel-frames.html b/LayoutTests/http/tests/navigation/resources/forward-and-cancel-frames.html
new file mode 100644 (file)
index 0000000..761f285
--- /dev/null
@@ -0,0 +1,15 @@
+<!-- Parent page -->
+<script>
+function clickLink() {
+  // Simulate a mouse click on a link targeting the child frame.
+  var evt = document.createEvent("MouseEvents");
+  evt.initMouseEvent("click", true, true, window,
+      0, 0, 0, 0, 0, false, false, false, false, 0, null);
+  document.getElementById("link").dispatchEvent(evt);
+}
+</script>
+<!-- Child iframe -->
+<iframe id="frame1" src="about:blank"></iframe>
+<br>
+<p>This test checks that the backForward list is not corrupted when a frame load is canceled.
+<p>If testing manually, <a id="link" href="slow-resource-1-sec.pl" target="frame1">click here</a> and then Back.  Then click Forward and quickly click Stop.  Ensure that Back and Forward still work.
index efb906106a70c41e3518dd0f18590da1153d5432..e409e3c797ab83568b0315d0a1f3dc1581bf5ce5 100644 (file)
@@ -1,3 +1,18 @@
+2011-01-08  Charlie Reis  <creis@chromium.org>
+
+        Reviewed by Mihai Parparita.
+
+        Canceled frame loads can corrupt back forward list
+        https://bugs.webkit.org/show_bug.cgi?id=50254
+
+        Avoids changing m_currentItem until the navigation commits.
+        Also resets top-level history items if a subframe navigation is canceled.
+
+        * WebCore/loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::checkLoadCompleteForThisFrame):
+        * WebCore/loader/HistoryController.cpp:
+        * WebCore/loader/HistoryController.h:
+
 2011-01-08  Chang Shu  <chang.shu@nokia.com>
 
         Reviewed by Kenneth Rohde Christiansen.
index 6dcd054e27832cec746938654cfad9a5f70f8d1c..3d864becad970b5987740bf54720633424bac15b 100644 (file)
@@ -2333,8 +2333,9 @@ void FrameLoader::checkLoadCompleteForThisFrame()
             // Check all children first.
             RefPtr<HistoryItem> item;
             if (Page* page = m_frame->page())
-                if (isBackForwardLoadType(loadType()) && m_frame == page->mainFrame())
-                    item = history()->currentItem();
+                if (isBackForwardLoadType(loadType()))
+                    // Reset the back forward list to the last committed history item at the top level.
+                    item = page->mainFrame()->loader()->history()->currentItem();
                 
             bool shouldReset = true;
             if (!(pdl->isLoadingInAPISense() && !pdl->isStopping())) {
index ff733a9e33a7d6983befa875b058af188fd4745d..89ae80289622d52f71f37d0c504dd25ed174f833 100644 (file)
@@ -235,6 +235,13 @@ void HistoryController::goToItem(HistoryItem* targetItem, FrameLoadType type)
     page->backForward()->setCurrentItem(targetItem);
     Settings* settings = m_frame->settings();
     page->setGlobalHistoryItem((!settings || settings->privateBrowsingEnabled()) ? 0 : targetItem);
+
+    // First set the provisional item of any frames that are not actually navigating.
+    // This must be done before trying to navigate the desired frame, because some
+    // navigations can commit immediately (such as about:blank).  We must be sure that
+    // all frames have provisional items set before the commit.
+    recursiveSetProvisionalItem(targetItem, currentItem, type);
+    // Now that all other frames have provisional items, do the actual navigation.
     recursiveGoToItem(targetItem, currentItem, type);
 }
 
@@ -399,9 +406,50 @@ void HistoryController::updateForCommit()
         ASSERT(m_provisionalItem);
         m_currentItem = m_provisionalItem;
         m_provisionalItem = 0;
+
+        // Tell all other frames in the tree to commit their provisional items and
+        // restore their scroll position.  We'll avoid this frame (which has already
+        // committed) and its children (which will be replaced).
+        Page* page = m_frame->page();
+        ASSERT(page);
+        page->mainFrame()->loader()->history()->recursiveUpdateForCommit();
     }
 }
 
+void HistoryController::recursiveUpdateForCommit()
+{
+    // The frame that navigated will now have a null provisional item.
+    // Ignore it and its children.
+    if (!m_provisionalItem)
+        return;
+
+    // For each frame that already had the content the item requested (based on
+    // (a matching URL and frame tree snapshot), just restore the scroll position.
+    // Save form state (works from currentItem, since m_frameLoadComplete is true)
+    ASSERT(m_frameLoadComplete);
+    saveDocumentState();
+    saveScrollPositionAndViewStateToItem(m_currentItem.get());
+
+    if (FrameView* view = m_frame->view())
+        view->setWasScrolledByUser(false);
+
+    // Now commit the provisional item
+    m_frameLoadComplete = false;
+    m_previousItem = m_currentItem;
+    m_currentItem = m_provisionalItem;
+    m_provisionalItem = 0;
+
+    // Restore form state (works from currentItem)
+    restoreDocumentState();
+
+    // Restore the scroll position (we choose to do this rather than going back to the anchor point)
+    restoreScrollPositionAndViewState();
+
+    // Iterate over the rest of the tree
+    for (Frame* child = m_frame->tree()->firstChild(); child; child = child->tree()->nextSibling())
+        child->loader()->history()->recursiveUpdateForCommit();
+}
+
 void HistoryController::updateForSameDocumentNavigation()
 {
     if (m_frame->loader()->url().isEmpty())
@@ -551,44 +599,42 @@ PassRefPtr<HistoryItem> HistoryController::createItemTree(Frame* targetFrame, bo
 
 // The general idea here is to traverse the frame tree and the item tree in parallel,
 // tracking whether each frame already has the content the item requests.  If there is
-// a match (by URL), we just restore scroll position and recurse.  Otherwise we must
-// reload that frame, and all its kids.
-void HistoryController::recursiveGoToItem(HistoryItem* item, HistoryItem* fromItem, FrameLoadType type)
+// a match, we set the provisional item and recurse.  Otherwise we will reload that
+// frame and all its kids in recursiveGoToItem.
+void HistoryController::recursiveSetProvisionalItem(HistoryItem* item, HistoryItem* fromItem, FrameLoadType type)
 {
     ASSERT(item);
     ASSERT(fromItem);
 
-    // If the item we're going to is a clone of the item we're at, then do
-    // not load it again, and continue history traversal to its children.
-    // The current frame tree and the frame tree snapshot in the item have
-    // to match.
-    // Note: If item and fromItem are the same, then we need to create a new
-    // document.
-    if (item != fromItem 
-        && item->itemSequenceNumber() == fromItem->itemSequenceNumber()
-        && currentFramesMatchItem(item)
-        && fromItem->hasSameFrames(item))
-    {
-        // This content is good, so leave it alone and look for children that need reloading
-        // Save form state (works from currentItem, since m_frameLoadComplete is true)
-        ASSERT(m_frameLoadComplete);
-        saveDocumentState();
-        saveScrollPositionAndViewStateToItem(m_currentItem.get());
+    if (itemsAreClones(item, fromItem)) {
+        // Set provisional item, which will be committed in recursiveUpdateForCommit.
+        m_provisionalItem = item;
 
-        if (FrameView* view = m_frame->view())
-            view->setWasScrolledByUser(false);
+        const HistoryItemVector& childItems = item->children();
 
-        m_previousItem = m_currentItem;
-        m_currentItem = item;
-                
-        // Restore form state (works from currentItem)
-        restoreDocumentState();
-        
-        // Restore the scroll position (we choose to do this rather than going back to the anchor point)
-        restoreScrollPositionAndViewState();
-        
+        int size = childItems.size();
+        for (int i = 0; i < size; ++i) {
+            String childFrameName = childItems[i]->target();
+            HistoryItem* fromChildItem = fromItem->childItemWithTarget(childFrameName);
+            ASSERT(fromChildItem);
+            Frame* childFrame = m_frame->tree()->child(childFrameName);
+            ASSERT(childFrame);
+            childFrame->loader()->history()->recursiveSetProvisionalItem(childItems[i].get(), fromChildItem, type);
+        }
+    }
+}
+
+// We now traverse the frame tree and item tree a second time, loading frames that
+// do have the content the item requests.
+void HistoryController::recursiveGoToItem(HistoryItem* item, HistoryItem* fromItem, FrameLoadType type)
+{
+    ASSERT(item);
+    ASSERT(fromItem);
+
+    if (itemsAreClones(item, fromItem)) {
+        // Just iterate over the rest, looking for frames to navigate.
         const HistoryItemVector& childItems = item->children();
-        
+
         int size = childItems.size();
         for (int i = 0; i < size; ++i) {
             String childFrameName = childItems[i]->target();
@@ -603,6 +649,21 @@ void HistoryController::recursiveGoToItem(HistoryItem* item, HistoryItem* fromIt
     }
 }
 
+bool HistoryController::itemsAreClones(HistoryItem* item1, HistoryItem* item2) const
+{
+    // If the item we're going to is a clone of the item we're at, then we do
+    // not need to load it again.  The current frame tree and the frame tree
+    // snapshot in the item have to match.
+    // Note: Some clients treat a navigation to the current history item as
+    // a reload.  Thus, if item1 and item2 are the same, we need to create a
+    // new document and should not consider them clones.
+    // (See http://webkit.org/b/35532 for details.)
+    return item1 != item2
+        && item1->itemSequenceNumber() == item2->itemSequenceNumber()
+        && currentFramesMatchItem(item1)
+        && item2->hasSameFrames(item1);
+}
+
 // Helper method that determines whether the current frame tree matches given history item's.
 bool HistoryController::currentFramesMatchItem(HistoryItem* item) const
 {
index 1bf5072c0edb8d29b28504d306558d0f846de485..9923179a46d6b138a9021e6850fed185b7e93418 100644 (file)
@@ -87,7 +87,10 @@ private:
     PassRefPtr<HistoryItem> createItem(bool useOriginal);
     PassRefPtr<HistoryItem> createItemTree(Frame* targetFrame, bool clipAtTarget);
 
+    void recursiveSetProvisionalItem(HistoryItem*, HistoryItem*, FrameLoadType);
     void recursiveGoToItem(HistoryItem*, HistoryItem*, FrameLoadType);
+    void recursiveUpdateForCommit();
+    bool itemsAreClones(HistoryItem*, HistoryItem*) const;
     bool currentFramesMatchItem(HistoryItem*) const;
     void updateBackForwardListClippedAtTarget(bool doClip);