Bug 50254 – Canceled frame loads can corrupt back forward list

• Component: Frames
• Status: RESOLVED FIXED

[Charles Reis]

Created attachment 75173 [details]
Forward and cancel frame test

If a page load is canceled, FrameLoader::checkLoadCompleteForThisFrame resets the back forward list to the previously committed history item, using:
  page->backForward()->setCurrentItem(item.get());

However, this logic only runs if the canceled page load is a main frame.  If a frame load is canceled, the back forward list is left pointing to the canceled item.  This leads to incorrect behavior if the user goes back or forward.

The attached test case can demonstrate this if copied to LayoutTests/http/tests/navigation/.  Going back after canceling the frame load copies the 3rd entry into the 2nd slot.

Not sure what the fix is yet, since calling setCurrentItem on subframe loads doesn't help.

[Charles Reis]

Created attachment 76713 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 76713 [details]
Patch

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

> WebCore/loader/FrameLoader.cpp:2390
> +                        page->mainFrame()->loader()->history()->cancelCurrentItem();

The name cancelCurrentItem is not so great.  You aren't really cancelling it.
You are cancelling / reverting the switch to it.

Also, it seems like you might need to cancel more than just the current item
of the top-level frame.  What if m_frame is actually a grandchild of the main
frame?  What about other sibling frames that aren't navigating?  R- because
of this issue.

I wonder if we shouldn't be making use of HistoryController::m_provisionalItem
more.  It seems like we shouldn't commit to changing m_currentItem until we
know that it is going to stick.  Although that might be a much larger change :-/

[Charles Reis]

I'm trying a new approach with this bug.  I agree that we shouldn't be updating m_currentItem until the navigation is ready to commit, so it seems like recursiveGoToItem needs to change.  I'm taking your suggestion and setting m_provisionalItem in each of the frames of the tree until the commit, instead.

There's one wrinkle here that makes it more complicated.  While we are walking the frame tree with recursiveGoToItem and setting provisional items, it is possible for the actual navigation to immediately commit (on URLs like about:blank) *before* we visit the rest of the frames.  This means that a series of commit related events like updateForCommit, updateForFrameLoadCompleted, and the SetContentState calls in TestShell can all happen before we've set the provisional items on the remaining frames in the tree.

To clean this up, I'll split recursiveGoToItem into two phases: one that sets provisional items on all the frames that aren't going anywhere, and a second that navigates the desired frame(s).  As a result, we'll be ready if the navigation commits immediately.

I'll also have to fix updateForCommit to walk the frame tree and commit each of the provisional items afterward.  This will also restore their scroll position, instead of doing it in recursiveGoToItem.

I have a draft of a patch that seems to be working, and I'll upload it once it's cleaned up and passing all the tests.

[Charles Reis]

Created attachment 78057 [details]
Patch

[Mihai Parparita]

Comment on attachment 78057 [details]
Patch

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

> WebCore/loader/HistoryController.cpp:243
> +    recursiveGoToProvisionalItem(targetItem, currentItem, type);

recursiveSetProvisionalItem seems like a more appropriate name, since it doesn't actually do any navigating/"going."

> WebCore/loader/HistoryController.cpp:423
> +    if (m_provisionalItem) {

Seems better to have a !m_provisionalItem early return, so that the rest function body isn't indented by an extra level.

> WebCore/loader/HistoryController.cpp:601
> +// a match (by URL), we set the provisional item and recurse.  Otherwise we will reload

This comment should probably be updated or removed, we no longer match by URL (it's by item sequence number and frame name now).

> WebCore/loader/HistoryController.cpp:643
> +    if (item != fromItem 

The logic to see if the items/frames match seems complex enough that it shouldn't be repeated in recursiveGoToProvisionalItem and recursiveGoToItem, and instead moved to a itemTransitionNeedsLoad(item1, item2) helper method (possibly with a better name than that).

> LayoutTests/http/tests/navigation/forward-and-cancel.html:16
> +    layoutTestController.queueLoad("javascript:frames[0].clickLink();");

Do you actually need to use the javascript: URI scheme, or can this be queueNonLoadingScript("frames[0].clickLink()")?

> LayoutTests/http/tests/navigation/forward-and-cancel.html:27
> +    layoutTestController.queueLoadingScript("layoutTestController.waitUntilDone();");

Do you need to use queueLoadingScript, or can you just call layoutTestController.waitUntilDone() directly at the top, where you call dumpAsText and other setup code?

[Charles Reis]

Comment on attachment 78057 [details]
Patch

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

Thanks for the feedback.  New patch on the way!

>> WebCore/loader/HistoryController.cpp:243
>> +    recursiveGoToProvisionalItem(targetItem, currentItem, type);
> 
> recursiveSetProvisionalItem seems like a more appropriate name, since it doesn't actually do any navigating/"going."

Done.

>> WebCore/loader/HistoryController.cpp:423
>> +    if (m_provisionalItem) {
> 
> Seems better to have a !m_provisionalItem early return, so that the rest function body isn't indented by an extra level.

Done.

>> WebCore/loader/HistoryController.cpp:601
>> +// a match (by URL), we set the provisional item and recurse.  Otherwise we will reload
> 
> This comment should probably be updated or removed, we no longer match by URL (it's by item sequence number and frame name now).

Done.

>> WebCore/loader/HistoryController.cpp:643
>> +    if (item != fromItem 
> 
> The logic to see if the items/frames match seems complex enough that it shouldn't be repeated in recursiveGoToProvisionalItem and recursiveGoToItem, and instead moved to a itemTransitionNeedsLoad(item1, item2) helper method (possibly with a better name than that).

Good idea.  Done.

>> LayoutTests/http/tests/navigation/forward-and-cancel.html:16
>> +    layoutTestController.queueLoad("javascript:frames[0].clickLink();");
> 
> Do you actually need to use the javascript: URI scheme, or can this be queueNonLoadingScript("frames[0].clickLink()")?

It needs to be queueLoadingScript, but yes, that's better.

>> LayoutTests/http/tests/navigation/forward-and-cancel.html:27
>> +    layoutTestController.queueLoadingScript("layoutTestController.waitUntilDone();");
> 
> Do you need to use queueLoadingScript, or can you just call layoutTestController.waitUntilDone() directly at the top, where you call dumpAsText and other setup code?

I can't seem to get it to work if I call waitUntilDone() directly-- it always hangs on the first page without doing any of the queued navigations.

I did make the notifyDone() call only happen on the second visit to the page, but that didn't help.

[Charles Reis]

Created attachment 78175 [details]
Patch

[Mihai Parparita]

Comment on attachment 78175 [details]
Patch

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

> WebCore/loader/HistoryController.cpp:657
> +    // Note: If item1 and item2 are the same, then we need to create a new

This comment (which I realize predates your patch, you just moved it around) always bothered me, since it's just stating what the code does, but doesn't explain why that behavior is desired/necessary.  Based on http://webkit.org/b/35532 (which is the oldest reference to it that I can find), it seems like some WebKit clients use navigation to the current history item as a reload mechanism. Perhaps you can mention that (and/or the bug number) so that the comment provides some justification for the behavior?

[Charles Reis]

Comment on attachment 78175 [details]
Patch

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

New patch ready for review.

>> WebCore/loader/HistoryController.cpp:657
>> +    // Note: If item1 and item2 are the same, then we need to create a new
> 
> This comment (which I realize predates your patch, you just moved it around) always bothered me, since it's just stating what the code does, but doesn't explain why that behavior is desired/necessary.  Based on http://webkit.org/b/35532 (which is the oldest reference to it that I can find), it seems like some WebKit clients use navigation to the current history item as a reload mechanism. Perhaps you can mention that (and/or the bug number) so that the comment provides some justification for the behavior?

Thanks for figuring out why it's the case!  I've updated the comment as you suggest.

[Charles Reis]

Created attachment 78246 [details]
Patch

[Mihai Parparita]

Comment on attachment 78246 [details]
Patch

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

You should also rebaseline your patch against ToT, otherwise the commit queue may have trouble landing your change (at least the review tool can't apply your changes to FrameLoader.cpp to tip of tree).

> WebCore/loader/HistoryController.cpp:657
> +    // Note: Some clients treat a navigatation to the current history item as

Typo (navigatation)

[Charles Reis]

Created attachment 78252 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 78252 [details]
Patch

Clearing flags on attachment: 78252

Committed r75336: <http://trac.webkit.org/changeset/75336>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.