Bug 44217 – Assertion failure when going back inside frame during onload

• Status: RESOLVED
• Resolution: FIXED
• Component: History

[Mihai Parparita]

While working on bug 44183, I ran into another assert with navigation inside iframes. http://persistent.info/webkit/test-cases/back-button-crash/history-back-within-subframe-url.html  triggers this assertion:

ASSERTION FAILED: !m_previousItem
(/Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/loader/HistoryController.cpp:567 void WebCore::HistoryController::recursiveGoToItem(WebCore::HistoryItem*, WebCore::HistoryItem*, WebCore::FrameLoadType))

Stack trace:

WebCore::HistoryController::recursiveGoToItem (this=0x10702d5c8, item=0x10615bc00, fromItem=0x106193c40, type=WebCore::FrameLoadTypeIndexedBackForward) at /WebCore/loader/HistoryController.cpp:567
WebCore::HistoryController::goToItem (this=0x10702d5c8, targetItem=0x10615bc00, type=WebCore::FrameLoadTypeIndexedBackForward) at /WebCore/loader/HistoryController.cpp:238
WebCore::RedirectScheduler::scheduleHistoryNavigation (this=0x10702d7f0, steps=-1) at /WebCore/loader/RedirectScheduler.cpp:349
WebCore::History::back (this=0x10618dee0) at /WebCore/page/History.cpp:66
WebCore::jsHistoryPrototypeFunctionBack (exec=0x11c80c0c0) at /WebKitBuild/Debug/DerivedSources/WebCore/JSHistory.cpp:145
?? ()
JSC::JITCode::execute (this=0x1061ab958, registerFile=0x10615e1e8, callFrame=0x11c80c040, globalData=0x106878600, exception=0x106879f48) at JITCode.h:77

This happens when calling parent.onFrameLoaded() once the inner frame navigates, which in turn calls history.back(). The triggering conditions are:
- The history.back() call is in the parent, not in the iframe
- The call happens immediately, not in a 0 interval timeout

[Mihai Parparita]

+Brady since this only happens in Safari/DRT, not in Chromium/TestShell, presumably because HISTORY_ALWAYS_ASYNC is set to 1 in the latter.

[Mihai Parparita]

As best as I can tell, this is the ordering of what's happening when history.back() is synchronous:

FrameLoader::checkCompleted()
 checkCallImplicitClose()
    Document::implicitClose
      dispatchWindowLoadEvent
        (skipping over JS bindings/JIT): History::back
           RedirectScheduler::scheduleHistoryNavigation
            HistoryController::goToItem
              HistoryController::recursiveGoToItem
                check that m_previousItem is cleared             <--
  checkLoadComplete()
    recursiveCheckLoadComplete()
      calls checkLoadCompleteForThisFrame()
        frameLoadCompleted()
          history()->updateForFrameLoadCompleted()
            clears m_previousItem

The check indicated by the <-- happens before the actual clearing.

One possible fix is for the "goToItem should be called synchronously" check in scheduleHistoryNavigation to not just look at the documentSequenceNumber of the root of the HistoryItem, but also to recursively compare all of the children frames' itemSequenceNumbers, so that we only do the call synchronously if we really are doing just a "state object traversal or a fragment traversal" (as the comment says).

However, I'm not sure what the intention is with the synchronous history.back(). I spent some time reading bug 33538, but it was also interesting to see that all the fast/history/stateobjects layout tests still pass even with asynchronous history.back(). Was there a reason why the original test case in the bug was never landed?

[Mihai Parparita]

Created attachment 64910 [details]
Patch

[Mihai Parparita]

The attached patch fixes this assert by (I believe) staying close to the sprit of the "navigation should be synchronous" check in RedirectScheduler::scheduleHistoryNavigation by also having it check the document sequence numbers. The test cases no longer crashes.

Brady, do you have time to review this?

[WebKit Commit Bot]

Comment on attachment 64910 [details]
Patch

Clearing flags on attachment: 64910

Committed r65724: <http://trac.webkit.org/changeset/65724>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.