Add a check for mismatching URL and origin in the renderer process.

Add a check for mismatching URL and origin in the renderer process.

The renderer process kills with reason RFH_INVALID_ORIGIN_ON_COMMIT are
checking parameters passed from the renderer process. However, we are
lacking a bunch of information since it is in the browser process and
it is hard to reason about what the state was in the renderer.

We can also try to implement similar checking in the renderer process,
so we can catch these mismatches closer to where we have more data.
This CL implements some of the checks performed browser side, but
it is a set that should catch the majority of the cases we have seen
in crash reports.

BUG=628677
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/0be4d34d5c65243b03d4fc6469a78b32eb1403c0
Cr-Commit-Position: refs/heads/master@{#406145}

[Charlie Reis, 2016-07-15 17:49:17]

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

[Charlie Reis, 2016-07-15 17:49:18]

https://codereview.chromium.org/2151323003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2151323003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4777: // https://crbug.com/628677 is
understood.
Why not leave it in?  :)  Seems like it could help catch future bugs, if this is
never expected to be hit.

[nasko, 2016-07-15 17:51:18]

https://codereview.chromium.org/2151323003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2151323003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4777: // https://crbug.com/628677 is
understood.
On 2016/07/15 17:49:18, Charlie Reis wrote:
> Why not leave it in?  :)  Seems like it could help catch future bugs, if this
is
> never expected to be hit.

Sure :).

[nasko, 2016-07-15 18:21:41]

Description was changed from

==========
Add a check for mismatching URL and origin in the renderer process.

The renderer process kills with reason RFH_INVALID_ORIGIN_ON_COMMIT are
checking parameters passed from the renderer process. However, we are
lacking a bunch of information since it is in the browser process and
it is hard to reason about what the state was in the renderer.

We can also try to implement similar checking in the renderer process,
so we can catch these mismatches closer to where we have more data.
This CL implements one of the many checks performed browser side, but
it is the one that should catch the majority of the cases we have seen
in crash reports.

BUG=628677
==========

to

==========
Add a check for mismatching URL and origin in the renderer process.

The renderer process kills with reason RFH_INVALID_ORIGIN_ON_COMMIT are
checking parameters passed from the renderer process. However, we are
lacking a bunch of information since it is in the browser process and
it is hard to reason about what the state was in the renderer.

We can also try to implement similar checking in the renderer process,
so we can catch these mismatches closer to where we have more data.
This CL implements some of the checks performed browser side, but
it is a set that should catch the majority of the cases we have seen
in crash reports.

BUG=628677
==========

[nasko, 2016-07-15 18:21:53]

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

[commit-bot: I haz the power, 2016-07-15 18:22:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 19:20:37]

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

[commit-bot: I haz the power, 2016-07-15 19:20:38]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-07-15 20:22:39]

Hey Charlie,
Can you review the CL at this time? It is properly formatted and passed all
tryjobs.
Thanks!
Nasko

[Charlie Reis, 2016-07-15 20:33:17]

https://codereview.chromium.org/2151323003/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2151323003/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4777:
render_view_->GetWebkitPreferences().web_security_enabled) {
Ah, good point.  We'll need to skip the check for file URLs when
prefs.allow_universal_access_from_file_urls is true, too, or we'll get a bunch
of Android WebView crash reports.  

We're basically encoding RFHI::CanCommitOrigin into here, but I think I'm ok
with that.  Maybe put a reference to that method in the comment above?

[nasko, 2016-07-15 20:56:31]

https://codereview.chromium.org/2151323003/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2151323003/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4777:
render_view_->GetWebkitPreferences().web_security_enabled) {
On 2016/07/15 20:33:17, Charlie Reis wrote:
> Ah, good point.  We'll need to skip the check for file URLs when
> prefs.allow_universal_access_from_file_urls is true, too, or we'll get a bunch
> of Android WebView crash reports.  

Such check added. However, all tests passed before I added it, so it indicates
that we probably have no test coverage for this :(.
 
> We're basically encoding RFHI::CanCommitOrigin into here, but I think I'm ok
> with that.  Maybe put a reference to that method in the comment above?

Done. Yeah, it isn't nice, but we need it to at least diagnose the kills. We can
decide whether we want to keep it longer term later on.

[nasko, 2016-07-15 20:56:39]

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

[commit-bot: I haz the power, 2016-07-15 20:57:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2016-07-15 21:01:17]

LGTM, though I might recommend adding the test for universal file access in this
CL.  That condition is just complex enough that a test would be worthwhile (not
to mention would have caught the bug in the earlier patch set).

[commit-bot: I haz the power, 2016-07-15 21:43:16]

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

[commit-bot: I haz the power, 2016-07-15 21:43:17]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-07-15 22:23:59]

Description was changed from

==========
Add a check for mismatching URL and origin in the renderer process.

The renderer process kills with reason RFH_INVALID_ORIGIN_ON_COMMIT are
checking parameters passed from the renderer process. However, we are
lacking a bunch of information since it is in the browser process and
it is hard to reason about what the state was in the renderer.

We can also try to implement similar checking in the renderer process,
so we can catch these mismatches closer to where we have more data.
This CL implements some of the checks performed browser side, but
it is a set that should catch the majority of the cases we have seen
in crash reports.

BUG=628677
==========

to

==========
Add a check for mismatching URL and origin in the renderer process.

The renderer process kills with reason RFH_INVALID_ORIGIN_ON_COMMIT are
checking parameters passed from the renderer process. However, we are
lacking a bunch of information since it is in the browser process and
it is hard to reason about what the state was in the renderer.

We can also try to implement similar checking in the renderer process,
so we can catch these mismatches closer to where we have more data.
This CL implements some of the checks performed browser side, but
it is a set that should catch the majority of the cases we have seen
in crash reports.

BUG=628677
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2016-07-15 22:25:49]

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

[commit-bot: I haz the power, 2016-07-15 22:26:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2016-07-15 22:30:40]

Awesome, thanks for the test!  LGTM.

https://codereview.chromium.org/2151323003/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2151323003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2825:
EXPECT_EQ(2, web_contents->GetController().GetEntryCount());
Maybe verify that the renderer process is still live?  (I'm guessing the entry
count check was the part that failed, but that's a bit subtle.)

[nasko, 2016-07-15 22:35:29]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-07-15 22:35:29]

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/2151323003/#ps120001
(title: "Add liveness check.")

[nasko, 2016-07-15 22:35:54]

https://codereview.chromium.org/2151323003/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2151323003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2825:
EXPECT_EQ(2, web_contents->GetController().GetEntryCount());
On 2016/07/15 22:30:40, Charlie Reis wrote:
> Maybe verify that the renderer process is still live?  (I'm guessing the entry
> count check was the part that failed, but that's a bit subtle.)

Done.

[commit-bot: I haz the power, 2016-07-15 22:36:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 23:19:59]

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

[commit-bot: I haz the power, 2016-07-15 23:20:00]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nasko, 2016-07-18 20:58:15]

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

[commit-bot: I haz the power, 2016-07-18 20:59:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 22:13:10]

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

[commit-bot: I haz the power, 2016-07-18 22:13:11]

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

[nasko, 2016-07-18 22:21:33]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-07-18 22:21:33]

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/2151323003/#ps140001
(title: "Disable the test on Mac, until more debugging can be done.")

[commit-bot: I haz the power, 2016-07-18 22:23:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 23:43:59]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-18 23:44:02]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-18 23:45:42]

Description was changed from

==========
Add a check for mismatching URL and origin in the renderer process.

The renderer process kills with reason RFH_INVALID_ORIGIN_ON_COMMIT are
checking parameters passed from the renderer process. However, we are
lacking a bunch of information since it is in the browser process and
it is hard to reason about what the state was in the renderer.

We can also try to implement similar checking in the renderer process,
so we can catch these mismatches closer to where we have more data.
This CL implements some of the checks performed browser side, but
it is a set that should catch the majority of the cases we have seen
in crash reports.

BUG=628677
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add a check for mismatching URL and origin in the renderer process.

The renderer process kills with reason RFH_INVALID_ORIGIN_ON_COMMIT are
checking parameters passed from the renderer process. However, we are
lacking a bunch of information since it is in the browser process and
it is hard to reason about what the state was in the renderer.

We can also try to implement similar checking in the renderer process,
so we can catch these mismatches closer to where we have more data.
This CL implements some of the checks performed browser side, but
it is a set that should catch the majority of the cases we have seen
in crash reports.

BUG=628677
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/0be4d34d5c65243b03d4fc6469a78b32eb1403c0
Cr-Commit-Position: refs/heads/master@{#406145}
==========

[commit-bot: I haz the power, 2016-07-18 23:45:43]

Patchset 8 (id:??) landed as
https://crrev.com/0be4d34d5c65243b03d4fc6469a78b32eb1403c0
Cr-Commit-Position: refs/heads/master@{#406145}