This is currently a top crasher on the Chromium dev channel. 0x5b93e82b [chrome.dll - frameloader.cpp:2994 WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const &,WTF::PassRefPtr<WebCore::FormState>,bool) 0x5b93e434 [chrome.dll - frameloader.cpp:2875 WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy(void *,WebCore::ResourceRequest const &,WTF::PassRefPtr<WebCore::FormState>,bool) 0x5b9b1d16 [chrome.dll - policycallback.cpp:102 WebCore::PolicyCallback::call(bool) 0x5b9ae695 [chrome.dll - policychecker.cpp:160 WebCore::PolicyChecker::continueAfterNavigationPolicy(WebCore::PolicyAction) 0x5bca7bfc [chrome.dll - frameloaderclientimpl.cpp:958 WebKit::FrameLoaderClientImpl::dispatchDecidePolicyForNavigationAction(void ( WebCore::PolicyChecker::*)(WebCore::PolicyAction),WebCore::NavigationAction const &,WebCore::ResourceRequest const &,WTF::PassRefPtr<WebCore::FormState>) 0x5b9ae3a2 [chrome.dll - policychecker.cpp:88 WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const &,WebCore::DocumentLoader *,WTF::PassRefPtr<WebCore::FormState>,void (*)(void *,WebCore::ResourceRequest const &,WTF::PassRefPtr<WebCore::FormState>,bool),void *) 0x5b93bce2 [chrome.dll - frameloader.cpp:1491 WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader *,WebCore::FrameLoadType,WTF::PassRefPtr<WebCore::FormState>) 0x5b93b9ce [chrome.dll - frameloader.cpp:1408 WebCore::FrameLoader::loadWithNavigationAction(WebCore::ResourceRequest const &,WebCore::NavigationAction const &,bool,WebCore::FrameLoadType,WTF::PassRefPtr<WebCore::FormState>) 0x5b93f327 [chrome.dll - frameloader.cpp:3258 WebCore::FrameLoader::loadDifferentDocumentItem(WebCore::HistoryItem *,WebCore::FrameLoadType) 0x5b9ac494 [chrome.dll - historycontroller.cpp:683 WebCore::HistoryController::recursiveGoToItem(WebCore::HistoryItem *,WebCore::HistoryItem *,WebCore::FrameLoadType) 0x5b9aba59 [chrome.dll - historycontroller.cpp:250 WebCore::HistoryController::goToItem(WebCore::HistoryItem *,WebCore::FrameLoadType) 0x5bc8fcfe [chrome.dll - webframeimpl.cpp:899 WebKit::WebFrameImpl::loadHistoryItem(WebKit::WebHistoryItem const &) 0x5b507560 [chrome.dll - render_view.cc:1426 RenderView::OnNavigate(ViewMsg_Navigate_Params const &)
It looks like the crash is happening on this line: if (isBackForwardLoadType(type) && history()->provisionalItem()->isInPageCache()) { loadProvisionalItemFromCachedPage(); return; } It's possible history()->provisionalItem() might be null at the time.
The location of the crash in the crash dump's disassembly does seem to confirm that we've already called isBackForwardNavigation and then blow up when we access history()->provisionalItem()->isInPageCache(), which is inlined. The curious thing is that the provisional item gets set on the first line of loadDifferentDocumentItem, which is still on the call stack. That means something between there and here cleared the provisional item. The closest thing I see is the call to stopAllLoaders just above the crash, but that's explicitly called with ShouldNotClearProvisionalItem (because we know a new navigation is in progress). We started clearing provisional items in stopAllLoaders as part of http://trac.webkit.org/changeset/76357. For what it's worth, a simple null check is not sufficient to fix the problem-- that just delays the crash until later. It looks like we're expecting the provisional item to be valid here.
Eric Roman found a way to repro (posted on the corresponding Chrome bug, http://crbug.com/72458): 1. Visit http://shop.ebay.com/quickshipwarehouse/m.html?_nkw=&_armrs=1&_from=&_ipg=&_trksid=p3686 2. Click any product link and let it load fully. 3. Click the blue "Place Bid" button. 4. Click Back, then click Back again after the page has started to render but before it finishes. It turns out the product page (from step 2) does a DOMWindow::setLocation to a hash, which makes us call FrameLoader::loadInSameDocument. I recently updated that to clear the history's provisional item as part of http://trac.webkit.org/changeset/77705, since we don't want to leave it dangling in a normal same-document navigation. That leads to the crash. However, it's surprising that this can happen *within* a call to loadDifferentDocumentItem (via stopAllLoaders), as you can see deep within the stack trace below. In other words, as part of doing a back navigation, we stop the current loaders, which somehow lets the DOMWindow dispatch an event, where it then manages to set the location (even though we already have a navigation in progress). We then proceed with the back navigation logic that's already on the call stack, ending up with the crash. This nested navigation seems broken to me, but I'll admit that I don't understand enough of this flow to know whether this is the intended behavior. Even if we weren't clearing the provisional item in recursiveUpdateForSameDocumentNavigation, we'll end up confused due to the nested navigation. Any thoughts on the right way to fix this? 00 chrome_1210000!WebCore::HistoryController::recursiveUpdateForSameDocumentNavigation 01 chrome_1210000!WebCore::FrameLoader::loadInSameDocument+0x157 02 chrome_1210000!WebCore::FrameLoader::callContinueFragmentScrollAfterNavigationPolicy+0x37 03 chrome_1210000!WebCore::PolicyCallback::call+0x2b 04 chrome_1210000!WebCore::PolicyChecker::continueAfterNavigationPolicy+0xe7 05 chrome_1210000!WebKit::FrameLoaderClientImpl::dispatchDecidePolicyForNavigationAction+0x217 06 chrome_1210000!WebCore::PolicyChecker::checkNavigationPolicy+0x192 07 chrome_1210000!WebCore::FrameLoader::loadURL+0x2a9 08 chrome_1210000!WebCore::FrameLoader::loadFrameRequest+0x153 09 chrome_1210000!WebCore::FrameLoader::urlSelected+0x114 0a chrome_1210000!WebCore::FrameLoader::changeLocation+0x75 0b chrome_1210000!WebCore::NavigationScheduler::scheduleLocationChange+0xad 0c chrome_1210000!WebCore::DOMWindow::setLocation+0xe7 0d chrome_1210000!WebCore::V8Location::replaceCallback+0x65 0e chrome_1210000!v8::internal::HandleApiCallHelper<0>+0x16a 0f chrome_1210000!v8::internal::Builtin_HandleApiCall+0xf WARNING: Frame IP not in any known module. Following frames may be wrong. 10 0x1e1b028e 11 0x6c23af7 12 chrome_1210000!v8::internal::Invoke+0xf9 13 chrome_1210000!v8::internal::Execution::Call+0x25 14 chrome_1210000!v8::Function::Call+0xea 15 chrome_1210000!WebCore::V8Proxy::callFunction+0x13d 16 chrome_1210000!WebCore::V8EventListener::callListenerFunction+0x55 17 chrome_1210000!WebCore::V8AbstractEventListener::invokeEventHandler+0xdf 18 chrome_1210000!WebCore::V8AbstractEventListener::handleEvent+0x4f 19 chrome_1210000!WebCore::EventTarget::fireEventListeners+0xb6 1a chrome_1210000!WebCore::EventTarget::fireEventListeners+0x47 1b chrome_1210000!WebCore::DOMWindow::dispatchEvent+0x99 1c chrome_1210000!WebCore::DOMWindow::dispatchTimedEvent+0x31 1d chrome_1210000!WebCore::DOMWindow::dispatchLoadEvent+0x85 1e chrome_1210000!WebCore::Document::implicitClose+0xf4 1f chrome_1210000!WebCore::FrameLoader::checkCallImplicitClose+0x4d 20 chrome_1210000!WebCore::FrameLoader::checkCompleted+0x82 21 chrome_1210000!WebCore::FrameLoader::completed+0x59 22 chrome_1210000!WebCore::FrameLoader::checkCompleted+0x96 23 chrome_1210000!WebCore::FrameLoader::mainReceivedCompleteError+0x29 24 chrome_1210000!WebCore::DocumentLoader::mainReceivedError+0x41 25 chrome_1210000!WebCore::FrameLoader::receivedMainResourceError+0xd1 26 chrome_1210000!WebCore::MainResourceLoader::didCancel+0x50 27 chrome_1210000!WebCore::ResourceLoader::cancel+0x4a 28 chrome_1210000!WebCore::ResourceLoader::cancel+0x25 29 chrome_1210000!WebCore::DocumentLoader::stopLoading+0x7b 2a chrome_1210000!WebCore::FrameLoader::stopAllLoaders+0x68 2b chrome_1210000!WebCore::FrameLoader::stopLoadingSubframes+0x1d 2c chrome_1210000!WebCore::FrameLoader::stopAllLoaders+0x56 2d chrome_1210000!WebCore::FrameLoader::stopLoadingSubframes+0x1d 2e chrome_1210000!WebCore::FrameLoader::stopAllLoaders+0x56 2f chrome_1210000!WebCore::FrameLoader::continueLoadAfterNavigationPolicy+0xc8 30 chrome_1210000!WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy+0x1e 31 chrome_1210000!WebCore::PolicyCallback::call+0x2b 32 chrome_1210000!WebCore::PolicyChecker::continueAfterNavigationPolicy+0xe7 33 chrome_1210000!WebKit::FrameLoaderClientImpl::dispatchDecidePolicyForNavigationAction+0x217 34 chrome_1210000!WebCore::PolicyChecker::checkNavigationPolicy+0x192 35 chrome_1210000!WebCore::FrameLoader::loadWithDocumentLoader+0x209 36 chrome_1210000!WebCore::FrameLoader::loadWithNavigationAction+0x135 37 chrome_1210000!WebCore::FrameLoader::loadDifferentDocumentItem+0x2bc 38 chrome_1210000!WebCore::HistoryController::recursiveGoToItem+0x100 39 chrome_1210000!WebCore::HistoryController::goToItem+0x82 3a chrome_1210000!WebKit::WebFrameImpl::loadHistoryItem+0x88 3b chrome_1210000!RenderView::OnNavigate+0x1a2
(In reply to comment #3) > Any thoughts on the right way to fix this? At the very least, it seems like you'd want to protect the provisional item with a RefPtr. I ran into something similar with http://trac.webkit.org/changeset/71170.
(In reply to comment #4) > (In reply to comment #3) > > Any thoughts on the right way to fix this? > > At the very least, it seems like you'd want to protect the provisional item with a RefPtr. I ran into something similar with http://trac.webkit.org/changeset/71170. Actually, it's already protected with a RefPtr in HistoryController. I suppose we could also put it in a RefPtr in loadDifferentDocumentItem, but that wouldn't prevent this crash. In other words, the HistoryItem itself isn't being deleted and then used. The problem is that we set the HistoryController's provisional item, then start an entirely unrelated nested navigation (which sets and commits a different provisional item), and then we expect to commit the first provisional item. HistoryController isn't set up to have a stack of navigations in progress.
(In reply to comment #5) > HistoryController isn't set up to have a stack of navigations in progress. It looks like part of the problem is this part of NavigationScheduler::scheduleLocationChange // If the URL we're going to navigate to is the same as the current one, except for the // fragment part, we don't need to schedule the location change. KURL parsedURL(ParsedURLString, url); if (parsedURL.hasFragmentIdentifier() && equalIgnoringFragmentIdentifier(m_frame->document()->url(), parsedURL)) { loader->changeLocation(securityOrigin, loader->completeURL(url), referrer, lockHistory, lockBackForwardList); return; } Normally all page-initiated location changes are asynchronous, so we don't end up in this state of having more than one navigation in progress. One possible fix is to remove this special-casing of fragment changes here, and let them be handled like other navigations (which involves enqueuing a task on the NavigationScheduler queue, by the time that fires the previous navigation will be complete).
(In reply to comment #6) > Normally all page-initiated location changes are asynchronous, so we don't end up in this state of having more than one navigation in progress. One possible fix is to remove this special-casing of fragment changes here, and let them be handled like other navigations (which involves enqueuing a task on the NavigationScheduler queue, by the time that fires the previous navigation will be complete). I can confirm that change would avoid the crash, but I'm not sure it'll catch all the ways we might get there. For example, about:blank does a similar shortcut. Nate and I were considering returning early from FrameLoader::loadURL if m_inStopAllLoaders is true. There's already a check like that in FrameLoader::load, and it would be a good sanity check. The biggest problem I'm facing right now is not being able to boil this down to a reduced test case. The Ebay URL in comment 3 works reliably, but only on Windows. I've written a test that puts an onload handler on a slow iframe, navigating to #foo once onload fires. I can't seem to get it to fire, though. Instead, FrameLoader::m_isComplete is true when we get to FrameLoader::checkCompleted (unlike in the Ebay example), so we never call FrameLoader::checkCallImplicitClose. (See frame 1f in comment 3.) I should have a patch as soon as I get the test working.
Created attachment 82384 [details] Patch
(In reply to comment #8) > Created an attachment (id=82384) [details] > Patch This patch avoids the crash by ensuring we don't start a new navigation with loadURL while stopAllLoaders is in progress. It mimics a check we have in FrameLoader::load. I'm not thrilled with the test-- it can only reproduce the crash if you click the back button in the browser, and not if we use history.back() to go back. This is currently a top crasher, though, so I wanted to get the fix in review as soon as possible.
Created attachment 82399 [details] Patch
(In reply to comment #10) > Created an attachment (id=82399) [details] > Patch Here's a patch with a manual test instead, since the layout test in the previous test wouldn't have caught a regression anyway. Mihai points out that the difference between Chromium's back button and history.back() is that the latter calls Page::goToItem (which calls FrameLoader::stopAllLoaders) before HistoryController::goToItem. That prevents the crash seen in this bug. It'd be nice to get either a way to call HistoryController::goToItem without Page::goToItem via layoutTestController, or change Chromium to call Page::goToItem. However, this is currently a top crasher for Chromium, so we're hoping to get a patch in quickly. I'm happy to iterate on the test design afterward.
Comment on attachment 82399 [details] Patch This minimally-invasive change seems fine to not hold up the release, but I'd like a more through follow-up fix too (e.g. change chromium's WebFrameImpl to use Page::goToItem and/or stop loaders, and add an assert in HistoryController::goToItem that checks that loading is stopped).
Comment on attachment 82399 [details] Patch Clearing flags on attachment: 82399 Committed r78561: <http://trac.webkit.org/changeset/78561>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/78561 might have broken GTK Linux 64-bit Debug