Grant permission to the base url when loadDataWithBaseURL is called.

Grant permission to the base url when loadDataWithBaseURL is called.

When there's a base URL specified for the data URL, we also need to
grant access to the base URL. This allows file: and other unexpected
schemes to be accepted at commit time and during CORS checks (e.g., for
font requests).

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

Committed: https://crrev.com/4bdb4f88f8197bebe32ce3480a3851201f648a8b
Cr-Commit-Position: refs/heads/master@{#407867}

[hush (inactive), 2016-07-19 21:53:24]

Description was changed from

==========
Grant permission to the base url with loadDataWithBaseURL is called.

This is needed because font requests use the base url as the origin.

BUG=627564
==========

to

==========
Grant permission to the base url with loadDataWithBaseURL is called.

This is needed because font requests use the base url as the origin.

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

[hush (inactive), 2016-07-19 21:57:09]

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

[hush (inactive), 2016-07-19 21:57:09]

PTAL

[hush (inactive), 2016-07-21 17:50:04]

ping?

[clamy, 2016-07-22 14:11:22]

Description was changed from

==========
Grant permission to the base url with loadDataWithBaseURL is called.

This is needed because font requests use the base url as the origin.

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

to

==========
Grant permission to the base url with loadDataWithBaseURL is called.

This is needed because font requests use the base url as the origin.

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

[clamy, 2016-07-22 14:11:23]

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

[clamy, 2016-07-22 14:12:28]

@nasko: PTAL. This touches child process permissions, so it needs security
review.

[nasko, 2016-07-22 18:17:55]

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

[nasko, 2016-07-22 18:17:55]

Hey Charlie,
Can you take this one? I think you are way more familiar with
loadDataWithBaseURL than myself.
Thanks!
Nasko

[Charlie Reis, 2016-07-22 18:31:49]

Thanks, I'm happy with this.  Can you update the CL description similarly to the
request below?  (There's also a typo: s/with/when/)

Also, is there a way to add a test for this?  It might be as easy as making an
XHR from a page loaded with a base URL with a custom scheme, and verifying that
the renderer isn't killed.  I imagine you could do that in a test similar to
NavigationControllerBrowserTest's LoadDataWithBaseURL, with an ExecuteScript to
do the XHR.

Also, let's run try jobs to be sure this doesn't affect other
LoadDataWithBaseURL tests.  :)

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

https://codereview.chromium.org/2165783003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:2814: // as the origin for
font requests.
Let's update the comment, because CORS is not just about font requests (and
because the previous file: comment still applies).  Something like:

When there's a base URL specified for the data URL, we also need to grant access
to the base URL.  This allows file: and other unexpected schemes to be accepted
at commit time and during CORS checks (e.g., for font requests).

[hush (inactive), 2016-07-22 23:18:58]

Description was changed from

==========
Grant permission to the base url with loadDataWithBaseURL is called.

This is needed because font requests use the base url as the origin.

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

to

==========
Grant permission to the base url when loadDataWithBaseURL is called.

This is needed because font requests use the base url as the origin.

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

[hush (inactive), 2016-07-26 01:34:15]

Patchset #2 (id:20001) has been deleted

[hush (inactive), 2016-07-26 02:41:45]

Patchset #2 (id:40001) has been deleted

[hush (inactive), 2016-07-26 02:42:29]

PTAL

[hush (inactive), 2016-07-26 02:42:54]

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

https://codereview.chromium.org/2165783003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:2814: // as the origin for
font requests.
On 2016/07/22 18:31:49, Charlie Reis wrote:
> Let's update the comment, because CORS is not just about font requests (and
> because the previous file: comment still applies).  Something like:
> 
> When there's a base URL specified for the data URL, we also need to grant
access
> to the base URL.  This allows file: and other unexpected schemes to be
accepted
> at commit time and during CORS checks (e.g., for font requests).

Done.

[hush (inactive), 2016-07-26 02:43:38]

Description was changed from

==========
Grant permission to the base url when loadDataWithBaseURL is called.

This is needed because font requests use the base url as the origin.

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

to

==========
Grant permission to the base url when loadDataWithBaseURL is called.

When there's a base URL specified for the data URL, we also need to
grant access to the base URL. This allows file: and other unexpected
schemes to be accepted at commit time and during CORS checks (e.g., for
font requests).

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

[hush (inactive), 2016-07-26 02:44:24]

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

[commit-bot: I haz the power, 2016-07-26 02:44:37]

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-26 03:56:01]

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

[commit-bot: I haz the power, 2016-07-26 03:56:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Reis, 2016-07-26 17:57:13]

Thanks for the test!  LGTM with an observation below.

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

https://codereview.chromium.org/2165783003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:231:
ReloadBlockUntilNavigationsComplete(shell(), 1);
I'm a bit surprised this fails without the fix, but I confirmed it by patching
it in.

Normally I would expect reload to bring the renderer process back if it has
already been killed.  That means there would be a race between the IO thread
killing the renderer and the UI thread telling it to reload.  Without the fix,
the test might sometimes pass.

It looks like the test fails even when I set a breakpoint on the UI thread and
let the kill happen before the reload, which might be because the UI thread
doesn't hear about the kill until after it has told the old process to reload.

I'm not sure if there's a better thing to wait for, so given that this seems
very likely to catch regressions (if not 100%), I'm ok with it.

[hush (inactive), 2016-07-26 18:05:37]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2016-07-26 18:05:57]

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-26 18:56:43]

Description was changed from

==========
Grant permission to the base url when loadDataWithBaseURL is called.

When there's a base URL specified for the data URL, we also need to
grant access to the base URL. This allows file: and other unexpected
schemes to be accepted at commit time and during CORS checks (e.g., for
font requests).

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

to

==========
Grant permission to the base url when loadDataWithBaseURL is called.

When there's a base URL specified for the data URL, we also need to
grant access to the base URL. This allows file: and other unexpected
schemes to be accepted at commit time and during CORS checks (e.g., for
font requests).

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

[commit-bot: I haz the power, 2016-07-26 18:56:44]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-07-26 18:59:46]

Description was changed from

==========
Grant permission to the base url when loadDataWithBaseURL is called.

When there's a base URL specified for the data URL, we also need to
grant access to the base URL. This allows file: and other unexpected
schemes to be accepted at commit time and during CORS checks (e.g., for
font requests).

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

to

==========
Grant permission to the base url when loadDataWithBaseURL is called.

When there's a base URL specified for the data URL, we also need to
grant access to the base URL. This allows file: and other unexpected
schemes to be accepted at commit time and during CORS checks (e.g., for
font requests).

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

Committed: https://crrev.com/4bdb4f88f8197bebe32ce3480a3851201f648a8b
Cr-Commit-Position: refs/heads/master@{#407867}
==========

[commit-bot: I haz the power, 2016-07-26 18:59:47]

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