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.

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

Committed: https://crrev.com/73c9e79d72b0781c3f22b5c61e95ba5170f4f4a1
Cr-Commit-Position: refs/heads/master@{#374223}

[Charlie Reis, 2016-02-06 05:17:36]

Description was changed from

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

BUG=317812
TEST=No behavior change.
==========

to

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

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

[Charlie Reis, 2016-02-08 04:30:19]

Description was changed from

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

BUG=317812
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.

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

[Charlie Reis, 2016-02-08 07:02:38]

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

[Charlie Reis, 2016-02-08 07:02:38]

Avi, can you take a look?  I'm splitting this out from the larger effort to
reclassify all location.replaces as New+replace
(https://codereview.chromium.org/1250163002/), since that was getting unwieldy.

This changes the plumbing and makes it so that we don't rely on the pending
entry anymore, which is sufficient for the next step of shortcutting transfer
navigations (https://crbug.com/495161).

[Avi (use Gerrit), 2016-02-08 15:41:14]

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.

BUG=317812
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.

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

[Avi (use Gerrit), 2016-02-08 15:41:32]

FYI I fixed your bug number; it was wrong. Looking...

[Avi (use Gerrit), 2016-02-08 16:02:29]

LGTM

The code looks correct, but like all navigation CLs it's pretty high-risk. I'll
cross my fingers for you.

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

https://codereview.chromium.org/1672373002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1012: // requires
updating several test expectations.  See https://crbug.com/317812.
Wrong bug number; you mean 317872.

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

https://codereview.chromium.org/1672373002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:659: // See
https://crbug.com/317812.
Wrong bug #; 317872.

[Charlie Reis, 2016-02-08 17:13:21]

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

[Charlie Reis, 2016-02-08 17:13:21]

On 2016/02/08 16:02:29, Avi wrote:
> LGTM
> 
> The code looks correct, but like all navigation CLs it's pretty high-risk.
I'll
> cross my fingers for you.
> 

Thanks!  I'll certainly be watching the crash reports for the new checks, in the
hopes of removing them.  :)

Nasko, can you review frame_messages.h for IPC owners?

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

https://codereview.chromium.org/1672373002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1012: // requires
updating several test expectations.  See https://crbug.com/317812.
On 2016/02/08 16:02:28, Avi wrote:
> Wrong bug number; you mean 317872.

Done.

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

https://codereview.chromium.org/1672373002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:659: // See
https://crbug.com/317812.
On 2016/02/08 16:02:29, Avi wrote:
> Wrong bug #; 317872.

Done.

[Charlie Reis, 2016-02-08 17:13:48]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 17:14:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1672373002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1672373002/100001

[commit-bot: I haz the power, 2016-02-08 17:55:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-08 17:55:58]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[nasko, 2016-02-08 18:41:12]

IPC LGTM

[Charlie Reis, 2016-02-08 18:43:41]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2016-02-08 18:43:41]

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/1672373002/#ps100001
(title: "Fix bug numbers")

[commit-bot: I haz the power, 2016-02-08 18:47:37]

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

[commit-bot: I haz the power, 2016-02-08 20:19:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-08 20:19:09]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Charlie Reis, 2016-02-08 20:23:25]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-02-08 20:26:09]

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

[commit-bot: I haz the power, 2016-02-09 00:08:32]

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.

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.

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

[commit-bot: I haz the power, 2016-02-09 00:08:33]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-09 00:09:50]

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.

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.

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

Committed: https://crrev.com/73c9e79d72b0781c3f22b5c61e95ba5170f4f4a1
Cr-Commit-Position: refs/heads/master@{#374223}
==========

[commit-bot: I haz the power, 2016-02-09 00:09:51]

Patchset 6 (id:??) landed as
https://crrev.com/73c9e79d72b0781c3f22b5c61e95ba5170f4f4a1
Cr-Commit-Position: refs/heads/master@{#374223}

[Charlie Reis, 2016-02-09 19:11:51]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1679383002/ by creis@chromium.org.

The reason for reverting is: Causing crashes in
NavigationControllerImpl::RendererDidNavigate (https://crbug.com/585499) and
RenderFrameImpl::willSendRequest (https://crbug.com/585520).  Will investigate
before re-landing..