2011-02-04 Charlie Reis <creis@chromium.org>
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 5 Feb 2011 00:48:31 +0000 (00:48 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 5 Feb 2011 00:48:31 +0000 (00:48 +0000)
        Reviewed by Mihai Parparita.

        Crash in WebCore::HistoryController::itemsAreClones
        https://bugs.webkit.org/show_bug.cgi?id=52819

        Tests that navigating back and forward between hash items works.

        * fast/history/history-back-forward-within-subframe-hash.html: Added.
        * fast/history/history-back-forward-within-subframe-hash-expected.txt: Added.
2011-02-04  Charlie Reis  <creis@chromium.org>

        Reviewed by Mihai Parparita.

        Crash in WebCore::HistoryController::itemsAreClones
        https://bugs.webkit.org/show_bug.cgi?id=52819

        Avoids deleting the current HistoryItem while it is still in use.
        Ensures that provisional items are committed for same document navigations.
        Ensures that error pages are committed on back/forward navigations.
        Also removes unneeded sanity checks used for diagnosing the problem.

        * loader/HistoryController.cpp:
        * loader/HistoryController.h:
2011-02-04  Charlie Reis  <creis@chromium.org>

        Reviewed by Mihai Parparita.

        Crash in WebCore::HistoryController::itemsAreClones
        https://bugs.webkit.org/show_bug.cgi?id=52819

        Removes unneeded sanity checks used for diagnosing a memory error.

        * src/WebFrameImpl.cpp:

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

LayoutTests/ChangeLog
LayoutTests/fast/history/history-back-forward-within-subframe-hash-expected.txt [new file with mode: 0644]
LayoutTests/fast/history/history-back-forward-within-subframe-hash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/HistoryController.cpp
Source/WebCore/loader/HistoryController.h
Source/WebKit/chromium/ChangeLog
Source/WebKit/chromium/src/WebFrameImpl.cpp

index f768d2835b35781c80c7e5b1930e51b344f6b08b..3ec749870cf7bc8dd945aa879eab448c73771dba 100644 (file)
@@ -1,3 +1,15 @@
+2011-02-04  Charlie Reis  <creis@chromium.org>
+
+        Reviewed by Mihai Parparita.
+
+        Crash in WebCore::HistoryController::itemsAreClones
+        https://bugs.webkit.org/show_bug.cgi?id=52819
+
+        Tests that navigating back and forward between hash items works.
+
+        * fast/history/history-back-forward-within-subframe-hash.html: Added.
+        * fast/history/history-back-forward-within-subframe-hash-expected.txt: Added.
+
 2011-02-04  Dimitri Glazkov  <dglazkov@chromium.org>
 
         [Chromium] Add expectations for the test, added in r77690.
diff --git a/LayoutTests/fast/history/history-back-forward-within-subframe-hash-expected.txt b/LayoutTests/fast/history/history-back-forward-within-subframe-hash-expected.txt
new file mode 100644 (file)
index 0000000..0ebdfa4
--- /dev/null
@@ -0,0 +1,12 @@
+Ensures that repeated back and forward work for frame hash navigations.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+First visit to foo.
+Gone back.
+Gone forward.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/history/history-back-forward-within-subframe-hash.html b/LayoutTests/fast/history/history-back-forward-within-subframe-hash.html
new file mode 100644 (file)
index 0000000..dbbcec1
--- /dev/null
@@ -0,0 +1,55 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <link rel="stylesheet" href="../js/resources/js-test-style.css">
+  <script src="../js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<pre id="console"></pre>
+
+<iframe src="resources/subframe.html" id="iframe1"></iframe>
+
+<script>
+description('Ensures that repeated back and forward work for frame hash navigations.');
+
+var frame = document.getElementById("iframe1").contentWindow;
+onload = function()
+{
+    // Make sure that we can generate history entries
+    setTimeout(runTest, 0);
+}
+
+// 1. Navigate to hash "#foo".
+// 2. Go back to hash "".
+// 3. Go forward to hash "#foo".
+function runTest()
+{
+    frame.location.hash = "#foo";
+}
+
+frame.onhashchange = function()
+{
+    if (frame.location.hash == "#foo") {
+        if (!window.localStorage.beenHere) {
+            window.localStorage.beenHere = true;
+            debug("First visit to foo.");
+            history.back();
+        } else {
+            delete window.localStorage.beenHere;
+            debug("Gone forward.");
+            finishJSTest();
+        }
+    }
+    if (frame.location.hash == "") {
+        debug("Gone back.");
+        history.forward();
+    }
+}
+
+var successfullyParsed = true;
+var jsTestIsAsync = true;
+</script>  
+<script src="../js/resources/js-test-post.js"></script>
+</body>
+</html>
index 431e191877ec533767ad9e7bf069c33fdf72438c..1adcf89032fe45ac036b60b8c1889a0e7ffb38a4 100644 (file)
@@ -1,3 +1,18 @@
+2011-02-04  Charlie Reis  <creis@chromium.org>
+
+        Reviewed by Mihai Parparita.
+
+        Crash in WebCore::HistoryController::itemsAreClones
+        https://bugs.webkit.org/show_bug.cgi?id=52819
+
+        Avoids deleting the current HistoryItem while it is still in use.
+        Ensures that provisional items are committed for same document navigations.
+        Ensures that error pages are committed on back/forward navigations.
+        Also removes unneeded sanity checks used for diagnosing the problem.
+
+        * loader/HistoryController.cpp:
+        * loader/HistoryController.h:
+
 2011-02-04  Carol Szabo  <carol.szabo@nokia.com>
 
         Reviewed by David Hyatt.
index 8f29d3cdb3c42fbdcdbf61fbc34bea5bf8aef5f7..7bfe380f0b9fa8c1953c499ce264caa20fdd95e0 100644 (file)
@@ -236,7 +236,7 @@ void HistoryController::goToItem(HistoryItem* targetItem, FrameLoadType type)
     // Set the BF cursor before commit, which lets the user quickly click back/forward again.
     // - plus, it only makes sense for the top level of the operation through the frametree,
     // as opposed to happening for some/one of the page commits that might happen soon
-    HistoryItem* currentItem = page->backForward()->currentItem();    
+    RefPtr<HistoryItem> currentItem = page->backForward()->currentItem();
     page->backForward()->setCurrentItem(targetItem);
     Settings* settings = m_frame->settings();
     page->setGlobalHistoryItem((!settings || settings->privateBrowsingEnabled()) ? 0 : targetItem);
@@ -245,9 +245,9 @@ void HistoryController::goToItem(HistoryItem* targetItem, FrameLoadType type)
     // 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);
+    recursiveSetProvisionalItem(targetItem, currentItem.get(), type);
     // Now that all other frames have provisional items, do the actual navigation.
-    recursiveGoToItem(targetItem, currentItem, type);
+    recursiveGoToItem(targetItem, currentItem.get(), type);
 }
 
 void HistoryController::updateForBackForwardNavigation()
@@ -402,8 +402,9 @@ void HistoryController::updateForCommit()
         LOG(History, "WebCoreHistory: Updating History for commit in frame %s", frameLoader->documentLoader()->title().utf8().data());
 #endif
     FrameLoadType type = frameLoader->loadType();
-    if (isBackForwardLoadType(type) ||
-        ((type == FrameLoadTypeReload || type == FrameLoadTypeReloadFromOrigin) && !frameLoader->provisionalDocumentLoader()->unreachableURL().isEmpty())) {
+    if (isBackForwardLoadType(type)
+        || isReplaceLoadTypeWithProvisionalItem(type)
+        || ((type == FrameLoadTypeReload || type == FrameLoadTypeReloadFromOrigin) && !frameLoader->provisionalDocumentLoader()->unreachableURL().isEmpty())) {
         // Once committed, we want to use current item for saving DocState, and
         // the provisional item for restoring state.
         // Note previousItem must be set before we close the URL, which will
@@ -423,6 +424,13 @@ void HistoryController::updateForCommit()
     }
 }
 
+bool HistoryController::isReplaceLoadTypeWithProvisionalItem(FrameLoadType type)
+{
+    // Going back to an error page in a subframe can trigger a FrameLoadTypeReplace
+    // while m_provisionalItem is set, so we need to commit it.
+    return type == FrameLoadTypeReplace && m_provisionalItem;
+}
+
 void HistoryController::recursiveUpdateForCommit()
 {
     // The frame that navigated will now have a null provisional item.
@@ -471,6 +479,25 @@ void HistoryController::updateForSameDocumentNavigation()
         return;
 
     addVisitedLink(page, m_frame->document()->url());
+    page->mainFrame()->loader()->history()->recursiveUpdateForSameDocumentNavigation();
+}
+
+void HistoryController::recursiveUpdateForSameDocumentNavigation()
+{
+    // The frame that navigated will now have a null provisional item.
+    // Ignore it and its children.
+    if (!m_provisionalItem)
+        return;
+
+    // Commit the provisional item.
+    m_frameLoadComplete = false;
+    m_previousItem = m_currentItem;
+    m_currentItem = m_provisionalItem;
+    m_provisionalItem = 0;
+
+    // Iterate over the rest of the tree.
+    for (Frame* child = m_frame->tree()->firstChild(); child; child = child->tree()->nextSibling())
+        child->loader()->history()->recursiveUpdateForSameDocumentNavigation();
 }
 
 void HistoryController::updateForFrameLoadCompleted()
@@ -621,17 +648,6 @@ void HistoryController::recursiveSetProvisionalItem(HistoryItem* item, HistoryIt
 
         int size = childItems.size();
 
-        // Sanity checks for http://webkit.org/b/52819.
-        if (size > 0) {
-            // fromItem should have same number of children according to hasSameFrames,
-            // but crash dumps suggest it might have 0.
-            if (!fromItem->children().size())
-                CRASH();
-            // itemsAreClones checked fromItem->hasSameFrames(item). Check vice versa.
-            if (!item->hasSameFrames(fromItem))
-                CRASH();
-        }
-
         for (int i = 0; i < size; ++i) {
             String childFrameName = childItems[i]->target();
             HistoryItem* fromChildItem = fromItem->childItemWithTarget(childFrameName);
@@ -670,14 +686,6 @@ void HistoryController::recursiveGoToItem(HistoryItem* item, HistoryItem* fromIt
 
 bool HistoryController::itemsAreClones(HistoryItem* item1, HistoryItem* item2) const
 {
-    // It appears that one of the items can be null in release builds, leading
-    // to the crashes seen in http://webkit.org/b/52819.  For now, try to
-    // narrow it down with a more specific crash.
-    if (!item1)
-        CRASH();
-    if (!item2)
-        CRASH();
-
     // 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.
index 061f2356585e7098f5da915eb20cc77f116100d9..9514d2f940da1bc548680c950a95c087cbf9f7de 100644 (file)
@@ -91,7 +91,9 @@ private:
 
     void recursiveSetProvisionalItem(HistoryItem*, HistoryItem*, FrameLoadType);
     void recursiveGoToItem(HistoryItem*, HistoryItem*, FrameLoadType);
+    bool isReplaceLoadTypeWithProvisionalItem(FrameLoadType);
     void recursiveUpdateForCommit();
+    void recursiveUpdateForSameDocumentNavigation();
     bool itemsAreClones(HistoryItem*, HistoryItem*) const;
     bool currentFramesMatchItem(HistoryItem*) const;
     void updateBackForwardListClippedAtTarget(bool doClip);
index bba8815eed73e42c699bf392083649d0df58594e..985ee70ea179021afbb03f852c6148c8562a3fc8 100644 (file)
@@ -1,3 +1,14 @@
+2011-02-04  Charlie Reis  <creis@chromium.org>
+
+        Reviewed by Mihai Parparita.
+
+        Crash in WebCore::HistoryController::itemsAreClones
+        https://bugs.webkit.org/show_bug.cgi?id=52819
+
+        Removes unneeded sanity checks used for diagnosing a memory error.
+
+        * src/WebFrameImpl.cpp:
+
 2011-02-04  Daniel Cheng  <dcheng@chromium.org>
 
         Reviewed by Dmitry Titov.
index d2c06ff965ba93d4c7d602121f31a2880bf9032d..9d6129be25bf7d9233e057c78c2cc1f51c876b3b 100644 (file)
@@ -881,17 +881,6 @@ void WebFrameImpl::loadHistoryItem(const WebHistoryItem& item)
     RefPtr<HistoryItem> historyItem = PassRefPtr<HistoryItem>(item);
     ASSERT(historyItem.get());
 
-    // Sanity check for http://webkit.org/b/52819.  It appears that some child
-    // items of this item might be null.  Try validating just the first set of
-    // children in an attempt to catch it early.
-    const HistoryItemVector& childItems = historyItem->children();
-    int size = childItems.size();
-    for (int i = 0; i < size; ++i) {
-      RefPtr<HistoryItem> childItem = childItems[i].get();
-      if (!childItem.get())
-        CRASH();
-    }
-
     // If there is no currentItem, which happens when we are navigating in
     // session history after a crash, we need to manufacture one otherwise WebKit
     // hoarks. This is probably the wrong thing to do, but it seems to work.