Changeset 77705 in webkit


Ignore:
Timestamp:
Feb 4, 2011, 4:48:31 PM (14 years ago)
Author:
commit-queue@webkit.org
Message:

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 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:
Location:
trunk
Files:
2 added
6 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r77704 r77705  
     12011-02-04  Charlie Reis  <creis@chromium.org>
     2
     3        Reviewed by Mihai Parparita.
     4
     5        Crash in WebCore::HistoryController::itemsAreClones
     6        https://bugs.webkit.org/show_bug.cgi?id=52819
     7
     8        Tests that navigating back and forward between hash items works.
     9
     10        * fast/history/history-back-forward-within-subframe-hash.html: Added.
     11        * fast/history/history-back-forward-within-subframe-hash-expected.txt: Added.
     12
    1132011-02-04  Dimitri Glazkov  <dglazkov@chromium.org>
    214
  • trunk/Source/WebCore/ChangeLog

    r77702 r77705  
     12011-02-04  Charlie Reis  <creis@chromium.org>
     2
     3        Reviewed by Mihai Parparita.
     4
     5        Crash in WebCore::HistoryController::itemsAreClones
     6        https://bugs.webkit.org/show_bug.cgi?id=52819
     7
     8        Avoids deleting the current HistoryItem while it is still in use.
     9        Ensures that provisional items are committed for same document navigations.
     10        Ensures that error pages are committed on back/forward navigations.
     11        Also removes unneeded sanity checks used for diagnosing the problem.
     12
     13        * loader/HistoryController.cpp:
     14        * loader/HistoryController.h:
     15
    1162011-02-04  Carol Szabo  <carol.szabo@nokia.com>
    217
  • trunk/Source/WebCore/loader/HistoryController.cpp

    r77210 r77705  
    237237    // - plus, it only makes sense for the top level of the operation through the frametree,
    238238    // as opposed to happening for some/one of the page commits that might happen soon
    239     HistoryItem* currentItem = page->backForward()->currentItem();   
     239    RefPtr<HistoryItem> currentItem = page->backForward()->currentItem();
    240240    page->backForward()->setCurrentItem(targetItem);
    241241    Settings* settings = m_frame->settings();
     
    246246    // navigations can commit immediately (such as about:blank).  We must be sure that
    247247    // all frames have provisional items set before the commit.
    248     recursiveSetProvisionalItem(targetItem, currentItem, type);
     248    recursiveSetProvisionalItem(targetItem, currentItem.get(), type);
    249249    // Now that all other frames have provisional items, do the actual navigation.
    250     recursiveGoToItem(targetItem, currentItem, type);
     250    recursiveGoToItem(targetItem, currentItem.get(), type);
    251251}
    252252
     
    403403#endif
    404404    FrameLoadType type = frameLoader->loadType();
    405     if (isBackForwardLoadType(type) ||
    406         ((type == FrameLoadTypeReload || type == FrameLoadTypeReloadFromOrigin) && !frameLoader->provisionalDocumentLoader()->unreachableURL().isEmpty())) {
     405    if (isBackForwardLoadType(type)
     406        || isReplaceLoadTypeWithProvisionalItem(type)
     407        || ((type == FrameLoadTypeReload || type == FrameLoadTypeReloadFromOrigin) && !frameLoader->provisionalDocumentLoader()->unreachableURL().isEmpty())) {
    407408        // Once committed, we want to use current item for saving DocState, and
    408409        // the provisional item for restoring state.
     
    424425}
    425426
     427bool HistoryController::isReplaceLoadTypeWithProvisionalItem(FrameLoadType type)
     428{
     429    // Going back to an error page in a subframe can trigger a FrameLoadTypeReplace
     430    // while m_provisionalItem is set, so we need to commit it.
     431    return type == FrameLoadTypeReplace && m_provisionalItem;
     432}
     433
    426434void HistoryController::recursiveUpdateForCommit()
    427435{
     
    472480
    473481    addVisitedLink(page, m_frame->document()->url());
     482    page->mainFrame()->loader()->history()->recursiveUpdateForSameDocumentNavigation();
     483}
     484
     485void HistoryController::recursiveUpdateForSameDocumentNavigation()
     486{
     487    // The frame that navigated will now have a null provisional item.
     488    // Ignore it and its children.
     489    if (!m_provisionalItem)
     490        return;
     491
     492    // Commit the provisional item.
     493    m_frameLoadComplete = false;
     494    m_previousItem = m_currentItem;
     495    m_currentItem = m_provisionalItem;
     496    m_provisionalItem = 0;
     497
     498    // Iterate over the rest of the tree.
     499    for (Frame* child = m_frame->tree()->firstChild(); child; child = child->tree()->nextSibling())
     500        child->loader()->history()->recursiveUpdateForSameDocumentNavigation();
    474501}
    475502
     
    621648
    622649        int size = childItems.size();
    623 
    624         // Sanity checks for http://webkit.org/b/52819.
    625         if (size > 0) {
    626             // fromItem should have same number of children according to hasSameFrames,
    627             // but crash dumps suggest it might have 0.
    628             if (!fromItem->children().size())
    629                 CRASH();
    630             // itemsAreClones checked fromItem->hasSameFrames(item). Check vice versa.
    631             if (!item->hasSameFrames(fromItem))
    632                 CRASH();
    633         }
    634650
    635651        for (int i = 0; i < size; ++i) {
     
    671687bool HistoryController::itemsAreClones(HistoryItem* item1, HistoryItem* item2) const
    672688{
    673     // It appears that one of the items can be null in release builds, leading
    674     // to the crashes seen in http://webkit.org/b/52819.  For now, try to
    675     // narrow it down with a more specific crash.
    676     if (!item1)
    677         CRASH();
    678     if (!item2)
    679         CRASH();
    680 
    681689    // If the item we're going to is a clone of the item we're at, then we do
    682690    // not need to load it again.  The current frame tree and the frame tree
  • trunk/Source/WebCore/loader/HistoryController.h

    r76248 r77705  
    9292    void recursiveSetProvisionalItem(HistoryItem*, HistoryItem*, FrameLoadType);
    9393    void recursiveGoToItem(HistoryItem*, HistoryItem*, FrameLoadType);
     94    bool isReplaceLoadTypeWithProvisionalItem(FrameLoadType);
    9495    void recursiveUpdateForCommit();
     96    void recursiveUpdateForSameDocumentNavigation();
    9597    bool itemsAreClones(HistoryItem*, HistoryItem*) const;
    9698    bool currentFramesMatchItem(HistoryItem*) const;
  • trunk/Source/WebKit/chromium/ChangeLog

    r77687 r77705  
     12011-02-04  Charlie Reis  <creis@chromium.org>
     2
     3        Reviewed by Mihai Parparita.
     4
     5        Crash in WebCore::HistoryController::itemsAreClones
     6        https://bugs.webkit.org/show_bug.cgi?id=52819
     7
     8        Removes unneeded sanity checks used for diagnosing a memory error.
     9
     10        * src/WebFrameImpl.cpp:
     11
    1122011-02-04  Daniel Cheng  <dcheng@chromium.org>
    213
  • trunk/Source/WebKit/chromium/src/WebFrameImpl.cpp

    r77687 r77705  
    882882    ASSERT(historyItem.get());
    883883
    884     // Sanity check for http://webkit.org/b/52819.  It appears that some child
    885     // items of this item might be null.  Try validating just the first set of
    886     // children in an attempt to catch it early.
    887     const HistoryItemVector& childItems = historyItem->children();
    888     int size = childItems.size();
    889     for (int i = 0; i < size; ++i) {
    890       RefPtr<HistoryItem> childItem = childItems[i].get();
    891       if (!childItem.get())
    892         CRASH();
    893     }
    894 
    895884    // If there is no currentItem, which happens when we are navigating in
    896885    // session history after a crash, we need to manufacture one otherwise WebKit
Note: See TracChangeset for help on using the changeset viewer.