Bug 54517 – Ensure loading has stopped in HistoryController::goToItem

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Charles Reis]

Chromium has encountered multiple crashes because it can call HistoryController::goToItem directly from WebFrameImpl::loadHistoryItem without stopping the current loaders first.  This leads to differing behavior between the back button and history.back() (which calls stopAllLoaders in Page::goToItem first), as well as between Chromium and Safari.

We should call FrameLoader::stopAllLoaders first, possibly by calling Page::goToItem instead of HistoryController::goToItem.  We can ensure all WebKit ports use this behavior by asserting that the FrameLoader is not loading when we get to HistoryController::goToItem.

Examples of bugs we've seen that would have been prevented by this:
https://bugs.webkit.org/show_bug.cgi?id=54219 - Navigation during an onload handler triggered by a back navigation.
https://bugs.webkit.org/show_bug.cgi?id=48812 - Back twice without committing.

[Charles Reis]

Created attachment 82565 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 82565 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82565&action=review

> Source/WebKit/chromium/src/WebFrameImpl.cpp:896
> +    m_frame->page()->goToItem(historyItem.get(),

Are you sure that back/forward navigations are supposed to stop all loaders first?

What if the back/forward navigation is just a reference fragment navigation?  What
do other browsers do in that case?

Please see http://trac.webkit.org/changeset/55626.  This would appear to be
undoing that change.

[Darin Fisher (:fishd, Google)]

Oh, nevermind... I see that Page::goToItem special cases navigations that do not change the document!  Good :-)

[Darin Fisher (:fishd, Google)]

Comment on attachment 82565 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82565&action=review

> Source/WebCore/loader/HistoryController.cpp:225
> +    ASSERT(!m_frame->loader()->isLoading());

so i think this assertion will fail when goToItem is called to just jump to a different reference fragment.

[Charles Reis]

(In reply to comment #3)
> Oh, nevermind... I see that Page::goToItem special cases navigations that do not change the document!  Good :-)

Interesting point!  Page::goToItem actually only special cases state navigations, not fragment navigations.  As a result, we do end up stopping all loaders even if you're just going forward to a fragment.  This is visible in Safari, as well as in Chromium with my patch.  Repro steps:

1. Start at a simple page like about:blank.
2. Go to a page with an onload handler plus a slow, uncacheable resource (e.g., in an img tag).
3. Go to a fragment on that page.
4. Go back twice to the first page.
5. Click forward, then forward again before the onload handler fires.

The onload handler will never fire.

I'd argue that we should change Page::goToItem to avoid stopping all loaders for a fragment navigation, which would fix this in both Safari and this Chromium patch.  (Note that Page::goToItem already makes a similar exception for the database thread.)

And yes, this would require changing or removing the assert in HistoryController.

Sound reasonable?

[Charles Reis]

Created attachment 82860 [details]
Patch

[Charles Reis]

(In reply to comment #6)
> Created an attachment (id=82860) [details]
> Patch

Here's a draft of what I mentioned in comment 5.

The main drawback is the need to handle Chromium's chrome-back-forward://go URLs for history.back() and history.forward().  We want to avoid stopping all loaders for those, since they get translated to a normal URL and then pass through Page::goToItem again.

This patch handles that, although it hard-codes the protocol rather than getting it from BackForwardListChromium.h.  (That would take a lot of ugly plumbing to get to here, probably via PlatformBridge).  I can change that if necessary.

I also removed the assert in HistoryController, because it would need similar exceptions for same document navigations and the chrome-back-forward scheme.  Maybe we can abstract all that into a function and keep the assert, though I'm not sure it's necessary now that we have a layout test.

[Mihai Parparita]

Comment on attachment 82860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82860&action=review

> Source/WebCore/page/Page.cpp:341
> +#if PLATFORM(CHROMIUM)

This feels pretty icky. Since we're not under the gun to get this into M10, I'd like to explore a cleaner solution.  The current check for the back-forward scheme is done by Chromium's implementation of FrameLoaderClient::shouldGoToHistoryItem. Adding a FrameLoaderClient::shouldStopLoadingForHistoryItem seems reasonable.

Additionally, having all this history logic in Page seems strange. Putting all these checks (lines 335 to 343) in a static HistoryController::shouldStopLoadingForHistoryItemTransition(currentItem, item) method may be cleaner.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:895
> +    // Use Page::goToItem to ensure stopAllLoaders is called first (except for

Can we hide HistoryController::goToItem so that all calls go through Page (make it private and make Page a friend?)

> LayoutTests/http/tests/navigation/resources/forward-to-fragment-fires-onload-2.html:31
> +    setTimeout(function() {

Shouldn't be necessary to do this in a timeout, history.forward() is already async.

[Mihai Parparita]

(In reply to comment #5)
> I'd argue that we should change Page::goToItem to avoid stopping all loaders for a fragment navigation, which would fix this in both Safari and this Chromium patch.  (Note that Page::goToItem already makes a similar exception for the database thread.)

Do you mind testing this behavior in other browsers too? I think this change is reasonable, but I'm curious if Firefox and IE made the same choice.

[Charles Reis]

Great-- I'll clean it up tomorrow and upload a new patch.

(In reply to comment #9)
> Do you mind testing this behavior in other browsers too? I think this change is reasonable, but I'm curious if Firefox and IE made the same choice.

Just tested, and both Firefox and IE keep loading after going forward to a fragment.  Looks like we're doing the right thing.

[Charles Reis]

Comment on attachment 82860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82860&action=review

>> Source/WebCore/page/Page.cpp:341

> 
> This feels pretty icky. Since we're not under the gun to get this into M10, I'd like to explore a cleaner solution.  The current check for the back-forward scheme is done by Chromium's implementation of FrameLoaderClient::shouldGoToHistoryItem. Adding a FrameLoaderClient::shouldStopLoadingForHistoryItem seems reasonable.
> 
> Additionally, having all this history logic in Page seems strange. Putting all these checks (lines 335 to 343) in a static HistoryController::shouldStopLoadingForHistoryItemTransition(currentItem, item) method may be cleaner.

Good ideas.  I've switched these over and updated all the clients.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:895
>> +    // Use Page::goToItem to ensure stopAllLoaders is called first (except for
> 
> Can we hide HistoryController::goToItem so that all calls go through Page (make it private and make Page a friend?)

Done.

>> LayoutTests/http/tests/navigation/resources/forward-to-fragment-fires-onload-2.html:31
>> +    setTimeout(function() {
> 
> Shouldn't be necessary to do this in a timeout, history.forward() is already async.

Actually, the test doesn't work unless it's in a timeout.  It appears to be important to have the history.forward() happen after the page finishes parsing.

[Charles Reis]

Created attachment 82993 [details]
Patch

[Mihai Parparita]

Comment on attachment 82993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82993&action=review

> Source/WebCore/page/Page.cpp:-347
> -            databasePolicy = DatabasePolicyContinue;

I think with this change we no longer need DatabasePolicyContinue (or the whole enum), since we always stop databases when stopping loads. Perhaps add a FIXME to FrameLoader::stopAllLoaders to remove that option?

> LayoutTests/http/tests/navigation/resources/forward-to-fragment-fires-onload-2.html:31
> +    setTimeout(function() {

If it's important that this happen after after the page finishes parsing (any ideas why?), doing it in a DOMContentLoaded event listener seems more reliable than a setTimeout 0.

[Charles Reis]

Created attachment 83017 [details]
Patch

[Charles Reis]

(In reply to comment #13)
> (From update of attachment 82993 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82993&action=review
> 
> > Source/WebCore/page/Page.cpp:-347
> > -            databasePolicy = DatabasePolicyContinue;
> 
> I think with this change we no longer need DatabasePolicyContinue (or the whole enum), since we always stop databases when stopping loads. Perhaps add a FIXME to FrameLoader::stopAllLoaders to remove that option?

Looks like you're right-- added.

> 
> > LayoutTests/http/tests/navigation/resources/forward-to-fragment-fires-onload-2.html:31
> > +    setTimeout(function() {
> 
> If it's important that this happen after after the page finishes parsing (any ideas why?), doing it in a DOMContentLoaded event listener seems more reliable than a setTimeout 0.

Yep, that works.  Not sure why it doesn't work before DOMContentLoaded, since it doesn't trigger a stopAllLoaders and the stack trace from History::goToItem looks the same either way.

[WebKit Commit Bot]

Comment on attachment 83017 [details]
Patch

Clearing flags on attachment: 83017

Committed r79107: <http://trac.webkit.org/changeset/79107>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.