Bug 33830 – assertion failure calling history.pushState within popstate event handler

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Darin Fisher (:fishd, Google)]

Created attachment 46892 [details]
test case

assertion failure calling history.pushState within popstate event handler

call stack:

>	test_shell.exe!WebCore::FrameLoader::navigateWithinDocument(WebCore::HistoryItem * item=0x096e80f8)  Line 3707 + 0x30 bytes	C++
 	test_shell.exe!WebCore::FrameLoader::loadItem(WebCore::HistoryItem * item=0x096e80f8, WebCore::FrameLoadType loadType=FrameLoadTypeIndexedBackForward)  Line 3830	C++
 	test_shell.exe!WebCore::HistoryController::recursiveGoToItem(WebCore::HistoryItem * item=0x096e80f8, WebCore::HistoryItem * fromItem=0x008cc148, WebCore::FrameLoadType type=FrameLoadTypeIndexedBackForward)  Line 587	C++
 	test_shell.exe!WebCore::HistoryController::goToItem(WebCore::HistoryItem * targetItem=0x096e80f8, WebCore::FrameLoadType type=FrameLoadTypeIndexedBackForward)  Line 228	C++
 	test_shell.exe!WebKit::WebFrameImpl::loadHistoryItem(const WebKit::WebHistoryItem & item={...})  Line 759	C++
 	test_shell.exe!TestShell::Navigate(const TestNavigationEntry & entry={...}, bool reload=false)  Line 627 + 0x2b bytes	C++
 	test_shell.exe!TestNavigationController::NavigateToPendingEntry(bool reload=false)  Line 219 + 0x16 bytes	C++
 	test_shell.exe!TestNavigationController::GoToIndex(int index=2)  Line 87	C++
 	test_shell.exe!TestNavigationController::GoToOffset(int offset=-1)  Line 77	C++
 	test_shell.exe!TestWebViewDelegate::navigateBackForwardSoon(int offset=-1)  Line 552	C++
 	test_shell.exe!WebKit::FrameLoaderClientImpl::handleBackForwardNavigation(const WebCore::KURL & url={...})  Line 1464 + 0x21 bytes	C++
 	test_shell.exe!WebKit::FrameLoaderClientImpl::dispatchDecidePolicyForNavigationAction(void (WebCore::PolicyAction)* function=0x01f0acd0, const WebCore::NavigationAction & action={...}, const WebCore::ResourceRequest & request={...}, WTF::PassRefPtr<WebCore::FormState> formState={...})  Line 899	C++
 	test_shell.exe!WebCore::PolicyChecker::checkNavigationPolicy(const WebCore::ResourceRequest & request={...}, WebCore::DocumentLoader * loader=0x097016d8, WTF::PassRefPtr<WebCore::FormState> formState={...}, void (void *, const WebCore::ResourceRequest &, WTF::PassRefPtr<WebCore::FormState>, bool)* function=0x01ad5030, void * argument=0x0083e478)  Line 89	C++
 	test_shell.exe!WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader * loader=0x097016d8, WebCore::FrameLoadType type=FrameLoadTypeIndexedBackForward, WTF::PassRefPtr<WebCore::FormState> prpFormState={...})  Line 2087	C++
 	test_shell.exe!WebCore::FrameLoader::loadWithNavigationAction(const WebCore::ResourceRequest & request={...}, const WebCore::NavigationAction & action={...}, bool lockHistory=false, WebCore::FrameLoadType type=FrameLoadTypeIndexedBackForward, WTF::PassRefPtr<WebCore::FormState> formState={...})  Line 2010	C++
 	test_shell.exe!WebCore::FrameLoader::navigateToDifferentDocument(WebCore::HistoryItem * item=0x008cb890, WebCore::FrameLoadType loadType=FrameLoadTypeIndexedBackForward)  Line 3806	C++
 	test_shell.exe!WebCore::FrameLoader::loadItem(WebCore::HistoryItem * item=0x008cb890, WebCore::FrameLoadType loadType=FrameLoadTypeIndexedBackForward)  Line 3832	C++
 	test_shell.exe!WebCore::HistoryController::recursiveGoToItem(WebCore::HistoryItem * item=0x008cb890, WebCore::HistoryItem * fromItem=0x008cc148, WebCore::FrameLoadType type=FrameLoadTypeIndexedBackForward)  Line 587	C++
 	test_shell.exe!WebCore::HistoryController::goToItem(WebCore::HistoryItem * targetItem=0x008cb890, WebCore::FrameLoadType type=FrameLoadTypeIndexedBackForward)  Line 228	C++
 	test_shell.exe!WebCore::Page::goToItem(WebCore::HistoryItem * item=0x008cb890, WebCore::FrameLoadType type=FrameLoadTypeIndexedBackForward)  Line 311	C++
 	test_shell.exe!WebCore::Page::goBackOrForward(int distance=-1)  Line 288	C++
 	test_shell.exe!WebCore::RedirectScheduler::timerFired(WebCore::Timer<WebCore::RedirectScheduler> * __formal=0x0083e790)  Line 302	C++
 	test_shell.exe!WebCore::Timer<WebCore::RedirectScheduler>::fired()  Line 98 + 0x23 bytes	C++
 	test_shell.exe!WebCore::ThreadTimers::sharedTimerFiredInternal()  Line 112 + 0xf bytes	C++
 	test_shell.exe!WebCore::ThreadTimers::sharedTimerFired()  Line 91	C++

code snippet:

    history()->setCurrentItem(item);
        
    // loadInSameDocument() actually changes the URL and notifies load delegates of a "fake" load
    loadInSameDocument(item->url(), item->stateObject(), false);

    // Restore user view state from the current history item here since we don't do a normal load.
    // Even though we just manually set the current history item, this ASSERT verifies nothing 
    // inside of loadInSameDocument() caused it to change.
    ASSERT(history()->currentItem() == item);

^^^ within loadInSameDocument, we dispatch the popstate event.  the test case, calls pushState, which has the effect of synchronously changing the current HistoryItem.

[Brady Eidson]

Coincidentally enough I'd already found this in my WIP for https://bugs.webkit.org/show_bug.cgi?id=33538 and was just going to include removing the ASSERT in that patch.

We might as well just do a one-liner to remove the assertion here.

[Brady Eidson]

There is a real bug the ASSERT is trying to describe.  In this case, if there was meaningful state to restore, then we'd end up restoring it from the wrong item.

That said, I'd argue that is a minor edge case bug compared to this ASSERTion causing constant grief for people working heavily in this area (which I certainly am).

Filed https://bugs.webkit.org/show_bug.cgi?id=33931 to follow up with the real release build symptom.  Patch to remove this assert is upcoming.

[Brady Eidson]

Created attachment 47089 [details]
Patch

[Brady Eidson]

http://trac.webkit.org/changeset/53590