Bug 54517 - Ensure loading has stopped in HistoryController::goToItem
Summary: Ensure loading has stopped in HistoryController::goToItem
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-15 17:52 PST by Charles Reis
Modified: 2011-02-19 00:29 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.76 KB, patch)
2011-02-15 18:02 PST, Charles Reis
no flags Details | Formatted Diff | Diff
Patch (8.98 KB, patch)
2011-02-17 14:07 PST, Charles Reis
no flags Details | Formatted Diff | Diff
Patch (33.81 KB, patch)
2011-02-18 12:15 PST, Charles Reis
no flags Details | Formatted Diff | Diff
Patch (34.53 KB, patch)
2011-02-18 14:56 PST, Charles Reis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charles Reis 2011-02-15 17:52:00 PST
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.
Comment 1 Charles Reis 2011-02-15 18:02:37 PST
Created attachment 82565 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 2011-02-15 21:22:04 PST
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.
Comment 3 Darin Fisher (:fishd, Google) 2011-02-15 21:23:47 PST
Oh, nevermind... I see that Page::goToItem special cases navigations that do not change the document!  Good :-)
Comment 4 Darin Fisher (:fishd, Google) 2011-02-15 21:24:28 PST
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.
Comment 5 Charles Reis 2011-02-16 12:04:05 PST
(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?
Comment 6 Charles Reis 2011-02-17 14:07:25 PST
Created attachment 82860 [details]
Patch
Comment 7 Charles Reis 2011-02-17 14:13:53 PST
(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.
Comment 8 Mihai Parparita 2011-02-17 17:53:06 PST
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.
Comment 9 Mihai Parparita 2011-02-17 17:54:38 PST
(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.
Comment 10 Charles Reis 2011-02-17 18:20:41 PST
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.
Comment 11 Charles Reis 2011-02-18 12:14:33 PST
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.
Comment 12 Charles Reis 2011-02-18 12:15:00 PST
Created attachment 82993 [details]
Patch
Comment 13 Mihai Parparita 2011-02-18 14:32:33 PST
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.
Comment 14 Charles Reis 2011-02-18 14:56:19 PST
Created attachment 83017 [details]
Patch
Comment 15 Charles Reis 2011-02-18 14:57:46 PST
(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.
Comment 16 WebKit Commit Bot 2011-02-19 00:29:43 PST
Comment on attachment 83017 [details]
Patch

Clearing flags on attachment: 83017

Committed r79107: <http://trac.webkit.org/changeset/79107>
Comment 17 WebKit Commit Bot 2011-02-19 00:29:49 PST
All reviewed patches have been landed.  Closing bug.