Changeset 71368 in webkit


Ignore:
Timestamp:
Nov 4, 2010 5:08:27 PM (13 years ago)
Author:
mihaip@chromium.org
Message:

Merge 71170 - 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 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).

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

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

TBR=​mihaip@chromium.org

Location:
branches/chromium/552
Files:
4 edited
5 copied

Legend:

Unmodified
Added
Removed

  • branches/chromium/552/LayoutTests/ChangeLog

    r70817 r71368  
     12010-11-02  Mihai Parparita  <mihaip@chromium.org>
     2
     3        Reviewed by Adam Barth.
     4
     5        [Chromium] Crash when encountering history.back() call during Page::goToItem execution
     6        https://bugs.webkit.org/show_bug.cgi?id=48477
     7       
     8        Add a reduced version of the page that was originally reported at
     9        http://crbug.com/59554.
     10
     11        * http/tests/history/back-during-onload-triggered-by-back-expected.txt: Added.
     12        * http/tests/history/back-during-onload-triggered-by-back.html: Added.
     13        * http/tests/history/resources/back-during-onload-container.html: Added.
     14        * http/tests/history/resources/back-during-onload-hung-page.php: Added.
     15        * http/tests/history/resources/back-during-onload-middle.html: Added.
     16
    1172010-10-27  Andy Estes  <aestes@apple.com>
    218

  • branches/chromium/552/WebCore/ChangeLog

    r70817 r71368  
     12010-11-02  Mihai Parparita  <mihaip@chromium.org>
     2
     3        Reviewed by Adam Barth.
     4
     5        [Chromium] Crash when encountering history.back() call during Page::goToItem execution
     6        https://bugs.webkit.org/show_bug.cgi?id=48477
     7       
     8        For the Chromium port, BackForwardList::itemAtIndex synthesizes a
     9        HistoryItem and saves a pointer to it in m_pendingItem. During
     10        Page::goToItem we call FrameLoader::stopAllLoaders, which can trigger
     11        onload handlers (if a subframe was not considered committed by the frame
     12        loader). If one of those handlers calls calls history.back() or another
     13        operation that ends up in NavigationScheduler::scheduleHistoryNavigation,
     14        we would call BackForwardList::itemAtIndex, which means that we would
     15        lose the m_pendingItem RefPtr that pointed to the item being navigated
     16        to, causing its ref count to go to 0*, and thus for the HistoryItem to
     17        be deleted before we were done navigating to it.
     18       
     19        This is fixed in two ways:
     20        - Add a protector RefPtr in Page::goToItem to make sure that the item is
     21          still around for when we pass it to HistoryController:goToItem.
     22        - Change NavigationScheduler::scheduleHistoryNavigation to not use
     23          BackForwardList::itemAtIndex and instead look at the
     24          forward/backListCount() (since it doesn't actually care about the
     25          returned HistoryItem).
     26       
     27        * Full annotated stack trace of this is at http://crbug.com/59554#c9.       
     28
     29        Test: http/tests/history/back-during-onload-triggered-by-back.html
     30
     31        * loader/NavigationScheduler.cpp:
     32        (WebCore::NavigationScheduler::scheduleHistoryNavigation):
     33        * page/Page.cpp:
     34        (WebCore::Page::goToItem):
     35
    1362010-10-27  Andy Estes  <aestes@apple.com>
    237

  • branches/chromium/552/WebCore/loader/NavigationScheduler.cpp

    r71202 r71368  
    342342    // Invalid history navigations (such as history.forward() during a new load) have the side effect of cancelling any scheduled
    343343    // redirects. We also avoid the possibility of cancelling the current load by avoiding the scheduled redirection altogether.
    344     HistoryItem* specifiedEntry = m_frame->page()->backForwardList()->itemAtIndex(steps);
    345     if (!specifiedEntry) {
     344    BackForwardList* backForwardList = m_frame->page()->backForwardList();
     345    if (steps > backForwardList->forwardListCount() || -steps > backForwardList->backListCount()) {
    346346        cancel();
    347347        return;

  • branches/chromium/552/WebCore/page/Page.cpp

    r71202 r71368  
    348348    if (defersLoading())
    349349        return;
     350
     351    // stopAllLoaders may end up running onload handlers, which could cause further history traversals that may lead to the passed in HistoryItem
     352    // being deref()-ed. Make sure we can still use it with HistoryController::goToItem later.
     353    RefPtr<HistoryItem> protector(item);
    350354   
    351355    // Abort any current load unless we're navigating the current document to a new state object
Note: See TracChangeset for help on using the changeset viewer.