Ensure in RFH the DidFailProvisionalLoad IPCs correspond to a navigation

Ensure in RFH the DidFailProvisionalLoad IPCs correspond to a navigation

This CL adds a few checks
RenderFrameHostImpl::OnDidFailProvisionalLoadWithError to only take into
account the IPC when an actual navigation is taking place. When PlzNavigate is
enabled, we also ensure that an error page is expected to commit in the RFH.
This is a followup on https://codereview.chromium.org/1872313003.

BUG=439423
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/9b98b3bf85f5d5501362f1718ac03dd5a997aef4
Cr-Commit-Position: refs/heads/master@{#387573}

[clamy, 2016-04-13 12:50:19]

Description was changed from

==========
Ensure in RFH the DidFailProvisionalLoad IPCs correspond to a navigation

This CL adds a few checks
RenderFrameHostImpl::OnDidFailProvisionalLoadWithError to only take into
account the IPC when an actual navigation is taking place. When PlzNavigate is
enabled, we also ensure that an error page is expected to commit in the RFH.
This is a followup on https://codereview.chromium.org/1872313003.

BUG=439423
==========

to

==========
Ensure in RFH the DidFailProvisionalLoad IPCs correspond to a navigation

This CL adds a few checks
RenderFrameHostImpl::OnDidFailProvisionalLoadWithError to only take into
account the IPC when an actual navigation is taking place. When PlzNavigate is
enabled, we also ensure that an error page is expected to commit in the RFH.
This is a followup on https://codereview.chromium.org/1872313003.

BUG=439423
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-04-13 12:52:09]

Description was changed from

==========
Ensure in RFH the DidFailProvisionalLoad IPCs correspond to a navigation

This CL adds a few checks
RenderFrameHostImpl::OnDidFailProvisionalLoadWithError to only take into
account the IPC when an actual navigation is taking place. When PlzNavigate is
enabled, we also ensure that an error page is expected to commit in the RFH.
This is a followup on https://codereview.chromium.org/1872313003.

BUG=439423
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ensure in RFH the DidFailProvisionalLoad IPCs correspond to a navigation

This CL adds a few checks
RenderFrameHostImpl::OnDidFailProvisionalLoadWithError to only take into
account the IPC when an actual navigation is taking place. When PlzNavigate is
enabled, we also ensure that an error page is expected to commit in the RFH.
This is a followup on https://codereview.chromium.org/1872313003.

BUG=439423
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-04-13 12:52:10]

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

[clamy, 2016-04-13 12:52:24]

@creis: PTAL

[Charlie Reis, 2016-04-13 17:16:27]

Thanks for splitting them!  LGTM unless you think the race I mention below is a
concern.

Either way, let's put this one in a later Canary than
https://codereview.chromium.org/1872313003/.

https://codereview.chromium.org/1880423002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1880423002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:903: return;
Sanity check: We did still call navigator()->DidFailProvisionalLoadWithError
when there was no handle before, and that has sensitive logic for avoiding URL
spoofs.  I think we're still safe from URL spoofs after your change, since the
renderer will be killed.

Still, let's be sure to check the histograms for whether this case happens in
practice.  :)  I wouldn't be surprised if a race makes it possible (e.g.,
something in the browser process cancels the navigation while a DidFail IPC is
in flight).  We wouldn't want a legitimate race to kill the renderer.

https://codereview.chromium.org/1880423002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:918: // PlzNavigate: this
has already done in NavigationRequest::OnRequestFailed.
nit: has already been done

[Charlie Reis, 2016-04-13 20:23:16]

Ah, be aware of conflicts in bad_message.h from
https://codereview.chromium.org/1832553002/, which I just reviewed.

[clamy, 2016-04-14 12:03:03]

clamy@chromium.org changed reviewers:
+ isherman@chromium.org

[clamy, 2016-04-14 12:03:04]

Thanks! I've rebased on top of it.
@isherman: PTAL at the changes in histograms.xml

[Ilya Sherman, 2016-04-14 21:25:36]

histograms.xml lgtm

[clamy, 2016-04-15 09:54:18]

Thanks!

[clamy, 2016-04-15 09:54:22]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-04-15 09:54:23]

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

[commit-bot: I haz the power, 2016-04-15 09:54:41]

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

[commit-bot: I haz the power, 2016-04-15 11:15:54]

Description was changed from

==========
Ensure in RFH the DidFailProvisionalLoad IPCs correspond to a navigation

This CL adds a few checks
RenderFrameHostImpl::OnDidFailProvisionalLoadWithError to only take into
account the IPC when an actual navigation is taking place. When PlzNavigate is
enabled, we also ensure that an error page is expected to commit in the RFH.
This is a followup on https://codereview.chromium.org/1872313003.

BUG=439423
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ensure in RFH the DidFailProvisionalLoad IPCs correspond to a navigation

This CL adds a few checks
RenderFrameHostImpl::OnDidFailProvisionalLoadWithError to only take into
account the IPC when an actual navigation is taking place. When PlzNavigate is
enabled, we also ensure that an error page is expected to commit in the RFH.
This is a followup on https://codereview.chromium.org/1872313003.

BUG=439423
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-04-15 11:15:55]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-15 11:17:22]

Description was changed from

==========
Ensure in RFH the DidFailProvisionalLoad IPCs correspond to a navigation

This CL adds a few checks
RenderFrameHostImpl::OnDidFailProvisionalLoadWithError to only take into
account the IPC when an actual navigation is taking place. When PlzNavigate is
enabled, we also ensure that an error page is expected to commit in the RFH.
This is a followup on https://codereview.chromium.org/1872313003.

BUG=439423
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ensure in RFH the DidFailProvisionalLoad IPCs correspond to a navigation

This CL adds a few checks
RenderFrameHostImpl::OnDidFailProvisionalLoadWithError to only take into
account the IPC when an actual navigation is taking place. When PlzNavigate is
enabled, we also ensure that an error page is expected to commit in the RFH.
This is a followup on https://codereview.chromium.org/1872313003.

BUG=439423
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/9b98b3bf85f5d5501362f1718ac03dd5a997aef4
Cr-Commit-Position: refs/heads/master@{#387573}
==========

[commit-bot: I haz the power, 2016-04-15 11:17:24]

Patchset 2 (id:??) landed as
https://crrev.com/9b98b3bf85f5d5501362f1718ac03dd5a997aef4
Cr-Commit-Position: refs/heads/master@{#387573}