Handle should_replace_current_entry in prerender.

Handle should_replace_current_entry in prerender.

This fixes calls to location.replace which trigger a prerender. History entries
after the current one are incorrectly discarded (http://crbug.com/317872), but
the current entry is correctly replaced.

BUG=305400
TEST=PrerenderBrowserTest.PrerenderReplaceCurrentEntry
     NavigationControllerTest.CopyStateFromAndPruneReplaceEntry
     NavigationControllerTest.CopyStateFromAndPruneMaxEntriesReplaceEntry

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

[davidben, 2013-10-29 23:00:09]

creis: For content/. Also you probably would know if this is an okay change to
make to CopyStateFromAndPrune.

samarth: For chrome/browser/ui/browser_instant_controller.cc.

[Charlie Reis, 2013-10-30 16:31:30]

FYI, I'm a bit backlogged with some blocker bugs and unfortunately
CopyStateFromAndPrune is one of the most tricky methods in NavigationController
to get right.  I'll need to carve out some time to think about the implications.
 Hopefully I can look by the end of the day.

[davidben, 2013-10-30 18:14:05]

On 2013/10/30 16:31:30, creis wrote:
> FYI, I'm a bit backlogged with some blocker bugs and unfortunately
> CopyStateFromAndPrune is one of the most tricky methods in
NavigationController
> to get right.  I'll need to carve out some time to think about the
implications.
>  Hopefully I can look by the end of the day.

Hrm, it occurs to me location.replace doesn't quite have this behavior. It
replaces the current entry but leaves the entries in front of it intact for a
same-process navigation. Interestingly, cross-process navigations also do this
right now. Load a few pages, press back a bit, and compare:
location.replace("http://example.com") vs
location.replace("https://chrome.google.com/webstore").

[davidben, 2013-10-30 18:31:34]

Perhaps we should just not bother using prerenders if they try to
location.replace. We can abort with an appropriate FinalStatus and monitor UMA
to see if it's worth supporting. Especially when http://crbug.com/305660 is
fixed (although it may be worth adding a histogram to see if we should just
abort prerenders altogether when they create multiple navigation entries), I
think the correct behavior would be to prune entries in front of the active
entry iff we are splicing in more than 1 entry. And that's quite a bit of
complexity to add to this function.

[mmenke, 2013-10-30 18:41:13]

On 2013/10/30 18:31:34, David Benjamin wrote:
> Perhaps we should just not bother using prerenders if they try to
> location.replace. We can abort with an appropriate FinalStatus and monitor UMA
> to see if it's worth supporting. Especially when http://crbug.com/305660 is
> fixed (although it may be worth adding a histogram to see if we should just
> abort prerenders altogether when they create multiple navigation entries), I
> think the correct behavior would be to prune entries in front of the active
> entry iff we are splicing in more than 1 entry. And that's quite a bit of
> complexity to add to this function.

That sounds reasonable to me.

[davidben, 2013-10-30 18:42:18]

On 2013/10/30 18:31:34, David Benjamin wrote:
> Perhaps we should just not bother using prerenders if they try to
> location.replace. We can abort with an appropriate FinalStatus and monitor UMA
> to see if it's worth supporting. Especially when http://crbug.com/305660 is
> fixed (although it may be worth adding a histogram to see if we should just
> abort prerenders altogether when they create multiple navigation entries), I
> think the correct behavior would be to prune entries in front of the active
> entry iff we are splicing in more than 1 entry. And that's quite a bit of
> complexity to add to this function.

Hrm, actually things like search redirects are probably a case where we care
about this. It looks like the redirect happens client-side. It currently works
because the redirect loads JS into an iframe (or something?) and has the iframe
navigate its parent, but there's also a codepath for location.replace if you're
not in a frame.

[mmenke, 2013-10-30 18:46:28]

On 2013/10/30 18:42:18, David Benjamin wrote:
> On 2013/10/30 18:31:34, David Benjamin wrote:
> > Perhaps we should just not bother using prerenders if they try to
> > location.replace. We can abort with an appropriate FinalStatus and monitor
UMA
> > to see if it's worth supporting. Especially when http://crbug.com/305660 is
> > fixed (although it may be worth adding a histogram to see if we should just
> > abort prerenders altogether when they create multiple navigation entries), I
> > think the correct behavior would be to prune entries in front of the active
> > entry iff we are splicing in more than 1 entry. And that's quite a bit of
> > complexity to add to this function.
> 
> Hrm, actually things like search redirects are probably a case where we care
> about this. It looks like the redirect happens client-side. It currently works
> because the redirect loads JS into an iframe (or something?) and has the
iframe
> navigate its parent, but there's also a codepath for location.replace if
you're
> not in a frame.

Search redirects use HTTP redirects, no?

[Charlie Reis, 2013-10-31 18:01:14]

Sorry for the delay.  I'm concerned about the case that source's last committed
entry isn't the last one (as you mention).

Are you still pursuing this, or did you decide on a different approach?  (I'd
like to avoid making CopyStateFromAndPrune more complex if possible, but I admit
it doesn't support the case you're describing.)

[davidben, 2013-12-10 20:49:55]

Apparently some people want this working, so I've rebased the patch. It still
doesn't preserve the entries forward of the current one but should work as well
as normal cross-process navigations do, until we fix #317872. (And then fixing
it for the merge function will be interesting... I think
SetHistoryLengthAndPrune will need some considerable tweaks.)

This does make the merge function more complicated, but I don't think it
introduces more problematic cases. The source's renderer-side page ID map is not
touched anyway. The destination's are, but we're just prepending one fewer -1
value. Otherwise it behaves like InsertOrReplaceEntry: pretend
last_committed_entry_index_ is one fewer than it actually is.

[Charlie Reis, 2013-12-11 02:57:38]

I'm still a bit scarred by my last encounters with CopyStateFromAndPrune, but it
looks like what you're doing should be safe.  (Famous last words.)  :)  I agree
that it doesn't yet preserve the forward history, but that can be tackled
separately.

The content/ parts should be good with the minor changes below.

https://codereview.chromium.org/49003011/diff/200001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/49003011/diff/200001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:3362: // First entry is
about:blank, second is prerender_page.html.
Might as well add checks for these specific URLs in the history, to help catch
regressions.

https://codereview.chromium.org/49003011/diff/200001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/49003011/diff/200001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3466:
EXPECT_NE(instance3, instance1);
Yikes, these are two different SiteInstances for the same site in the same tab. 
I suppose we decided that we allow that for prerendering by putting them in
different BrowsingInstances...

Please update the comment to say "A new SiteInstance in a different
BrowsingInstance should be used for the new entry" and add a check that
IsRelatedSiteInstance returns false.

https://codereview.chromium.org/49003011/diff/200001/content/public/browser/n...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/49003011/diff/200001/content/public/browser/n...
content/public/browser/navigation_controller.h:411: // If |replace_entry| is
true, the current entry in |source| is omitted. So
nit: s/omitted/replaced/

[davidben, 2013-12-11 21:20:59]

Hehe. Yeah, I don't know if I can go as far as asserting the function is
completely safe, but I'm pretty sure I'm not making it worse than it is already.
:-)

https://codereview.chromium.org/49003011/diff/200001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/49003011/diff/200001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:3362: // First entry is
about:blank, second is prerender_page.html.
On 2013/12/11 02:57:39, creis wrote:
> Might as well add checks for these specific URLs in the history, to help catch
> regressions.

Done.

https://codereview.chromium.org/49003011/diff/200001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/49003011/diff/200001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3466:
EXPECT_NE(instance3, instance1);
On 2013/12/11 02:57:39, creis wrote:
> Yikes, these are two different SiteInstances for the same site in the same
tab. 
> I suppose we decided that we allow that for prerendering by putting them in
> different BrowsingInstances...
> 
> Please update the comment to say "A new SiteInstance in a different
> BrowsingInstance should be used for the new entry" and add a check that
> IsRelatedSiteInstance returns false.

I just got that from the existing CopyStateFromAndPrune test. Updated both
versions.

https://codereview.chromium.org/49003011/diff/200001/content/public/browser/n...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/49003011/diff/200001/content/public/browser/n...
content/public/browser/navigation_controller.h:411: // If |replace_entry| is
true, the current entry in |source| is omitted. So
On 2013/12/11 02:57:39, creis wrote:
> nit: s/omitted/replaced/

Done.

[Charlie Reis, 2013-12-12 02:46:37]

Thanks, and sorry for the delay.  LGTM.

https://codereview.chromium.org/49003011/diff/220001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/49003011/diff/220001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:836:
content::OpenURLParams(GURL(kBlankURL), Referrer(),
This looks like an improvement to me, though it's not identical behavior.  (I
don't know what chrome://blank is supposed to do, but it appears to load the
NTP.)  That's ok, right?

[mmenke, 2013-12-12 15:52:11]

Prerender LGTM.  I defer to creis on the rest.

https://codereview.chromium.org/49003011/diff/220001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/49003011/diff/220001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:105: const char* kBlankURL =
"about:blank";
Should probable just use content::kAboutBlankURL (in
content/public/common/url_constants.cc, which we already include)

[davidben, 2013-12-12 16:58:47]

https://codereview.chromium.org/49003011/diff/220001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/49003011/diff/220001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:105: const char* kBlankURL =
"about:blank";
On 2013/12/12 15:52:12, mmenke wrote:
> Should probable just use content::kAboutBlankURL (in
> content/public/common/url_constants.cc, which we already include)

Done.

https://codereview.chromium.org/49003011/diff/220001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:836:
content::OpenURLParams(GURL(kBlankURL), Referrer(),
On 2013/12/12 02:46:38, creis wrote:
> This looks like an improvement to me, though it's not identical behavior.  (I
> don't know what chrome://blank is supposed to do, but it appears to load the
> NTP.)  That's ok, right?

It gives me ERR_INVALID_URL. But when I tried asserting on chrome://blank, the
URL in the first entry came out as about:blank. Not entirely sure what's going
on there, but I think about:blank is what's wanted. Perhaps this goes through
the codepath that treats funny about: and chrome:// URLs as about:blank so that
web content can't about:inducebrowsercrashforrealz?

[Charlie Reis, 2013-12-12 17:30:34]

https://codereview.chromium.org/49003011/diff/220001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/49003011/diff/220001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:836:
content::OpenURLParams(GURL(kBlankURL), Referrer(),
On 2013/12/12 16:58:47, David Benjamin wrote:
> On 2013/12/12 02:46:38, creis wrote:
> > This looks like an improvement to me, though it's not identical behavior. 
(I
> > don't know what chrome://blank is supposed to do, but it appears to load the
> > NTP.)  That's ok, right?
> 
> It gives me ERR_INVALID_URL. But when I tried asserting on chrome://blank, the
> URL in the first entry came out as about:blank. Not entirely sure what's going
> on there, but I think about:blank is what's wanted. Perhaps this goes through
> the codepath that treats funny about: and chrome:// URLs as about:blank so
that
> web content can't about:inducebrowsercrashforrealz?

That could be FilterURL rewriting an illegal URL as about:blank, or it could be
the fact that all about: URLs get rewritten to about:blank.

Anyway, this function appears to just want to open a new tab, so either starting
at about:blank or chrome://newtab would be ok.  I've never heard of
chrome://blank, so I'm glad you've removed it from the test.

[samarth, 2013-12-13 00:50:04]

lgtm

[davidben, 2013-12-13 01:00:39]

https://codereview.chromium.org/49003011/diff/220001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/49003011/diff/220001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:836:
content::OpenURLParams(GURL(kBlankURL), Referrer(),
On 2013/12/12 17:30:35, creis wrote:
> On 2013/12/12 16:58:47, David Benjamin wrote:
> > On 2013/12/12 02:46:38, creis wrote:
> > > This looks like an improvement to me, though it's not identical behavior. 
> (I
> > > don't know what chrome://blank is supposed to do, but it appears to load
the
> > > NTP.)  That's ok, right?
> > 
> > It gives me ERR_INVALID_URL. But when I tried asserting on chrome://blank,
the
> > URL in the first entry came out as about:blank. Not entirely sure what's
going
> > on there, but I think about:blank is what's wanted. Perhaps this goes
through
> > the codepath that treats funny about: and chrome:// URLs as about:blank so
> that
> > web content can't about:inducebrowsercrashforrealz?
> 
> That could be FilterURL rewriting an illegal URL as about:blank, or it could
be
> the fact that all about: URLs get rewritten to about:blank.
> 
> Anyway, this function appears to just want to open a new tab, so either
starting
> at about:blank or chrome://newtab would be ok.  I've never heard of
> chrome://blank, so I'm glad you've removed it from the test.

(Turns out I'm actually confused and this function is unrelated to this test,
but meh. Good to use the constant anyway.)

[commit-bot: I haz the power, 2013-12-13 01:02:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/49003011/240001

[commit-bot: I haz the power, 2013-12-13 08:09:08]

Change committed as 240535