Bug 44217 - Assertion failure when going back inside frame during onload
Summary: Assertion failure when going back inside frame during onload
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mihai Parparita
URL: http://persistent.info/webkit/test-ca...
Depends on:
Reported: 2010-08-18 16:53 PDT by Mihai Parparita
Modified: 2010-08-19 20:57 PDT (History)
3 users (show)

See Also:

Patch (9.31 KB, patch)
2010-08-19 16:47 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2010-08-18 16:53:43 PDT
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
Comment 1 Mihai Parparita 2010-08-18 17:10:53 PDT
+Brady since this only happens in Safari/DRT, not in Chromium/TestShell, presumably because HISTORY_ALWAYS_ASYNC is set to 1 in the latter.
Comment 2 Mihai Parparita 2010-08-18 18:41:50 PDT
As best as I can tell, this is the ordering of what's happening when history.back() is synchronous:

        (skipping over JS bindings/JIT): History::back
                check that m_previousItem is cleared             <--
      calls checkLoadCompleteForThisFrame()
            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?
Comment 3 Mihai Parparita 2010-08-19 16:47:37 PDT
Created attachment 64910 [details]
Comment 4 Mihai Parparita 2010-08-19 16:51:45 PDT
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?
Comment 5 WebKit Commit Bot 2010-08-19 20:57:37 PDT
Comment on attachment 64910 [details]

Clearing flags on attachment: 64910

Committed r65724: <http://trac.webkit.org/changeset/65724>
Comment 6 WebKit Commit Bot 2010-08-19 20:57:42 PDT
All reviewed patches have been landed.  Closing bug.