2010-11-02 Mihai Parparita <mihaip@chromium.org>
authormihaip@chromium.org <mihaip@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 Nov 2010 21:23:43 +0000 (21:23 +0000)
committermihaip@chromium.org <mihaip@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 Nov 2010 21:23:43 +0000 (21:23 +0000)
        Reviewed by Adam Barth.

        [Chromium] Crash when encountering history.back() call during Page::goToItem execution
        https://bugs.webkit.org/show_bug.cgi?id=48477

        Add a reduced version of the page that was originally reported at
        http://crbug.com/59554.

        * http/tests/history/back-during-onload-triggered-by-back-expected.txt: Added.
        * http/tests/history/back-during-onload-triggered-by-back.html: Added.
        * http/tests/history/resources/back-during-onload-container.html: Added.
        * http/tests/history/resources/back-during-onload-hung-page.php: Added.
        * http/tests/history/resources/back-during-onload-middle.html: Added.
2010-11-02  Mihai Parparita  <mihaip@chromium.org>

        Reviewed by Adam Barth.

        [Chromium] Crash when encountering history.back() call during Page::goToItem execution
        https://bugs.webkit.org/show_bug.cgi?id=48477

        For the Chromium port, BackForwardList::itemAtIndex synthesizes a
        HistoryItem and saves a pointer to it in m_pendingItem. During
        Page::goToItem we call FrameLoader::stopAllLoaders, which can trigger
        onload handlers (if a subframe was not considered committed by the frame
        loader). If one of those handlers calls calls history.back() or another
        operation that ends up in NavigationScheduler::scheduleHistoryNavigation,
        we would call BackForwardList::itemAtIndex, which means that we would
        lose the m_pendingItem RefPtr that pointed to the item being navigated
        to, causing its ref count to go to 0*, and thus for the HistoryItem to
        be deleted before we were done navigating to it.

        This is fixed in two ways:
        - Add a protector RefPtr in Page::goToItem to make sure that the item is
          still around for when we pass it to HistoryController:goToItem.
        - Change NavigationScheduler::scheduleHistoryNavigation to not use
          BackForwardList::itemAtIndex and instead look at the
          forward/backListCount() (since it doesn't actually care about the
          returned HistoryItem).

        * Full annotated stack trace of this is at http://crbug.com/59554#c9.

        Test: http/tests/history/back-during-onload-triggered-by-back.html

        * loader/NavigationScheduler.cpp:
        (WebCore::NavigationScheduler::scheduleHistoryNavigation):
        * page/Page.cpp:
        (WebCore::Page::goToItem):

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

LayoutTests/ChangeLog
LayoutTests/http/tests/history/back-during-onload-triggered-by-back-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/history/back-during-onload-triggered-by-back.html [new file with mode: 0644]
LayoutTests/http/tests/history/resources/back-during-onload-container.html [new file with mode: 0644]
LayoutTests/http/tests/history/resources/back-during-onload-hung-page.php [new file with mode: 0644]
LayoutTests/http/tests/history/resources/back-during-onload-middle.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/loader/NavigationScheduler.cpp
WebCore/page/Page.cpp

index cf4f8e1..4dc1951 100644 (file)
@@ -1,3 +1,19 @@
+2010-11-02  Mihai Parparita  <mihaip@chromium.org>
+
+        Reviewed by Adam Barth.
+
+        [Chromium] Crash when encountering history.back() call during Page::goToItem execution
+        https://bugs.webkit.org/show_bug.cgi?id=48477
+        
+        Add a reduced version of the page that was originally reported at
+        http://crbug.com/59554.
+
+        * http/tests/history/back-during-onload-triggered-by-back-expected.txt: Added.
+        * http/tests/history/back-during-onload-triggered-by-back.html: Added.
+        * http/tests/history/resources/back-during-onload-container.html: Added.
+        * http/tests/history/resources/back-during-onload-hung-page.php: Added.
+        * http/tests/history/resources/back-during-onload-middle.html: Added.
+
 2010-11-02  Dmitry Titov  <dimich@chromium.org>
 
         [Chromium] Unreviewed update of test expectations.
diff --git a/LayoutTests/http/tests/history/back-during-onload-triggered-by-back-expected.txt b/LayoutTests/http/tests/history/back-during-onload-triggered-by-back-expected.txt
new file mode 100644 (file)
index 0000000..3b416e9
--- /dev/null
@@ -0,0 +1,10 @@
+CONSOLE MESSAGE: line 23: Starting test.
+Tests that an onload handler that runs history.back() that's triggered by a history.back() doesn't crash the browser (see http://crbug.com/59554 for more details).
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/history/back-during-onload-triggered-by-back.html b/LayoutTests/http/tests/history/back-during-onload-triggered-by-back.html
new file mode 100644 (file)
index 0000000..eeca21d
--- /dev/null
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <link rel="stylesheet" href="../../js-test-resources/js-test-style.css">
+  <script src="../../js-test-resources/js-test-pre.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+
+<script>
+description('Tests that an onload handler that runs history.back() that\'s triggered by a history.back() doesn\'t crash the browser (see http://crbug.com/59554 for more details).');
+
+onload = function()
+{
+  if (window.localStorage.started) {
+      delete window.localStorage.started;
+      finishJSTest();
+  } else {
+      // To make sure that we hit this branch, log this to the console so that 
+      // it shows up in expected output (debug() will be blown away once we
+      // navigate out).
+      console.log('Starting test.');
+      window.localStorage.started = true;
+      // Navigate in a timeout to make sure we create a history entry.
+      setTimeout(function() {
+        window.location.href = 'resources/back-during-onload-container.html';
+      }, 0);
+ }
+};
+
+var successfullyParsed = true;
+var jsTestIsAsync = true;
+</script> 
+
+<script src="../../js-test-resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/history/resources/back-during-onload-container.html b/LayoutTests/http/tests/history/resources/back-during-onload-container.html
new file mode 100644 (file)
index 0000000..398f16a
--- /dev/null
@@ -0,0 +1,8 @@
+<script>
+onload = function() {
+  history.back();
+};
+</script>
+container
+<iframe src="back-during-onload-middle.html"></iframe>
+<script>history.back();</script>
\ No newline at end of file
diff --git a/LayoutTests/http/tests/history/resources/back-during-onload-hung-page.php b/LayoutTests/http/tests/history/resources/back-during-onload-hung-page.php
new file mode 100644 (file)
index 0000000..6031615
--- /dev/null
@@ -0,0 +1,2 @@
+<? sleep(1000); ?>
+FAIL: Should never see this
\ No newline at end of file
diff --git a/LayoutTests/http/tests/history/resources/back-during-onload-middle.html b/LayoutTests/http/tests/history/resources/back-during-onload-middle.html
new file mode 100644 (file)
index 0000000..6035e40
--- /dev/null
@@ -0,0 +1,2 @@
+middle<br>
+<iframe src="back-during-onload-hung-page.php"></iframe>
index 1ec2c30..e08773b 100644 (file)
@@ -1,3 +1,38 @@
+2010-11-02  Mihai Parparita  <mihaip@chromium.org>
+
+        Reviewed by Adam Barth.
+
+        [Chromium] Crash when encountering history.back() call during Page::goToItem execution
+        https://bugs.webkit.org/show_bug.cgi?id=48477
+        
+        For the Chromium port, BackForwardList::itemAtIndex synthesizes a 
+        HistoryItem and saves a pointer to it in m_pendingItem. During 
+        Page::goToItem we call FrameLoader::stopAllLoaders, which can trigger
+        onload handlers (if a subframe was not considered committed by the frame
+        loader). If one of those handlers calls calls history.back() or another
+        operation that ends up in NavigationScheduler::scheduleHistoryNavigation,
+        we would call BackForwardList::itemAtIndex, which means that we would 
+        lose the m_pendingItem RefPtr that pointed to the item being navigated 
+        to, causing its ref count to go to 0*, and thus for the HistoryItem to
+        be deleted before we were done navigating to it.
+        
+        This is fixed in two ways:
+        - Add a protector RefPtr in Page::goToItem to make sure that the item is
+          still around for when we pass it to HistoryController:goToItem.
+        - Change NavigationScheduler::scheduleHistoryNavigation to not use
+          BackForwardList::itemAtIndex and instead look at the
+          forward/backListCount() (since it doesn't actually care about the
+          returned HistoryItem).
+        
+        * Full annotated stack trace of this is at http://crbug.com/59554#c9.        
+
+        Test: http/tests/history/back-during-onload-triggered-by-back.html
+
+        * loader/NavigationScheduler.cpp:
+        (WebCore::NavigationScheduler::scheduleHistoryNavigation):
+        * page/Page.cpp:
+        (WebCore::Page::goToItem):
+
 2010-10-28  Zhenyao Mo  <zmo@google.com>
 
         Reviewed by Kenneth Russell.
index 66e6bac..28fda9a 100644 (file)
@@ -352,8 +352,8 @@ void NavigationScheduler::scheduleHistoryNavigation(int steps)
 
     // Invalid history navigations (such as history.forward() during a new load) have the side effect of cancelling any scheduled
     // redirects. We also avoid the possibility of cancelling the current load by avoiding the scheduled redirection altogether.
-    HistoryItem* specifiedEntry = m_frame->page()->backForward()->itemAtIndex(steps);
-    if (!specifiedEntry) {
+    BackForwardController* backForward = m_frame->page()->backForward();
+    if (steps > backForward->forwardCount() || -steps > backForward->backCount()) {
         cancel();
         return;
     }
index 3b546e9..746e53e 100644 (file)
@@ -347,6 +347,10 @@ void Page::goToItem(HistoryItem* item, FrameLoadType type)
 {
     if (defersLoading())
         return;
+
+    // stopAllLoaders may end up running onload handlers, which could cause further history traversals that may lead to the passed in HistoryItem
+    // being deref()-ed. Make sure we can still use it with HistoryController::goToItem later.
+    RefPtr<HistoryItem> protector(item);
     
     // Abort any current load unless we're navigating the current document to a new state object
     HistoryItem* currentItem = m_mainFrame->loader()->history()->currentItem();