Bug 48809 – Update correct content state during back/forward navigations

• Component: WebKit API
• Status: RESOLVED FIXED

[Charles Reis]

As part of http://code.google.com/p/chromium/issues/detail?id=58082, we discovered that Chromium's RenderView is sending UpdateState messages to the browser process using the page ID of the last committed history item but the state from the previous history item.  These are not necessarily the same history item, if multiple navigations have been attempted but have not committed.  For example, visiting multiple pages and then holding down the "back" keyboard shortcut can lead to corrupted UpdateState messages that have the original page's page ID but an intermediate page's state.

The UpdateState message should instead be sent with the state of the last committed history item, but the Chromium WebKit API does not currently expose this.  I propose adding WebFrame::lastCommittedHistoryItem() method, which will obtain the desired history item from BackForwardListClientImpl.  WebFrameImpl will have to notify BackForwardListClientImpl when history items commit, so that BackForwardListClientImpl can keep track of the correct item.

I have a draft changelist that shows that this fixes one of the key issues in Chromium bug 58082.

[Charles Reis]

Created attachment 72605 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 72605 [details]
Patch

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

> WebKit/chromium/src/WebFrameImpl.cpp:1006
> +    return WebHistoryItem(viewImpl()->lastCommittedHistoryItem());

I think you can implement this using m_frame->loader()->history()->currentItem()
as that provides you with the last-committed history item for this frame.

This would allow you to eliminate a lot of code in this patch.

Come to think of it, I suspect that WebFrameImpl::currentHistoryItem is
incorrectly implemented.  It is incorrectly accessing the currentItem of
the BackForwardList, which corresponds to the HistoryItem of the top-most
frame.  If a user of the WebKit API wants the HistoryItem of the top-most
frame, then they should just use WebFrame::top()->currentHistoryItem().

In other words, currentHistoryItem is supposed to mean the same thing as
lastCommittedHistoryItem.

[Darin Fisher (:fishd, Google)]

Same goes for previousHistoryItem.  It should be returning the HistoryItem associated with the Frame, not the top-most Frame.

[Charles Reis]

Comment on attachment 72605 [details]
Patch

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

>> WebKit/chromium/src/WebFrameImpl.cpp:1006
>> +    return WebHistoryItem(viewImpl()->lastCommittedHistoryItem());
> 
> I think you can implement this using m_frame->loader()->history()->currentItem()
> as that provides you with the last-committed history item for this frame.
> 
> This would allow you to eliminate a lot of code in this patch.
> 
> Come to think of it, I suspect that WebFrameImpl::currentHistoryItem is
> incorrectly implemented.  It is incorrectly accessing the currentItem of
> the BackForwardList, which corresponds to the HistoryItem of the top-most
> frame.  If a user of the WebKit API wants the HistoryItem of the top-most
> frame, then they should just use WebFrame::top()->currentHistoryItem().
> 
> In other words, currentHistoryItem is supposed to mean the same thing as
> lastCommittedHistoryItem.

My test in http://codereview.chromium.org/4444001/show is showing that won't work.  m_frame->loader()->history()->currentItem() is returning the item for the navigation that's currently in progress, not the last committed item.  As a result, all the UpdateState messages are sending state from the page that's about to commit, rather than the one that's about to be replaced.

Is there another way to get at the history item for the last committed navigation?

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
> My test in http://codereview.chromium.org/4444001/show is showing that won't work.  m_frame->loader()->history()->currentItem() is returning the item for the navigation that's currently in progress, not the last committed item.  As a result, all the UpdateState messages are sending state from the page that's about to commit, rather than the one that's about to be replaced.
> 
> Is there another way to get at the history item for the last committed navigation?

In that case, wouldn't the previousHistoryItem provide you with what you need?  The previousHistoryItem is the HistoryItem that was previously current.  So, if the currentHistoryItem is the new item, then the previousHistoryItem should be the one you need, no?

[Charles Reis]

(In reply to comment #5)
> (In reply to comment #4)
> > My test in http://codereview.chromium.org/4444001/show is showing that won't work.  m_frame->loader()->history()->currentItem() is returning the item for the navigation that's currently in progress, not the last committed item.  As a result, all the UpdateState messages are sending state from the page that's about to commit, rather than the one that's about to be replaced.
> > 
> > Is there another way to get at the history item for the last committed navigation?
> 
> In that case, wouldn't the previousHistoryItem provide you with what you need?  The previousHistoryItem is the HistoryItem that was previously current.  So, if the currentHistoryItem is the new item, then the previousHistoryItem should be the one you need, no?

Here's the scenario:
 - Visit pages A, B, C
 - Quickly press back twice (to B and then to A), before B has a chance to commit.

At this point in time, A is the current item, B is the previous item, and C is the last committed item.  I need a way to get to C.  Note that there may be an arbitrary number of pages like B (between previous and last committed), if you hold down the back button and race past several entries.

The original bug is that we were using the previous item, which was giving us B, but the last committed page ID (corresponding to C).

[Darin Fisher (:fishd, Google)]

> Here's the scenario:
>  - Visit pages A, B, C
>  - Quickly press back twice (to B and then to A), before B has a chance to commit.
> 
> At this point in time, A is the current item, B is the previous item, and C is the last committed item.  I need a way to get to C.  Note that there may be an arbitrary number of pages like B (between previous and last committed), if you hold down the back button and race past several entries.
> 
> The original bug is that we were using the previous item, which was giving us B, but the last committed page ID (corresponding to C).

Oh, I see.  Perhaps we should not be setting the previous HistoryItem until we successfully commit.  I think it may confuse other things if previousHistoryItem is not reliably the last committed HistoryItem.

The HistoryController appears to have the concept of a provisional HistoryItem that is only set once we commit.  See HistoryController::updateForCommit.

Perhaps you should try fixing WebFrameImpl::previousHistoryItem to use HistoryController::previousItem() instead of BackForwardList::previousItem()?

Likewise, perhaps we should defer the updating of BackForwardList::{current,previous}Item until we commit.  See HistoryController::goToItem, where it assigns those directly.

[Charles Reis]

Created attachment 73018 [details]
Patch

[Charles Reis]

(In reply to comment #7)
> Oh, I see.  Perhaps we should not be setting the previous HistoryItem until we successfully commit.  I think it may confuse other things if previousHistoryItem is not reliably the last committed HistoryItem.
> 
> The HistoryController appears to have the concept of a provisional HistoryItem that is only set once we commit.  See HistoryController::updateForCommit.
> 
> Perhaps you should try fixing WebFrameImpl::previousHistoryItem to use HistoryController::previousItem() instead of BackForwardList::previousItem()?

Ah.  This does fix the bug, and it looks like it's the right move for the other callers as well (e.g., the equivalent callers in test_shell and DRT).

Note that this requires adding a previousItem() getter to HistoryController, since it only has a private field for it and doesn't currently expose it.

> Likewise, perhaps we should defer the updating of BackForwardList::{current,previous}Item until we commit.  See HistoryController::goToItem, where it assigns those directly.

That seems like a much bigger change, especially since the comment in HistoryController::goToItem says it eagerly updates the BF cursor on purpose.  Do we need to tackle that one here?

I've uploaded a new patch.  The webkit-patch script generated the ChangeLog, but I'm wondering if I'm doing that wrong-- I thought this would have required modifications to two separate ChangeLog files.

[Darin Fisher (:fishd, Google)]

Comment on attachment 73018 [details]
Patch

This change LGTM, but yeah... you need an entry in WebCore/ChangeLog.

[Darin Fisher (:fishd, Google)]

(In reply to comment #9)
> Ah.  This does fix the bug, and it looks like it's the right move for the other callers as well (e.g., the equivalent callers in test_shell and DRT).

Excellent!


> > Likewise, perhaps we should defer the updating of BackForwardList::{current,previous}Item until we commit.  See HistoryController::goToItem, where it assigns those directly.
> 
> That seems like a much bigger change, especially since the comment in HistoryController::goToItem says it eagerly updates the BF cursor on purpose.  Do we need to tackle that one here?

Agreed.  It is better tackled elsewhere.

[Charles Reis]

Created attachment 73077 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 73077 [details]
Patch

Clearing flags on attachment: 73077

Committed r71437: <http://trac.webkit.org/changeset/71437>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Charles Reis]

After this landed and got picked up by Chrome, Chrome's SessionHistoryTest.FragmentBackForward started failing.  That test:
 - Visits fragment.html#a, fragment.html#b, fragment.html#c
 - Goes back several times, expecting to see them in reverse order

Instead, it sees #b (correct), then #c (incorrect), and keeps alternating between them.  I can verify this behavior manually in Chrome, but not in test_shell.  I'll have to take a closer look to see how this change caused that.

[Darin Fisher (:fishd, Google)]

(In reply to comment #15)
> After this landed and got picked up by Chrome, Chrome's SessionHistoryTest.FragmentBackForward started failing.  That test:
>  - Visits fragment.html#a, fragment.html#b, fragment.html#c
>  - Goes back several times, expecting to see them in reverse order
> 
> Instead, it sees #b (correct), then #c (incorrect), and keeps alternating between them.  I can verify this behavior manually in Chrome, but not in test_shell.  I'll have to take a closer look to see how this change caused that.

Ugh.  It is not uncommon for code in Chromium (e.g., navigation_controller.cc) to be working against you in cases like this because it was trying to compensate for some bug in WebKit (such as the one you fixed here).

[Charles Reis]

(In reply to comment #16)
> Ugh.  It is not uncommon for code in Chromium (e.g., navigation_controller.cc) to be working against you in cases like this because it was trying to compensate for some bug in WebKit (such as the one you fixed here).

Interesting-- this didn't turn out the way I expected.  The problem is that HistoryController's m_previousItem is null after the load commits, rather than being the last committed history item.  It gets set to null in HistoryController::updateForFrameLoadCompleted.  (As a result, WebFrameImpl::previousHistoryItem returns null when Chrome tries to send UpdateState messages, which leads to the bug I mentioned.  Details: http://code.google.com/p/chromium/issues/detail?id=62156)

I'm wary about changing the behavior of HistoryController itself, but that does seem like odd behavior.  Darin, do you understand what's going on there?  Is it better to change to having a new WebFrameImpl::lastCommittedHistoryItem() method (as in the earlier patch here), or to try to avoid setting m_previousItem to null?

Either way, this has led to buggy navigation behavior in Chrome, so we may want to revert this patch.

[Charles Reis]

(In reply to comment #17)
> Interesting-- this didn't turn out the way I expected.  The problem is that HistoryController's m_previousItem is null after the load commits, rather than being the last committed history item.  It gets set to null in HistoryController::updateForFrameLoadCompleted.  (As a result, WebFrameImpl::previousHistoryItem returns null when Chrome tries to send UpdateState messages, which leads to the bug I mentioned.  Details: http://code.google.com/p/chromium/issues/detail?id=62156)

A small clarification: m_previousItem always gets set to null, but it's at different times depending on whether it's a fragment navigation or not (which is why the Chrome bug only happens when navigating to a URL fragment).  For normal navigations, updateForFrameLoadCompleted gets called after we send the UpdateState message.  For fragment navigations, it gets called twice before we send the UpdateState message (preventing the message from getting sent).

[Darin Fisher (:fishd, Google)]

(In reply to comment #18)
> (In reply to comment #17)
> > Interesting-- this didn't turn out the way I expected.  The problem is that HistoryController's m_previousItem is null after the load commits, rather than being the last committed history item.  It gets set to null in HistoryController::updateForFrameLoadCompleted.  (As a result, WebFrameImpl::previousHistoryItem returns null when Chrome tries to send UpdateState messages, which leads to the bug I mentioned.  Details: http://code.google.com/p/chromium/issues/detail?id=62156)
> 
> A small clarification: m_previousItem always gets set to null, but it's at different times depending on whether it's a fragment navigation or not (which is why the Chrome bug only happens when navigating to a URL fragment).  For normal navigations, updateForFrameLoadCompleted gets called after we send the UpdateState message.  For fragment navigations, it gets called twice before we send the UpdateState message (preventing the message from getting sent).

I can't think of any good reason for nulling out m_previousItem.  Does anything actually break (layout test wise) if you make HistoryController::updateForFrameLoadCompleted() do nothing?  The comment in that function seems out-of-date.  It seems like the kind of thing that m_provisionalItem resolves.

[Charles Reis]

(In reply to comment #19)

> I can't think of any good reason for nulling out m_previousItem.  Does anything actually break (layout test wise) if you make HistoryController::updateForFrameLoadCompleted() do nothing?  The comment in that function seems out-of-date.  It seems like the kind of thing that m_provisionalItem resolves.

Yep, that breaks forward fragment navigations (in both Chrome and test_shell).  Plus it introduces a crash in certain cases because of an ASSERT(!m_previousItem) in HistoryController::recursiveGoToItem on line 567.  Disabling the assert still leaves the problem with forward navigations.

It's too bad these files aren't better commented, because it's not clear what values m_previousItem is expected to have at various points in time.  There seem to be a lot of assumptions that it is null between navigations rather than pointing to the last committed item, though.

[Darin Fisher (:fishd, Google)]

So, if we are going to have a m_lastCommittedItem anywhere, I think it should probably be maintained on HistoryController.  I suspect m_previousItem should be that, but it sounds like we'd need to untangle some things first.

[Charles Reis]

I dug a little deeper and found the root cause of why previousItem wasn't working for fragments.  As I mentioned in comment 18, HistoryController::updateForFrameLoadCompleted() is getting called before the content state is updated, which means m_previousItem is null when we need it.

It turns out this is because FrameLoader::loadInSameDocument() is simply calling dispatchDidNavigateWithinPage() after calling checkComplete() and checkLoadComplete().  By moving the dispatchDidNavigateWithinPage() call earlier, we still have m_previousItem when the content state is updated.  This fixes the issue with fragment navigations entirely.

I've double checked that the behavior of Chromium's FrameLoaderClientImpl::dispatchDidNavigateWithinPage() is the same whether we call it before or after checkComplete().  I'm running the layout tests now to be sure.  Patch on the way-- it's nice and simple, since we don't need to keep track of lastCommittedItem separately anymore.

[Charles Reis]

Created attachment 73647 [details]
Patch

[Charles Reis]

Comment on attachment 73647 [details]
Patch

This has two failing layout tests:
fast/history/same-document-iframes-changing-pushstate.html
fast/loader/stateobjects/document-destroyed-navigate-back.html

I'll look into it and update accordingly.

[Charles Reis]

Found the problem with the pushstate tests.  They revealed that we were not sending a content state update for same-page back/forward navigations before (because m_previousItem was 0).  After my change, we were sending updates, but with the wrong value for m_previousItem.

The problem was that HistoryController::m_previousItem was not being updated for back/forward navigations-- it's perfectly safe to send a content state update, as long as it's the correct one.

The new patch I'm uploading ensures m_previousItem is correct in this case by updating it in HistoryController::setCurrentItem.  The layout tests I mention in comment 24 cover the change, but I'm running the rest of the tests to double check.

[Charles Reis]

Created attachment 73692 [details]
Patch

[Charles Reis]

There's a lot of noise on the Chromium try servers (i.e., layout tests that fail with or without my change), but the results look good to me-- I haven't seen any failures relevant to the patch.  I'd say it's ready for review.

[Mihai Parparita]

Comment on attachment 73692 [details]
Patch

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

> WebCore/ChangeLog:8
> +        https://bugs.webkit.org/show_bug.cgi?id=48809

BTW, the usual ChangeLog format (if you use webkit-patch upload) is:

[bug title]
[bug link]

[longer description]

(this assumes you're going to retitle the bug, since you're not actually adding lastCommittedHistoryItem() anymore)

[Charles Reis]

(In reply to comment #28)
> BTW, the usual ChangeLog format (if you use webkit-patch upload) is:
> 
> [bug title]
> [bug link]
> 
> [longer description]
> 
> (this assumes you're going to retitle the bug, since you're not actually adding lastCommittedHistoryItem() anymore)

Sure-- update on the way.

[Charles Reis]

Created attachment 73802 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 73802 [details]
Patch

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

> WebCore/loader/FrameLoader.cpp:1164
> +    m_client->dispatchDidNavigateWithinPage();

Moving this here seems wrong to me.  This notification is meant to be
generated after the within-page navigation completes.  I'm concerned
that checkCompleted and checkLoadCompleted change our state in ways
that should be observable from dispatchDidNavigateWithinPage.  For
example, it looks like checkCompleted may generate a readystatechange
event on the document.

It still seems to me that the right solution is to not clear m_previousItem.
Given your change to setCurrentItem, does the forward navigation issue go
away by any chance?  Can you try to unravel what is going on there?

I can point to places in HistoryController that assume m_previousItem will
be valid.  See HistoryController::createItemTree.

[Charles Reis]

(In reply to comment #31)
> (From update of attachment 73802 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73802&action=review
> 
> > WebCore/loader/FrameLoader.cpp:1164
> > +    m_client->dispatchDidNavigateWithinPage();
> 
> Moving this here seems wrong to me.  This notification is meant to be
> generated after the within-page navigation completes.  I'm concerned
> that checkCompleted and checkLoadCompleted change our state in ways
> that should be observable from dispatchDidNavigateWithinPage.  For
> example, it looks like checkCompleted may generate a readystatechange
> event on the document.
> 
> It still seems to me that the right solution is to not clear m_previousItem.
> Given your change to setCurrentItem, does the forward navigation issue go
> away by any chance?  Can you try to unravel what is going on there?
> 
> I can point to places in HistoryController that assume m_previousItem will
> be valid.  See HistoryController::createItemTree.

Yes, with my change to setCurrentItem, it still works in most cases if we revert the FrameLoader change and don't clear m_previousItem in updateForFrameLoadCompleted.  There's a few layout tests that crash or fail now (e.g., due to the ASSERT in recursiveGoToItem), but I'll take a closer look at the assumptions to see if I can fix them.

For reference, I'm seeing fast/history/history-back-within-subframe-hash.html crash and fast/history/saves-state-after-fragment-nav.html fail.

[Darin Fisher (:fishd, Google)]

> Yes, with my change to setCurrentItem, it still works in most cases if we revert the FrameLoader change and don't clear m_previousItem in updateForFrameLoadCompleted.  There's a few layout tests that crash or fail now (e.g., due to the ASSERT in recursiveGoToItem), but I'll take a closer look at the assumptions to see if I can fix them.

Cool!


> For reference, I'm seeing fast/history/history-back-within-subframe-hash.html crash and fast/history/saves-state-after-fragment-nav.html fail.

OK.

My thinking here is that if we keep pulling on this thread that we'll find that things can be simpler, which would be good.

[Charles Reis]

(In reply to comment #33)
> My thinking here is that if we keep pulling on this thread that we'll find that things can be simpler, which would be good.

The main difficulty is the way that saveDocumentState saves the state of different items (previous or current) at different points in time.  That function assumes that "m_previousItem == 0" implies that the load has completed, in which case it should use current rather than previous.

I've teased apart the state of whether the load has completed from the value of previousItem, by adding a m_frameLoadCompleted field.  This has the advantage of matching the previous WebCore behavior, while keeping a valid item in m_previousItem so that Chromium's UpdateSessionHistory call works.

It might be possible to simplify things further, but I'd need help understanding more of the assumptions around frame loading.  Since Chromium is currently broken on fragment back/forward navigations, it might be nice to get a simple fix like this in place before taking time to refactor further.

Thoughts?

[Charles Reis]

Created attachment 74079 [details]
Patch

[Early Warning System Bot]

Attachment 74079 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6227003

[Charles Reis]

Created attachment 74082 [details]
Patch

[Build Bot]

Attachment 74079 [details] did not build on win:
Build output: http://queues.webkit.org/results/6221005

[Darin Fisher (:fishd, Google)]

Comment on attachment 74082 [details]
Patch

R=me

I like the explicit member variable.  This seems like an improvement over inferring
"frame load complete" from m_previousItem being null.

[WebKit Commit Bot]

Comment on attachment 74082 [details]
Patch

Clearing flags on attachment: 74082

Committed r72299: <http://trac.webkit.org/changeset/72299>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Levin]

Rolled out in http://trac.webkit.org/changeset/72334

Broke chromium ui test SessionHistoryTest.FrameBackForward.

[Charles Reis]

(In reply to comment #42)
> Rolled out in http://trac.webkit.org/changeset/72334
> 
> Broke chromium ui test SessionHistoryTest.FrameBackForward.

Good news-- that test caught a real issue with the patch, and it uncovered a previously existing bug that we can now fix.  (We weren't saving form state for subframes when you navigate back and forward.)

Turns out there was one other place in HistoryController where we were changing m_currentItem but not m_previousItem (in recursiveGoToItem).  Before my patch, that caused us to skip the content state update in that case, which meant we didn't save form state if the form was in a subframe and you navigated back/forward.

After my earlier patch landed, we were updating the wrong history item, which was caught by Chrome's UI test.  I've now fixed recursiveGoToItem so that it updates m_previousItem, and I've added a layout test for it in the latest patch.

[Charles Reis]

Created attachment 74433 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 74433 [details]
Patch

Rejecting patch 74433 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
...............
fast/frames ...................................................................................................................
fast/frames/flattening ...........
fast/gradients ...........
fast/harness ......
fast/history ...................
fast/history/saves-state-after-frame-nav.html -> failed

Exiting early after 1 failures. 8610 tests run.
439.55s total testing time

8609 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/6246062

[Charles Reis]

(In reply to comment #45)
> (From update of attachment 74433 [details])
> Rejecting patch 74433 from commit-queue.
> 
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
> Last 500 characters of output:
> ...............
> fast/frames ...................................................................................................................
> fast/frames/flattening ...........
> fast/gradients ...........
> fast/harness ......
> fast/history ...................
> fast/history/saves-state-after-frame-nav.html -> failed
> 
> Exiting early after 1 failures. 8610 tests run.
> 439.55s total testing time
> 
> 8609 test cases (99%) succeeded
> 1 test case (<1%) had incorrect layout
> 4 test cases (<1%) had stderr output
> 
> Full output: http://queues.webkit.org/results/6246062

Turns out this just needed another newline at the end of the expected.txt file.  The Chromium try servers didn't catch that.

[Charles Reis]

Created attachment 74487 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74487 [details]:

fast/dom/prototype-inheritance.html
compositing/iframes/overlapped-nested-iframes.html

Please file bugs against the tests.  These tests were authored by [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 74487 [details]
Patch

Clearing flags on attachment: 74487

Committed r72566: <http://trac.webkit.org/changeset/72566>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.