Don't clear the forward history when replacing entries.

Don't clear the forward history when replacing entries.

This is a prerequisite to fixing the classification of location.replace,
which should generate a new NavigationEntry and not update the old one
in place.

BUG=317872
TEST=Forward history still in place after cross-process location.replace.

Committed: https://crrev.com/ee17e93a2684e2420ab932432edce920b3355a8e
Cr-Commit-Position: refs/heads/master@{#339281}

[Charlie Reis, 2015-07-16 19:29:53]

creis@chromium.org changed reviewers:
+ avi@chromium.org

[Charlie Reis, 2015-07-16 19:29:54]

Avi, can you take a look?

This will be needed as I fix https://codereview.chromium.org/1225593003/ to use
NEW_PAGE rather than EXISTING_PAGE.

It's a little crazy to me that this was both wrong and untested for so long.  I
haven't found any cases that pass replace=true to InsertAndReplaceEntry that
actually expect the forward history to get pruned, and this change seems
consistent with other browsers and how we behave on in-process location.replace
(which classifies as EXISTING_PAGE).

[Avi (use Gerrit), 2015-07-16 19:58:11]

On 2015/07/16 19:29:54, Charlie Reis wrote:
> It's a little crazy to me that this was both wrong and untested for so long.

But not surprising.

[Avi (use Gerrit), 2015-07-16 20:00:16]

https://codereview.chromium.org/1245433002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1245433002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:201:
EXPECT_TRUE(controller.CanGoForward());
Are you sure that this change keeps testing the same code in RenderView that the
old code did? More precisely, this is testing the "&&
!navigation_state->start_params().should_replace_current_entry" part of
RenderFrameImpl::didCommitProvisionalLoad. Can you delete that clause and make
sure that this test fails?

[Charlie Reis, 2015-07-16 20:17:43]

https://codereview.chromium.org/1245433002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1245433002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:201:
EXPECT_TRUE(controller.CanGoForward());
On 2015/07/16 20:00:16, Avi wrote:
> Are you sure that this change keeps testing the same code in RenderView that
the
> old code did? More precisely, this is testing the "&&
> !navigation_state->start_params().should_replace_current_entry" part of
> RenderFrameImpl::didCommitProvisionalLoad. Can you delete that clause and make
> sure that this test fails?

Good point.  The fact that I only had one additional entry made the expectation
(2) the same as the buggy answer from the renderer (2).  Sure enough, the test
passed when removing that clause.

I updated it in two ways: I preserved your original test (causing it to fail
without the RenderFrameImpl clause), and I changed my new case to have 3 entries
in history (which independently fails without the RenderFrameImpl clause).

[Charlie Reis, 2015-07-16 21:29:29]

Looks like I've got a memory leak; I'll take a look.

[Charlie Reis, 2015-07-16 23:51:15]

I'm sad that ScopedVectors behave this way, but I think this should fix it. 
PTAL.

https://codereview.chromium.org/1245433002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1245433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1660: // ScopedVectors
don't automatically delete the replaced value, so make sure
Nick and I took a look, and there doesn't appear to be a safe way to replace an
entry in a ScopedVector without leaking the old value, since operator[] deals
with direct references in the underlying array.  This isn't great, but it makes
sure the old value gets deleted, and we couldn't think of a cleaner option.

[Avi (use Gerrit), 2015-07-17 02:17:25]

lgtm

https://codereview.chromium.org/1245433002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1245433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1660: // ScopedVectors
don't automatically delete the replaced value, so make sure
On 2015/07/16 23:51:14, Charlie Reis wrote:
> Nick and I took a look, and there doesn't appear to be a safe way to replace
an
> entry in a ScopedVector without leaking the old value, since operator[] deals
> with direct references in the underlying array.  This isn't great, but it
makes
> sure the old value gets deleted, and we couldn't think of a cleaner option.

Yeah. As soon as we can use the C++11 library we'll switch all use of
ScopedVector to std::vector<std::unique_ptr>. Until then, this is about as good
as we can make it.

[dcheng, 2015-07-17 03:49:59]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2015-07-17 03:49:59]

https://codereview.chromium.org/1245433002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1245433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1660: // ScopedVectors
don't automatically delete the replaced value, so make sure
On 2015/07/17 at 02:17:24, Avi wrote:
> On 2015/07/16 23:51:14, Charlie Reis wrote:
> > Nick and I took a look, and there doesn't appear to be a safe way to replace
an
> > entry in a ScopedVector without leaking the old value, since operator[]
deals
> > with direct references in the underlying array.  This isn't great, but it
makes
> > sure the old value gets deleted, and we couldn't think of a cleaner option.
> 
> Yeah. As soon as we can use the C++11 library we'll switch all use of
ScopedVector to std::vector<std::unique_ptr>. Until then, this is about as good
as we can make it.

An alternate way of doing this which maintains the unique ownership invariant of
scoped_ptr is this:
entries_.push_back(entry);
using std::swap;
swap(entries[last_committed_entry_index_], entries[entries.size() - 1]);
entries_.pop_back();

[ncarter (slow), 2015-07-17 16:54:19]

nick@chromium.org changed reviewers:
+ nick@chromium.org

[ncarter (slow), 2015-07-17 16:54:20]

https://codereview.chromium.org/1245433002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1245433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1660: // ScopedVectors
don't automatically delete the replaced value, so make sure
On 2015/07/17 03:49:59, dcheng wrote:
> On 2015/07/17 at 02:17:24, Avi wrote:
> > On 2015/07/16 23:51:14, Charlie Reis wrote:
> > > Nick and I took a look, and there doesn't appear to be a safe way to
replace
> an
> > > entry in a ScopedVector without leaking the old value, since operator[]
> deals
> > > with direct references in the underlying array.  This isn't great, but it
> makes
> > > sure the old value gets deleted, and we couldn't think of a cleaner
option.
> > 
> > Yeah. As soon as we can use the C++11 library we'll switch all use of
> ScopedVector to std::vector<std::unique_ptr>. Until then, this is about as
good
> as we can make it.
> 
> An alternate way of doing this which maintains the unique ownership invariant
of
> scoped_ptr is this:
> entries_.push_back(entry);
> using std::swap;
> swap(entries[last_committed_entry_index_], entries[entries.size() - 1]);
> entries_.pop_back();

reinterpret_cast<scoped_ptr<NavigationEntryImpl>&>(entries[last_commited_entry_index_])
= entry.Pass();

, obviously. Although I'm kidding, this is actually how the google3
PointerVector's operator[] worked until recently. (cl/60585078)

[Charlie Reis, 2015-07-17 17:00:11]

https://codereview.chromium.org/1245433002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1245433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1660: // ScopedVectors
don't automatically delete the replaced value, so make sure
On 2015/07/17 16:54:20, ncarter wrote:
> On 2015/07/17 03:49:59, dcheng wrote:
> > On 2015/07/17 at 02:17:24, Avi wrote:
> > > On 2015/07/16 23:51:14, Charlie Reis wrote:
> > > > Nick and I took a look, and there doesn't appear to be a safe way to
> replace
> > an
> > > > entry in a ScopedVector without leaking the old value, since operator[]
> > deals
> > > > with direct references in the underlying array.  This isn't great, but
it
> > makes
> > > > sure the old value gets deleted, and we couldn't think of a cleaner
> option.
> > > 
> > > Yeah. As soon as we can use the C++11 library we'll switch all use of
> > ScopedVector to std::vector<std::unique_ptr>. Until then, this is about as
> good
> > as we can make it.
> > 
> > An alternate way of doing this which maintains the unique ownership
invariant
> of
> > scoped_ptr is this:
> > entries_.push_back(entry);
> > using std::swap;
> > swap(entries[last_committed_entry_index_], entries[entries.size() - 1]);
> > entries_.pop_back();
> 
>
reinterpret_cast<scoped_ptr<NavigationEntryImpl>&>(entries[last_commited_entry_index_])
> = entry.Pass();
> 
> , obviously. Although I'm kidding, this is actually how the google3
> PointerVector's operator[] worked until recently. (cl/60585078)

Heh, I've stumbled into a fun corner case, I see.  :)

I'm inclined to avoid the extra array operations and stick with the current
approach if we'll be able to use the C++11 library before too long. 
std::vector<std::unique_ptr> seems like it will be the best option.

[Charlie Reis, 2015-07-17 17:31:48]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-07-17 17:33:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1245433002/60001

[commit-bot: I haz the power, 2015-07-17 17:56:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-17 17:57:42]

Patchset 4 (id:??) landed as
https://crrev.com/ee17e93a2684e2420ab932432edce920b3355a8e
Cr-Commit-Position: refs/heads/master@{#339281}