Don't rely on the pending NavigationEntry for location.replace.

Don't rely on the pending NavigationEntry for location.replace.

This CL adds a should_replace_current_entry param to commit IPCs,
and it ensures the WebDataSource is accurate on browser-initiated
navigations.  To keep the CL manageable, it does not yet change
same-process location.replace navigations from EXISTING_PAGE to
NEW_PAGE, though we want to do that as well.

This goes further than the previous attempt and removes the old
code that depended on the pending entry at commit time.

This also corrects a bug where we were using the wrong pending
entry to decide whether to replace in some cases.

TBR=nasko (for IPC)
BUG=317872, 593153
TEST=See bug 593153 for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/865ad44678b14ada619057ec2afb0d381f8c5ddd
Cr-Commit-Position: refs/heads/master@{#381233}

[Charlie Reis, 2016-03-13 22:32:59]

Description was changed from

==========
Don't rely on the pending NavigationEntry for location.replace.

This CL adds a should_replace_current_entry param to commit IPCs,
and it ensures the WebDataSource is accurate on browser-initiated
navigations.  To keep the CL manageable, it does not yet change
same-process location.replace navigations from EXISTING_PAGE to
NEW_PAGE, though we want to do that as well.

This goes further than the previous attempt and removes the old
code that depended on the pending entry at commit time.

BUG=317872
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't rely on the pending NavigationEntry for location.replace.

This CL adds a should_replace_current_entry param to commit IPCs,
and it ensures the WebDataSource is accurate on browser-initiated
navigations.  To keep the CL manageable, it does not yet change
same-process location.replace navigations from EXISTING_PAGE to
NEW_PAGE, though we want to do that as well.

This goes further than the previous attempt and removes the old
code that depended on the pending entry at commit time.

This also corrects a bug where we were using the wrong pending
entry to decide whether to replace in some cases.

BUG=317872, 593153
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-14 18:58:57]

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

[Charlie Reis, 2016-03-14 18:58:58]

Avi, can you review?  This is a second attempt after
https://codereview.chromium.org/1672373002 was reverted for causing lots of
crashes (https://crbug.com/585499).

Those crashes were due to https://crbug.com/593153, which meant that the old way
of doing things (i.e., relying on the pending NavigationEntry) was wrong.  I
thought I would fix the old way before trying to land this one again, but that
proved non-trivial on Android (see https://codereview.chromium.org/1777903003/
patchset 5).  Instead, let's just land the new approach and kill the old one.

For this CL, patch set 1 is a rebased version of my original fix.  I'm now
ripping out the old way in NavigationControllerImpl::RendererDidNavigate (and in
RenderFrameImpl), and I've included a new browser test to verify that 593153 is
fixed (OtherCommitDuringPendingEntryWithReplacement).

You can see this in the diffs between patches 1 and 4, though I might recommend
reviewing the whole thing again.  Thanks!

[Avi (use Gerrit), 2016-03-14 20:49:59]

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

https://codereview.chromium.org/1794513003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:857:
details->did_replace_entry = params.should_replace_current_entry;
When we specify the error page load, do we copy over the value of
should-replace? You're removing failed_pending_entry_should_replace_ but it's
not immediately obvious to me that the value that you're putting in now is
correct.

[Charlie Reis, 2016-03-14 21:18:19]

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

https://codereview.chromium.org/1794513003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:857:
details->did_replace_entry = params.should_replace_current_entry;
On 2016/03/14 20:49:59, Avi wrote:
> When we specify the error page load, do we copy over the value of
> should-replace? You're removing failed_pending_entry_should_replace_ but it's
> not immediately obvious to me that the value that you're putting in now is
> correct.

In which context?  In general, if the browser process set
should_replace_current_entry on the pending entry before, it will now get passed
down to the renderer, used, and passed back up at commit time.

For error pages, we still do the intended replacement in
NavigationControllerBrowserTest.ErrorPageReplacement (and pass the test), which
is the test you added for failed_pending_entry_should_replace_ in
https://codereview.chromium.org/1048963002.

I'm not certain if that covers all the necessary cases for that bool, though. 
Let me know if it looks like there's something I'm missing.

[Avi (use Gerrit), 2016-03-15 00:11:31]

On 2016/03/14 21:18:19, Charlie Reis (slow til 3-15) wrote:
> NavigationControllerBrowserTest.ErrorPageReplacement

That was the situation I was thinking of. Yay me for making it a test.

LGTM then. This code is much nicer. (And good luck in your landing it.)

[Charlie Reis, 2016-03-15 16:37:00]

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

[Charlie Reis, 2016-03-15 16:37:01]

Thanks.  I'll TBR Nasko for the frame_messages.h change, since it's the same as
last time.

[Charlie Reis, 2016-03-15 16:37:24]

Description was changed from

==========
Don't rely on the pending NavigationEntry for location.replace.

This CL adds a should_replace_current_entry param to commit IPCs,
and it ensures the WebDataSource is accurate on browser-initiated
navigations.  To keep the CL manageable, it does not yet change
same-process location.replace navigations from EXISTING_PAGE to
NEW_PAGE, though we want to do that as well.

This goes further than the previous attempt and removes the old
code that depended on the pending entry at commit time.

This also corrects a bug where we were using the wrong pending
entry to decide whether to replace in some cases.

BUG=317872, 593153
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't rely on the pending NavigationEntry for location.replace.

This CL adds a should_replace_current_entry param to commit IPCs,
and it ensures the WebDataSource is accurate on browser-initiated
navigations.  To keep the CL manageable, it does not yet change
same-process location.replace navigations from EXISTING_PAGE to
NEW_PAGE, though we want to do that as well.

This goes further than the previous attempt and removes the old
code that depended on the pending entry at commit time.

This also corrects a bug where we were using the wrong pending
entry to decide whether to replace in some cases.

TBR=nasko (for IPC)
BUG=317872, 593153
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-15 16:38:05]

Description was changed from

==========
Don't rely on the pending NavigationEntry for location.replace.

This CL adds a should_replace_current_entry param to commit IPCs,
and it ensures the WebDataSource is accurate on browser-initiated
navigations.  To keep the CL manageable, it does not yet change
same-process location.replace navigations from EXISTING_PAGE to
NEW_PAGE, though we want to do that as well.

This goes further than the previous attempt and removes the old
code that depended on the pending entry at commit time.

This also corrects a bug where we were using the wrong pending
entry to decide whether to replace in some cases.

TBR=nasko (for IPC)
BUG=317872, 593153
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't rely on the pending NavigationEntry for location.replace.

This CL adds a should_replace_current_entry param to commit IPCs,
and it ensures the WebDataSource is accurate on browser-initiated
navigations.  To keep the CL manageable, it does not yet change
same-process location.replace navigations from EXISTING_PAGE to
NEW_PAGE, though we want to do that as well.

This goes further than the previous attempt and removes the old
code that depended on the pending entry at commit time.

This also corrects a bug where we were using the wrong pending
entry to decide whether to replace in some cases.

TBR=nasko (for IPC)
BUG=317872, 593153
TEST=See bug 593153 for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-15 16:38:25]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2016-03-15 16:38:26]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1794513003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-15 16:38:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1794513003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1794513003/80001

[commit-bot: I haz the power, 2016-03-15 16:43:17]

Description was changed from

==========
Don't rely on the pending NavigationEntry for location.replace.

This CL adds a should_replace_current_entry param to commit IPCs,
and it ensures the WebDataSource is accurate on browser-initiated
navigations.  To keep the CL manageable, it does not yet change
same-process location.replace navigations from EXISTING_PAGE to
NEW_PAGE, though we want to do that as well.

This goes further than the previous attempt and removes the old
code that depended on the pending entry at commit time.

This also corrects a bug where we were using the wrong pending
entry to decide whether to replace in some cases.

TBR=nasko (for IPC)
BUG=317872, 593153
TEST=See bug 593153 for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't rely on the pending NavigationEntry for location.replace.

This CL adds a should_replace_current_entry param to commit IPCs,
and it ensures the WebDataSource is accurate on browser-initiated
navigations.  To keep the CL manageable, it does not yet change
same-process location.replace navigations from EXISTING_PAGE to
NEW_PAGE, though we want to do that as well.

This goes further than the previous attempt and removes the old
code that depended on the pending entry at commit time.

This also corrects a bug where we were using the wrong pending
entry to decide whether to replace in some cases.

TBR=nasko (for IPC)
BUG=317872, 593153
TEST=See bug 593153 for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-03-15 16:43:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-15 16:45:18]

Description was changed from

==========
Don't rely on the pending NavigationEntry for location.replace.

This CL adds a should_replace_current_entry param to commit IPCs,
and it ensures the WebDataSource is accurate on browser-initiated
navigations.  To keep the CL manageable, it does not yet change
same-process location.replace navigations from EXISTING_PAGE to
NEW_PAGE, though we want to do that as well.

This goes further than the previous attempt and removes the old
code that depended on the pending entry at commit time.

This also corrects a bug where we were using the wrong pending
entry to decide whether to replace in some cases.

TBR=nasko (for IPC)
BUG=317872, 593153
TEST=See bug 593153 for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't rely on the pending NavigationEntry for location.replace.

This CL adds a should_replace_current_entry param to commit IPCs,
and it ensures the WebDataSource is accurate on browser-initiated
navigations.  To keep the CL manageable, it does not yet change
same-process location.replace navigations from EXISTING_PAGE to
NEW_PAGE, though we want to do that as well.

This goes further than the previous attempt and removes the old
code that depended on the pending entry at commit time.

This also corrects a bug where we were using the wrong pending
entry to decide whether to replace in some cases.

TBR=nasko (for IPC)
BUG=317872, 593153
TEST=See bug 593153 for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/865ad44678b14ada619057ec2afb0d381f8c5ddd
Cr-Commit-Position: refs/heads/master@{#381233}
==========

[commit-bot: I haz the power, 2016-03-15 16:45:20]

Patchset 5 (id:??) landed as
https://crrev.com/865ad44678b14ada619057ec2afb0d381f8c5ddd
Cr-Commit-Position: refs/heads/master@{#381233}