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
+Brady since this only happens in Safari/DRT, not in Chromium/TestShell, presumably because HISTORY_ALWAYS_ASYNC is set to 1 in the latter.
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?
Created attachment 64910 [details] Patch
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 on attachment 64910 [details] Patch Clearing flags on attachment: 64910 Committed r65724: <http://trac.webkit.org/changeset/65724>
All reviewed patches have been landed. Closing bug.