Bug 54219 – Crash in WebCore::FrameLoader::continueLoadAfterNavigationPolicy

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Charles Reis]

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 &)

[Charles Reis]

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.

[Charles Reis]

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.

[Charles Reis]

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

[Mihai Parparita]

(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.

[Charles Reis]

(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.

[Mihai Parparita]

(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).

[Charles Reis]

(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.

[Charles Reis]

Created attachment 82384 [details]
Patch

[Charles Reis]

(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.

[Charles Reis]

Created attachment 82399 [details]
Patch

[Charles Reis]

(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.

[Mihai Parparita]

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).

[WebKit Commit Bot]

Comment on attachment 82399 [details]
Patch

Clearing flags on attachment: 82399

Committed r78561: <http://trac.webkit.org/changeset/78561>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/78561 might have broken GTK Linux 64-bit Debug