Also update the session history in the renderer when the active entry is a new pending entry

Also update the session history in the renderer when the active entry is a new pending entry

BUG=178103
TEST:content_unittests:NavigationControllerImplTest.PruneAllButActiveForPendingNotInList
R=creis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188910

[jochen (gone - plz use gerrit), 2013-03-08 19:01:06]

I'm actually not sure whether this is the right solution.

Is it enough to filter the past page ids? Or maybe just the page id of the last
committed entry? Or do we also need the page ids from the forward list?

Maybe the right solution is to swap out the RVH of the last committed entry (my
assumption is that this can only happen when the last committed entry and the
pending entry have different RVHs).

[Charlie Reis, 2013-03-08 23:35:50]

https://codereview.chromium.org/12552005/diff/1/content/browser/web_contents/...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/12552005/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl.cc:1001: // that
reference pruned page ids. Just ignore those navigations.
Ignoring a navigation typically leads to a URL spoof, since it means that the
displayed content and the URL we show in the omnibox differ.  I don't think this
is safe.

Still, this is a useful observation that PruneAllButActive might clear entries
while IPCs are in flight.  Perhaps there's another way to handle it?

Here's one idea, though I haven't thought through the implications.  In essence,
the browser process needs to warn the renderer that it's going to prune some
entries.  Suppose the prune call became:

1) Browser sends an IPC to renderer telling it what it plans to prune, and then
waits for an ack.
2) Renderer hears this, stops any current navigations (causing the stop IPC
messages to arrive before the ack), then updates its local view of page IDs and
history accordingly.  Any future navigation IPCs will be as if the prune has
already happened.
3) Renderer sends ACK, at which point browser does the actual prune.  Even if
new navigations have started in the renderer, they won't reference pruned page
IDs.

Would that work?

[jochen (gone - plz use gerrit), 2013-03-08 23:50:39]

https://codereview.chromium.org/12552005/diff/1/content/browser/web_contents/...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/12552005/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl.cc:1001: // that
reference pruned page ids. Just ignore those navigations.
On 2013/03/08 23:35:51, creis wrote:
> Ignoring a navigation typically leads to a URL spoof, since it means that the
> displayed content and the URL we show in the omnibox differ.  I don't think
this
> is safe.
> 
> Still, this is a useful observation that PruneAllButActive might clear entries
> while IPCs are in flight.  Perhaps there's another way to handle it?
> 
> Here's one idea, though I haven't thought through the implications.  In
essence,
> the browser process needs to warn the renderer that it's going to prune some
> entries.  Suppose the prune call became:
> 
> 1) Browser sends an IPC to renderer telling it what it plans to prune, and
then
> waits for an ack.
> 2) Renderer hears this, stops any current navigations (causing the stop IPC
> messages to arrive before the ack), then updates its local view of page IDs
and
> history accordingly.  Any future navigation IPCs will be as if the prune has
> already happened.
> 3) Renderer sends ACK, at which point browser does the actual prune.  Even if
> new navigations have started in the renderer, they won't reference pruned page
> IDs.
> 
> Would that work?

I think that's not necessary.

If the renderer is the one that does the navigation for the pending entry it
won't (shouldn't at least) send navigation signals for old pages.

So we only have this situation when the last committed entry and the pending
entry refer to two different RVH. In that case, we can just delete the RVH and
block incoming IPC messages from that render view.

[Charlie Reis, 2013-03-09 00:55:11]

https://codereview.chromium.org/12552005/diff/1/content/browser/web_contents/...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/12552005/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl.cc:1001: // that
reference pruned page ids. Just ignore those navigations.
On 2013/03/08 23:50:39, jochen wrote:
> On 2013/03/08 23:35:51, creis wrote:
> > Ignoring a navigation typically leads to a URL spoof, since it means that
the
> > displayed content and the URL we show in the omnibox differ.  I don't think
> this
> > is safe.
> > 
> > Still, this is a useful observation that PruneAllButActive might clear
entries
> > while IPCs are in flight.  Perhaps there's another way to handle it?
> > 
> > Here's one idea, though I haven't thought through the implications.  In
> essence,
> > the browser process needs to warn the renderer that it's going to prune some
> > entries.  Suppose the prune call became:
> > 
> > 1) Browser sends an IPC to renderer telling it what it plans to prune, and
> then
> > waits for an ack.
> > 2) Renderer hears this, stops any current navigations (causing the stop IPC
> > messages to arrive before the ack), then updates its local view of page IDs
> and
> > history accordingly.  Any future navigation IPCs will be as if the prune has
> > already happened.
> > 3) Renderer sends ACK, at which point browser does the actual prune.  Even
if
> > new navigations have started in the renderer, they won't reference pruned
page
> > IDs.
> > 
> > Would that work?
> 
> I think that's not necessary.
> 
> If the renderer is the one that does the navigation for the pending entry it
> won't (shouldn't at least) send navigation signals for old pages.
> 
> So we only have this situation when the last committed entry and the pending
> entry refer to two different RVH. In that case, we can just delete the RVH and
> block incoming IPC messages from that render view.

I don't understand which case you're referring to, then.  A given renderer is
free to go back to earlier entries, even during something like prerendering.  (A
page could do history.back(), for example.)  Why couldn't it have done that just
as the browser called PruneAllButActive?  That's what I thought this was about.

I'm not sure I follow the last committed vs pending entry case, either, but it
sounds plausible.  Sure, PruneAllButActive knows which RVH it will keep, so the
other can be deleted.  No need to do anything special for IPC blocking, since
the messages will just have a routing_id that no longer goes anywhere.

[jochen (gone - plz use gerrit), 2013-03-17 15:29:37]

I made a few tests:

It turns out that in the situation I see the crashes, there is no cross-site
navigation going on. (Also, there's a DCHECK in
WebContentsImpl::SetHistoryLengthAndPrune that no cross site navigation is going
on. I guess we should change to that a CHECK()).

Instead, the current RVH just sends navigation signals. However, I haven't found
a reliable way to make it stop sending navigation signals short of destroying
the document: while it's possible to stop the frameloader, javascript and
plugins will happily continue to navigate.

My conclusion is that the caller of PruneAllButActive has to ensure that the RV
being pruned cannot navigate... in my case, I achieve this by navigating the RV
first to about:blank.

[jochen (gone - plz use gerrit), 2013-03-17 23:04:38]

I've updated the CL to only sync the session history.

ptal

[Charlie Reis, 2013-03-18 22:42:24]

LGTM with one change.

https://codereview.chromium.org/12552005/diff/10001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/12552005/diff/10001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1411:
NavigationEntryImpl* entry = GetEntryCount() ?
I don't think this is necessary, since GetActiveEntry() returns the pending
entry if it exists.

(It will also return the transient entry if there is one, but it looks like that
will be in entries_, so we don't need to special case it above.)

[jochen (gone - plz use gerrit), 2013-03-18 22:58:07]

https://codereview.chromium.org/12552005/diff/10001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/12552005/diff/10001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1411:
NavigationEntryImpl* entry = GetEntryCount() ?
On 2013/03/18 22:42:24, creis wrote:
> I don't think this is necessary, since GetActiveEntry() returns the pending
> entry if it exists.
> 
> (It will also return the transient entry if there is one, but it looks like
that
> will be in entries_, so we don't need to special case it above.)

Done.

[commit-bot: I haz the power, 2013-03-18 23:01:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/12552005/16001

[commit-bot: I haz the power, 2013-03-19 01:44:31]

Change committed as 188910