Block renderer-initiated main frame navigations to data URLs

Block renderer-initiated main frame navigations to data URLs

This CL implements the blocking of data URL navigations as described in the
blink intent to deprecate and remove thread at https://goo.gl/BaZAea.

The blocking is done in two separate places:

- blink::FrameLoader: This is a renderer side check that blocks all top-frame
  loads for known mime types. This check ignores unknown mime types as those
  can end up as downloads or be handled by plugins.

- content::DataURLNavigationThrottler: This is a browser side check that
  handles data URLs that were ignored by the renderer check. By this point, the
  determination of whether the URL is a download or not has already been made.
  This check allows downloads and blocks remaining URLs (ie. mime types that
  are handled by plugins). When browser side navigation is enabled, all blocking
  is done in this throttler instead of some checks being in blink::FrameLoader.

This CL moves data URL navigation tests to a separate file, and removes layout
tests that are no longer realistic.

TEST=data_url_navigation_browsertest.cc
BUG=594215
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2702503002
Cr-Commit-Position: refs/heads/master@{#466504}
Committed: https://chromium.googlesource.com/chromium/src/+/ba52f56207a4b9d70b34880fbff2352e71a06422

[meacer, 2017-02-16 01:56:07]

Description was changed from

==========
Block content initiated navigations to data URLs

BUG=594215
==========

to

==========
Block content initiated navigations to data URLs

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

[meacer, 2017-02-16 01:56:11]

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

[commit-bot: I haz the power, 2017-02-16 01:57:22]

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, 2017-02-16 02:55:26]

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

[commit-bot: I haz the power, 2017-02-16 02:55:27]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[meacer, 2017-02-16 21:45:19]

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

[commit-bot: I haz the power, 2017-02-16 21:46:10]

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

[meacer, 2017-02-16 22:13:42]

Description was changed from

==========
Block content initiated navigations to data URLs

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

to

==========
Block content initiated navigations to data URLs

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

[commit-bot: I haz the power, 2017-02-16 23:05:56]

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

[commit-bot: I haz the power, 2017-02-16 23:05:57]

Dry run: 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_...)

[meacer, 2017-02-18 00:13:35]

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

[commit-bot: I haz the power, 2017-02-18 00:15:05]

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, 2017-02-18 01:30:09]

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

[commit-bot: I haz the power, 2017-02-18 01:30:10]

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

[meacer, 2017-03-14 22:09:43]

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

[commit-bot: I haz the power, 2017-03-14 22:10:39]

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, 2017-03-14 22:58:59]

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

[commit-bot: I haz the power, 2017-03-14 22:59:00]

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

[meacer, 2017-03-16 19:32:32]

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

[commit-bot: I haz the power, 2017-03-16 19:33:17]

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, 2017-03-16 19:43:40]

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

[commit-bot: I haz the power, 2017-03-16 19:43:41]

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

[meacer, 2017-03-16 19:43:58]

Description was changed from

==========
Block content initiated navigations to data URLs

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

to

==========
Block content initiated, top-frame, non-download navigations to data URLs

This CL implements the blocking of data URL navigations described in the blink
intent to deprecate and remove thread at https://goo.gl/BaZAea.

The blocking is done in three separate places:
1. blink::FrameLoader: This is a renderer side check that blocks all top-frame
   loads for known mime types. This check ignores unknown mime types as those
   can end up as downloads or be handled by plugins.

2. content::AsyncResourceHandler: This is a browser side check that handles data
   URLs that were ignoerd by (1). By this point, the determination of whether
   the URL is a download or not has already been made. This check allows
   downloads and blocks remaining URLs (ie. mime types that are handled by
   plugins). This check is only done when browser side navigation is disabled.

3. content::DataURLNavigationThrottler: This check is only done when browser
   side navigation is enabled. Similar to (2), at this point we know whether a 
   navigation is a download or not.

This CL also moves the majority of data URL navigation tests to a separate file.

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

[meacer, 2017-03-16 19:46:28]

meacer@chromium.org changed reviewers:
+ asanka@chromium.org, creis@chromium.org

[meacer, 2017-03-16 19:46:29]

Charlie, Asanka: Can you please take a premilinary look? The CL isn't fully
ready yet, as AsyncResourceLoader has layering violations, but I wanted to get
your opinions before moving forward.

[meacer, 2017-03-16 19:53:22]

https://codereview.chromium.org/2702503002/diff/100001/content/browser/loader...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2702503002/diff/100001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:86: void
CheckNavigationIsRendererInitiated(
This is probably very wrong to do, so suggestions welcome :)

[meacer, 2017-03-16 21:38:39]

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

[commit-bot: I haz the power, 2017-03-16 21:39:35]

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

[meacer, 2017-03-16 21:49:40]

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

[commit-bot: I haz the power, 2017-03-16 21:49:59]

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

[commit-bot: I haz the power, 2017-03-16 21:50:20]

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, 2017-03-16 22:15:18]

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

[commit-bot: I haz the power, 2017-03-16 22:15:19]

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

[Charlie Reis, 2017-03-20 23:30:50]

On 2017/03/16 19:46:29, Mustafa Emre Acer wrote:
> Charlie, Asanka: Can you please take a premilinary look? The CL isn't fully
> ready yet, as AsyncResourceLoader has layering violations, but I wanted to get
> your opinions before moving forward.

Sorry for the delay-- it's been a hectic few days.  I'll try to make sure I look
tomorrow.

[meacer, 2017-03-21 01:30:25]

On 2017/03/20 23:30:50, Charlie Reis (slow) wrote:
> On 2017/03/16 19:46:29, Mustafa Emre Acer wrote:
> > Charlie, Asanka: Can you please take a premilinary look? The CL isn't fully
> > ready yet, as AsyncResourceLoader has layering violations, but I wanted to
get
> > your opinions before moving forward.
> 
> Sorry for the delay-- it's been a hectic few days.  I'll try to make sure I
look
> tomorrow.

Thanks! Note that this is pretty incomplete: I realized that we need to allow
navigations if the top frame is already a data URL, so that we don't break
links, redirects, window.open etc. on data URLs. I am currently trying to figure
out if it's possible to detect the opener's URL from
FrameLoader::prepareRequestForThisFrame, as we need this for window.open check.

[Charlie Reis, 2017-03-22 16:41:02]

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

[Charlie Reis, 2017-03-22 16:41:03]

I'm going to have to pass this off to nasko@, since I'm afraid I just don't have
time to look more closely before I disappear on vacation.  Also CC'ing clamy@,
who knows the throttle logic very well and can help.

At a high level, your comment in the design doc
(https://docs.google.com/document/d/1UnFi4Idz1wkpIopnBb2TKkYYIJXM5zrjQYq3CQE5A...)
made it sound like the the AsyncResourceHandler changes wouldn't be needed since
the throttle worked in non-PlzNavigate.  Is there a reason it's still in the CL?
 (The thread hop there isn't something we'd want to add, and that's probably not
the right class for the check anyway.)

On 2017/03/21 01:30:25, Mustafa Emre Acer wrote:
> On 2017/03/20 23:30:50, Charlie Reis (slow) wrote:
> > On 2017/03/16 19:46:29, Mustafa Emre Acer wrote:
> > > Charlie, Asanka: Can you please take a premilinary look? The CL isn't
fully
> > > ready yet, as AsyncResourceLoader has layering violations, but I wanted to
> get
> > > your opinions before moving forward.
> > 
> > Sorry for the delay-- it's been a hectic few days.  I'll try to make sure I
> look
> > tomorrow.
> 
> Thanks! Note that this is pretty incomplete: I realized that we need to allow
> navigations if the top frame is already a data URL, so that we don't break
> links, redirects, window.open etc. on data URLs. I am currently trying to
figure
> out if it's possible to detect the opener's URL from
> FrameLoader::prepareRequestForThisFrame, as we need this for window.open
check.

That's a good point, and one you should call out in the design doc.

In the throttle, you can check what the current frame's last committed URL is,
but that only handles navigations from one data URL to another in the same
frame.  If a data URL does a window.open to another data URL, you'd have to know
the originating URL.  dcheng@ is working on making that available but I don't
think it's ready yet.

Anyway, I'll move myself to CC and let Nasko take over.  Sorry for holding
things up!

[meacer, 2017-03-22 17:59:28]

On 2017/03/22 16:41:03, Charlie Reis (slow) wrote:
> I'm going to have to pass this off to nasko@, since I'm afraid I just don't
have
> time to look more closely before I disappear on vacation.  Also CC'ing clamy@,
> who knows the throttle logic very well and can help.
> 
> At a high level, your comment in the design doc
>
(https://docs.google.com/document/d/1UnFi4Idz1wkpIopnBb2TKkYYIJXM5zrjQYq3CQE5A...)
> made it sound like the the AsyncResourceHandler changes wouldn't be needed
since
> the throttle worked in non-PlzNavigate.  Is there a reason it's still in the
CL?
>  (The thread hop there isn't something we'd want to add, and that's probably
not
> the right class for the check anyway.)

I removed it in the last patchset, perhaps you were looking at an earlier one?
Ah, and looks like it's still in the description. I'll update it.

> 
> On 2017/03/21 01:30:25, Mustafa Emre Acer wrote:
> > On 2017/03/20 23:30:50, Charlie Reis (slow) wrote:
> > > On 2017/03/16 19:46:29, Mustafa Emre Acer wrote:
> > > > Charlie, Asanka: Can you please take a premilinary look? The CL isn't
> fully
> > > > ready yet, as AsyncResourceLoader has layering violations, but I wanted
to
> > get
> > > > your opinions before moving forward.
> > > 
> > > Sorry for the delay-- it's been a hectic few days.  I'll try to make sure
I
> > look
> > > tomorrow.
> > 
> > Thanks! Note that this is pretty incomplete: I realized that we need to
allow
> > navigations if the top frame is already a data URL, so that we don't break
> > links, redirects, window.open etc. on data URLs. I am currently trying to
> figure
> > out if it's possible to detect the opener's URL from
> > FrameLoader::prepareRequestForThisFrame, as we need this for window.open
> check.
> 
> That's a good point, and one you should call out in the design doc.
> 
> In the throttle, you can check what the current frame's last committed URL is,
> but that only handles navigations from one data URL to another in the same
> frame.  If a data URL does a window.open to another data URL, you'd have to
know
> the originating URL.  dcheng@ is working on making that available but I don't
> think it's ready yet.

I think this is http://crbug.com/651895. The throttler will only block mime
types handled by plugins, which I think currently only applies to PDF. So until
651895 is fixed, window.opening PDFs from data URLs will not work.

> 
> Anyway, I'll move myself to CC and let Nasko take over.  Sorry for holding
> things up!

No worries! It looks like I had a lot of things missing and working on them
anyways, so I wasn't blocked on the review.

[meacer, 2017-03-22 18:00:31]

Description was changed from

==========
Block content initiated, top-frame, non-download navigations to data URLs

This CL implements the blocking of data URL navigations described in the blink
intent to deprecate and remove thread at https://goo.gl/BaZAea.

The blocking is done in three separate places:
1. blink::FrameLoader: This is a renderer side check that blocks all top-frame
   loads for known mime types. This check ignores unknown mime types as those
   can end up as downloads or be handled by plugins.

2. content::AsyncResourceHandler: This is a browser side check that handles data
   URLs that were ignoerd by (1). By this point, the determination of whether
   the URL is a download or not has already been made. This check allows
   downloads and blocks remaining URLs (ie. mime types that are handled by
   plugins). This check is only done when browser side navigation is disabled.

3. content::DataURLNavigationThrottler: This check is only done when browser
   side navigation is enabled. Similar to (2), at this point we know whether a 
   navigation is a download or not.

This CL also moves the majority of data URL navigation tests to a separate file.

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

to

==========
Block content initiated, top-frame, non-download navigations to data URLs

This CL implements the blocking of data URL navigations as described in the
blink intent to deprecate and remove thread at https://goo.gl/BaZAea.

The blocking is done in two separate places:

- blink::FrameLoader: This is a renderer side check that blocks all top-frame
  loads for known mime types. This check ignores unknown mime types as those
  can end up as downloads or be handled by plugins.

- content::DataURLNavigationThrottler: This is a browser side check that
  handles data URLs that were ignored by the renderer check. By this point, the
  determination of whether the URL is a download or not has already been made.
  This check allows downloads and blocks remaining URLs (ie. mime types that
  are handled by plugins). 

This CL also moves the majority of data URL navigation tests to a separate file.

TEST=data_url_navigation_browsertest.cc
BUG=594215
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2017-03-28 19:58:01]

Thanks for putting this together! It looks very close and I have some minor
comments.

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_browsertest.cc (right):

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:110: // Test that
a window.open on ||original_url| to |data_url| URL is blocked
nit: Only one |.

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:129: void
TestRedirectBlocked(const GURL& original_url, const GURL& data_url) {
I don't see any redirects in this test method. How is a redirect performed here?

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:166:
download_observer.WaitForFinished();
Would this observer time out if no download occurred?

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_throttle.cc (right):

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_throttle.cc:36:
navigation_handle()->GetURL().spec().c_str()));
This code might also block navigations to data: URLs in <webview> tag for apps.
Is this something we intend on doing? I don't know if they are considered
renderer-initiated or browser-initiated, so an explicit test to confirm/deny
will be useful.

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_throttle.cc:46: return
base::WrapUnique(new DataUrlNavigationThrottle(navigation_handle));
Does it make sense to install this throttle on all requests? We could
technically only create it for navigations to data: scheme. The only thing it
will not block is redirects to data: URLs from the web, but I think this is
already done elsewhere. 

Also, we allow webRequest API to redirect to data: URLs, so I would expect the
code as written to block those. If we move it to only install the throttle for
data: scheme, that would be avoided.

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:928:
DataUrlNavigationThrottle::CreateThrottleForNavigation(this));
Do we want this to be run after mixed content and CSP checks? If there is a good
reason for being placed last, might be a good thing to add a comment why.

https://codereview.chromium.org/2702503002/diff/140001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2702503002/diff/140001/content/public/browser...
content/public/browser/navigation_handle.h:186: // Whether the navigation is a
download. This is useful when the navigation
nit: "navigation request", since navigations themselves aren't really a
download. The navigation request ends up resulting in a download.

[meacer, 2017-03-28 20:44:35]

Thanks Nasko! I'll address your comments, but I have a quick question about
webview.

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_throttle.cc (right):

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_throttle.cc:36:
navigation_handle()->GetURL().spec().c_str()));
On 2017/03/28 19:58:01, nasko wrote:
> This code might also block navigations to data: URLs in <webview> tag for
apps.
> Is this something we intend on doing? I don't know if they are considered
> renderer-initiated or browser-initiated, so an explicit test to confirm/deny
> will be useful.

I've been looking at this in the last few days. IMO we shouldn't break webview
as there is no omnibox there and the risk of spoofing is different than drive by
browsing ending up on data URLs.

I haven't gotten to checking webview in this throttle yet -- currently I'm
trying to fix the renderer side check at FrameLoader.cpp with little success.
The problem is that there doesn't seem to be a way to get the topmost frame from
the renderer with a webview. In other words, I'd like to be able to do something
like this in RenderFrameImpl:

bool RenderFrameImpl::allowUnrestrictedDataUrlNavigations() {
  WebFrame topmostFrame = ... // (embedder's frame which has a chrome-extension
URL)
  if (GetExtensionOrAppByURL(topmostFrame.url()) // or something similar
    return true;
  // .. other checks, if any.
  return false;
}

I'm not sure if this is feasible though. Is it possible to get the webview's
embedder on the renderer side?

[nasko, 2017-03-28 22:55:19]

On 2017/03/28 20:44:35, Mustafa Emre Acer wrote:
> Thanks Nasko! I'll address your comments, but I have a quick question about
> webview.
> 
>
https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
> File content/browser/frame_host/data_url_navigation_throttle.cc (right):
> 
>
https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
> content/browser/frame_host/data_url_navigation_throttle.cc:36:
> navigation_handle()->GetURL().spec().c_str()));
> On 2017/03/28 19:58:01, nasko wrote:
> > This code might also block navigations to data: URLs in <webview> tag for
> apps.
> > Is this something we intend on doing? I don't know if they are considered
> > renderer-initiated or browser-initiated, so an explicit test to confirm/deny
> > will be useful.
> 
> I've been looking at this in the last few days. IMO we shouldn't break webview
> as there is no omnibox there and the risk of spoofing is different than drive
by
> browsing ending up on data URLs.

Sounds good.
 
> I haven't gotten to checking webview in this throttle yet -- currently I'm
> trying to fix the renderer side check at FrameLoader.cpp with little success.

Just a strawman question: why do we need to check anything in the renderer
process? Wouldn't we be able to do all of the same checks browser side? If we
can, then there is value to having the logic all in one place.

> The problem is that there doesn't seem to be a way to get the topmost frame
from
> the renderer with a webview. 

Yeah, because they are different beasts : ). The app and the content running
inside the <webview> both consider themselves to be top-level frames and they
run in separate processes.

> In other words, I'd like to be able to do something
> like this in RenderFrameImpl:
> 
> bool RenderFrameImpl::allowUnrestrictedDataUrlNavigations() {
>   WebFrame topmostFrame = ... // (embedder's frame which has a
chrome-extension
> URL)
>   if (GetExtensionOrAppByURL(topmostFrame.url()) // or something similar
>     return true;
>   // .. other checks, if any.
>   return false;
> }
> 
> I'm not sure if this is feasible though. Is it possible to get the webview's
> embedder on the renderer side?

I'm going to suggest asking lfg@, but my speculation would be not at this time.
We have two versions of <webview>, one using BrowserPlugin and one using OOPIF
(which we are trying to ship). The latter might have a way of doing it, but it
isn't the shipping product yet, so I doubt you can use it.

I'll come back to the overall question: why can't we enforce all of this in the
browser process?

[meacer, 2017-03-29 00:15:01]

On 2017/03/28 22:55:19, nasko wrote:
> On 2017/03/28 20:44:35, Mustafa Emre Acer wrote:
> > Thanks Nasko! I'll address your comments, but I have a quick question about
> > webview.
> > 
> >
>
https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
> > File content/browser/frame_host/data_url_navigation_throttle.cc (right):
> > 
> >
>
https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
> > content/browser/frame_host/data_url_navigation_throttle.cc:36:
> > navigation_handle()->GetURL().spec().c_str()));
> > On 2017/03/28 19:58:01, nasko wrote:
> > > This code might also block navigations to data: URLs in <webview> tag for
> > apps.
> > > Is this something we intend on doing? I don't know if they are considered
> > > renderer-initiated or browser-initiated, so an explicit test to
confirm/deny
> > > will be useful.
> > 
> > I've been looking at this in the last few days. IMO we shouldn't break
webview
> > as there is no omnibox there and the risk of spoofing is different than
drive
> by
> > browsing ending up on data URLs.
> 
> Sounds good.

Actually reading the I2D thread again, rbyers@ suggests blocking data URLs in
webview as well, and his reasoning makes sense to me:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/GbVcuwg_QjM/ueW_HZcg...

So perhaps we should block in webview as well and see how much breakage we will
cause. That will resolve this issue altogether.

>  
> > I haven't gotten to checking webview in this throttle yet -- currently I'm
> > trying to fix the renderer side check at FrameLoader.cpp with little
success.
> 
> Just a strawman question: why do we need to check anything in the renderer
> process? Wouldn't we be able to do all of the same checks browser side? If we
> can, then there is value to having the logic all in one place.

Not all data URL loads go through the browser, there is a shortcut in blink
that parses and loads data URLs completely on the renderer side which is why we
can't do that:
https://cs.chromium.org/chromium/src/content/child/web_url_loader_impl.cc?rcl...

The fact that you asked this twice made me take another look, but without
PlzNavigate, I still don't see a way to do all the checks on the browser side
given the above shortcut, so I'm not sure if I'm missing something.

> 
> > The problem is that there doesn't seem to be a way to get the topmost frame
> from
> > the renderer with a webview. 
> 
> Yeah, because they are different beasts : ). The app and the content running
> inside the <webview> both consider themselves to be top-level frames and they
> run in separate processes.
> 
> > In other words, I'd like to be able to do something
> > like this in RenderFrameImpl:
> > 
> > bool RenderFrameImpl::allowUnrestrictedDataUrlNavigations() {
> >   WebFrame topmostFrame = ... // (embedder's frame which has a
> chrome-extension
> > URL)
> >   if (GetExtensionOrAppByURL(topmostFrame.url()) // or something similar
> >     return true;
> >   // .. other checks, if any.
> >   return false;
> > }
> > 
> > I'm not sure if this is feasible though. Is it possible to get the webview's
> > embedder on the renderer side?
> 
> I'm going to suggest asking lfg@, but my speculation would be not at this
time.
> We have two versions of <webview>, one using BrowserPlugin and one using OOPIF
> (which we are trying to ship). The latter might have a way of doing it, but it
> isn't the shipping product yet, so I doubt you can use it.
> 
> I'll come back to the overall question: why can't we enforce all of this in
the
> browser process?

[meacer, 2017-03-30 20:43:55]

nasko: I addressed your comments and fixed most of the tests. There are still a
few Android tests broken, I'll fix them in shortly. PTAL?

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_browsertest.cc (right):

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:110: // Test that
a window.open on ||original_url| to |data_url| URL is blocked
On 2017/03/28 19:58:01, nasko wrote:
> nit: Only one |.

Done.

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:129: void
TestRedirectBlocked(const GURL& original_url, const GURL& data_url) {
On 2017/03/28 19:58:00, nasko wrote:
> I don't see any redirects in this test method. How is a redirect performed
here?

As we discussed, my terminology was wrong here. I assumed window.location.href
is a redirect instead of navigation. Fixed.

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:166:
download_observer.WaitForFinished();
On 2017/03/28 19:58:01, nasko wrote:
> Would this observer time out if no download occurred?

Yes, added a comment.

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_throttle.cc (right):

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_throttle.cc:46: return
base::WrapUnique(new DataUrlNavigationThrottle(navigation_handle));
On 2017/03/28 19:58:01, nasko wrote:
> Does it make sense to install this throttle on all requests? We could
> technically only create it for navigations to data: scheme. The only thing it
> will not block is redirects to data: URLs from the web, but I think this is
> already done elsewhere. 
> 
> Also, we allow webRequest API to redirect to data: URLs, so I would expect the
> code as written to block those. If we move it to only install the throttle for
> data: scheme, that would be avoided.

Changed it to install only if the data URL is potentially going to be blocked. I
manually checked and webRequest redirects to data URLs don't get handled by the
throttler even if it's always created (with PlzNavigate on).

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2702503002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:928:
DataUrlNavigationThrottle::CreateThrottleForNavigation(this));
On 2017/03/28 19:58:01, nasko wrote:
> Do we want this to be run after mixed content and CSP checks? If there is a
good
> reason for being placed last, might be a good thing to add a comment why.

Didn't really think about that as I was just appending to the list. Moved it to
the top (I'm assuming AncestorThrottle is the CSP check).

https://codereview.chromium.org/2702503002/diff/140001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2702503002/diff/140001/content/public/browser...
content/public/browser/navigation_handle.h:186: // Whether the navigation is a
download. This is useful when the navigation
On 2017/03/28 19:58:01, nasko wrote:
> nit: "navigation request", since navigations themselves aren't really a
> download. The navigation request ends up resulting in a download. 

Done.

[nasko, 2017-03-30 23:15:45]

I need to read through the test changes, which were substantial, but let me send
some comments on the other files first.

https://codereview.chromium.org/2702503002/diff/160001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_throttle.cc (right):

https://codereview.chromium.org/2702503002/diff/160001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_throttle.cc:50: if
(MayBlockDataUrlNavigation(navigation_handle()) &&
Why do you need to call this method here? If it failed before, it wouldn't have
created the throttle object, right? Between creation time and this point, none
of the checks will change, except for redirects to data URLs, which will have to
be allowed to avoid breaking webRequest extension API.

https://codereview.chromium.org/2702503002/diff/160001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2702503002/diff/160001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:929: // Check for
top-frame, content-initiated navigations to data URLs. This is
nit: s/content-initiated/renderer-initiated/, s/top-frame/main frame/

https://codereview.chromium.org/2702503002/diff/160001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2702503002/diff/160001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:134: bool IsDownload()
override;
This will move down and become is_download(), since we should not expose it
through the public interface quite yet.

https://codereview.chromium.org/2702503002/diff/160001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2702503002/diff/160001/content/public/browser...
content/public/browser/navigation_handle.h:189: virtual bool IsDownload() = 0;
We don't need to expose this method at this time, as I didn't see any callers
outside of content/ for now.

[meacer, 2017-04-05 22:31:19]

Description was changed from

==========
Block content initiated, top-frame, non-download navigations to data URLs

This CL implements the blocking of data URL navigations as described in the
blink intent to deprecate and remove thread at https://goo.gl/BaZAea.

The blocking is done in two separate places:

- blink::FrameLoader: This is a renderer side check that blocks all top-frame
  loads for known mime types. This check ignores unknown mime types as those
  can end up as downloads or be handled by plugins.

- content::DataURLNavigationThrottler: This is a browser side check that
  handles data URLs that were ignored by the renderer check. By this point, the
  determination of whether the URL is a download or not has already been made.
  This check allows downloads and blocks remaining URLs (ie. mime types that
  are handled by plugins). 

This CL also moves the majority of data URL navigation tests to a separate file.

TEST=data_url_navigation_browsertest.cc
BUG=594215
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Block renderer-initiated main frame d navigations to data URLs

This CL implements the blocking of data URL navigations as described in the
blink intent to deprecate and remove thread at https://goo.gl/BaZAea.

The blocking is done in two separate places:

- blink::FrameLoader: This is a renderer side check that blocks all top-frame
  loads for known mime types. This check ignores unknown mime types as those
  can end up as downloads or be handled by plugins.

- content::DataURLNavigationThrottler: This is a browser side check that
  handles data URLs that were ignored by the renderer check. By this point, the
  determination of whether the URL is a download or not has already been made.
  This check allows downloads and blocks remaining URLs (ie. mime types that
  are handled by plugins). When browser side navigation is enabled, all blocking
  is done in this throttler instead of some checks being in blink::FrameLoader.

This CL moves data URL navigation tests to a separate file, and removes layout
tests that are no longer realistic.

TEST=data_url_navigation_browsertest.cc
BUG=594215
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[meacer, 2017-04-05 22:33:27]

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

[meacer, 2017-04-05 22:33:28]

Charlie: Adding you back since this time Nasko will be OOO :) 

Based on our discussion with dcheng and nasko last week, I re-blocked data URL
to data URL navigations as there doesn't seem to be a good way to get a trusted
"initiated by" frame information in the navigation handle. Also as discussed, I
added a temporary feature flag in case we need to kill the blocking.

PTAL?

https://codereview.chromium.org/2702503002/diff/160001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_throttle.cc (right):

https://codereview.chromium.org/2702503002/diff/160001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_throttle.cc:50: if
(MayBlockDataUrlNavigation(navigation_handle()) &&
On 2017/03/30 23:15:45, nasko (out until Apr 17th) wrote:
> Why do you need to call this method here? If it failed before, it wouldn't
have
> created the throttle object, right? Between creation time and this point, none
> of the checks will change, except for redirects to data URLs, which will have
to
> be allowed to avoid breaking webRequest extension API.

Done, only checking for download here.

https://codereview.chromium.org/2702503002/diff/160001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2702503002/diff/160001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:929: // Check for
top-frame, content-initiated navigations to data URLs. This is
On 2017/03/30 23:15:45, nasko (out until Apr 17th) wrote:
> nit: s/content-initiated/renderer-initiated/, s/top-frame/main frame/

Done.

https://codereview.chromium.org/2702503002/diff/160001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2702503002/diff/160001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:134: bool IsDownload()
override;
On 2017/03/30 23:15:45, nasko (out until Apr 17th) wrote:
> This will move down and become is_download(), since we should not expose it
> through the public interface quite yet.

By move down I assume you mean move away from the interface. If so, done.

https://codereview.chromium.org/2702503002/diff/160001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2702503002/diff/160001/content/public/browser...
content/public/browser/navigation_handle.h:189: virtual bool IsDownload() = 0;
On 2017/03/30 23:15:45, nasko (out until Apr 17th) wrote:
> We don't need to expose this method at this time, as I didn't see any callers
> outside of content/ for now.

Done.

[meacer, 2017-04-05 22:45:13]

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

[commit-bot: I haz the power, 2017-04-05 22:45:57]

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

[meacer, 2017-04-05 22:46:34]

Description was changed from

==========
Block renderer-initiated main frame d navigations to data URLs

This CL implements the blocking of data URL navigations as described in the
blink intent to deprecate and remove thread at https://goo.gl/BaZAea.

The blocking is done in two separate places:

- blink::FrameLoader: This is a renderer side check that blocks all top-frame
  loads for known mime types. This check ignores unknown mime types as those
  can end up as downloads or be handled by plugins.

- content::DataURLNavigationThrottler: This is a browser side check that
  handles data URLs that were ignored by the renderer check. By this point, the
  determination of whether the URL is a download or not has already been made.
  This check allows downloads and blocks remaining URLs (ie. mime types that
  are handled by plugins). When browser side navigation is enabled, all blocking
  is done in this throttler instead of some checks being in blink::FrameLoader.

This CL moves data URL navigation tests to a separate file, and removes layout
tests that are no longer realistic.

TEST=data_url_navigation_browsertest.cc
BUG=594215
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Block renderer-initiated main frame navigations to data URLs

This CL implements the blocking of data URL navigations as described in the
blink intent to deprecate and remove thread at https://goo.gl/BaZAea.

The blocking is done in two separate places:

- blink::FrameLoader: This is a renderer side check that blocks all top-frame
  loads for known mime types. This check ignores unknown mime types as those
  can end up as downloads or be handled by plugins.

- content::DataURLNavigationThrottler: This is a browser side check that
  handles data URLs that were ignored by the renderer check. By this point, the
  determination of whether the URL is a download or not has already been made.
  This check allows downloads and blocks remaining URLs (ie. mime types that
  are handled by plugins). When browser side navigation is enabled, all blocking
  is done in this throttler instead of some checks being in blink::FrameLoader.

This CL moves data URL navigation tests to a separate file, and removes layout
tests that are no longer realistic.

TEST=data_url_navigation_browsertest.cc
BUG=594215
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2017-04-05 23:55:19]

Did a full pass over content/. I love the thoroughness of the test cases,
thanks!

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_browsertest.cc (right):

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:92: //
fail_pattern, instead of waiting for a message matching success_pattern.
nit: I'd rather build that functionality into the other object, but let's not do
it in this CL and possibly do as a follow up.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:105:
message_loop_runner_(new MessageLoopRunner()),
You need to pass IMMEDIATE for QuitMode, otherwise the event loop will still
spin for a bit, which is contrary to what your description of the class states.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:144: };
Why does this enum differ from the one below in terms of one line vs 4 lines?

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:190:
std::tuple<TestNavigationType, TestSitePerProcessType>> {
I wonder if you need all this complexity in your test. We already run
content_browsertests with --site-per-process and PlzNavigate on Linux trybots
and on the waterfall. You will have to explicitly remove PlzNavigate once it
ships, so your call.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:240:
EXPECT_TRUE(iframe_loaded);
Is the goal of this code to wait until the load has completed? If yes, then I
don't think it does that. You can use TestNavigationObserver to observe a
navigation in a WebContents and wait for it to complete.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:312:
TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
nit: I think the default constructor uses 1 navigation, so you could skip the
second parameter.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:328: ->GetURL()
All that can be done with WebContents::GetLastCommittedURL : ).

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:416: // printed on
that new shell).
Why not? Once you get the shell from the observer, can't you attach your
delegate and wait for the message?

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:453:
new_shell->web_contents()->GetBrowserContext());
nit: The BrowserContext is the same across all windows, it is the equivalent of
a profile. It is possible to create this before you open the new window, in case
you need this earlier.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:461:
EXPECT_TRUE(new_shell->web_contents()->GetURL().spec().empty());
nit: GetLastCommittedURL, GetURL is deprecated.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:499:
base::ScopedTempDir downloads_directory_;
Why does this need to be protected, can't it be private like the data_url_?

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:529: // Tests that
a direct navigation to a data URL doesn't show a console warning
nit: s/direct/browser-initiated/

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:691:
////////////////////////////////////////////////////////////////////////////////
nit: Empty line above this one.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:833: // Test that
a navigation to a data URL is blocked if the data URL has a mime
nit: Did you mean s/navigation/form post/?

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_throttle.cc (right):

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_throttle.cc:36: if
(!handle->is_download()) {
Let's avoid a level of indentation and use:

if (handle->is_download())
  return PROCEED;

... the rest of the code. It also fails closed, which is always a nice way to do
security checks ;).

https://codereview.chromium.org/2702503002/diff/180001/content/public/common/...
File content/public/common/content_features.cc (right):

https://codereview.chromium.org/2702503002/diff/180001/content/public/common/...
content/public/common/content_features.cc:23: "AllowInsecureDataUrlNavigations",
base::FEATURE_DISABLED_BY_DEFAULT};
Technically, content-initiated is a bit better term than insecure, as they are
just as "secure" as the document that delivered them. However, they are being
abused, which might link them to security.

https://codereview.chromium.org/2702503002/diff/180001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2702503002/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4713: return url.string() ==
kUnreachableWebDataURL;
Given this implementation, naming this method
"allowContentInitiatedDataUrlNavigations" sounds a bit better to me. Error pages
are pretty much secure as they are local : ).

https://codereview.chromium.org/2702503002/diff/180001/content/test/data/data...
File content/test/data/data_url_navigations.html (right):

https://codereview.chromium.org/2702503002/diff/180001/content/test/data/data...
content/test/data/data_url_navigations.html:88:
onclick='top.location.href=`data:application/octet-stream,test`'>
Shouldn't this be a different mime type? We already test the octet-stream above
in the binary section of the file.

[commit-bot: I haz the power, 2017-04-06 00:55:16]

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

[commit-bot: I haz the power, 2017-04-06 00:55:17]

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_...)

[meacer, 2017-04-06 01:24:01]

Patchset #11 (id:200001) has been deleted

[meacer, 2017-04-06 01:25:42]

Thanks Nasko, I addressed your comments.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_browsertest.cc (right):

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:92: //
fail_pattern, instead of waiting for a message matching success_pattern.
On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> nit: I'd rather build that functionality into the other object, but let's not
do
> it in this CL and possibly do as a follow up. 

Filed https://crbug.com/708831

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:105:
message_loop_runner_(new MessageLoopRunner()),
On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> You need to pass IMMEDIATE for QuitMode, otherwise the event loop will still
> spin for a bit, which is contrary to what your description of the class
states.

Done. I suppose we want the same for the other ConsoleObserverDelegate?

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:144: };
On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> Why does this enum differ from the one below in terms of one line vs 4 lines?

Looks like the extra comma caused clang-format to write in 4 lines.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:190:
std::tuple<TestNavigationType, TestSitePerProcessType>> {
On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> I wonder if you need all this complexity in your test. We already run
> content_browsertests with --site-per-process and PlzNavigate on Linux trybots
> and on the waterfall. You will have to explicitly remove PlzNavigate once it
> ships, so your call.

I noticed that trybots did these, but I also found having the full set of
parameters to be helpful while running the tests locally. This was more
important when the CL allowed data URL to data URL navigations since there were
some edge cases not covered with some combination of the flags, but it's not
that useful anymore so removed.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:240:
EXPECT_TRUE(iframe_loaded);
On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> Is the goal of this code to wait until the load has completed? If yes, then I
> don't think it does that. You can use TestNavigationObserver to observe a
> navigation in a WebContents and wait for it to complete.

Done.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:312:
TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
On 2017/04/05 23:55:19, nasko (out until Apr 17th) wrote:
> nit: I think the default constructor uses 1 navigation, so you could skip the
> second parameter.

Done.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:328: ->GetURL()
On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> All that can be done with WebContents::GetLastCommittedURL : ).

Done.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:416: // printed on
that new shell).
On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> Why not? Once you get the shell from the observer, can't you attach your
> delegate and wait for the message?

Doesn't work unfortunately. The console message arrives after the shell is
created but before the console observer can wait for incoming messages.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:453:
new_shell->web_contents()->GetBrowserContext());
On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> nit: The BrowserContext is the same across all windows, it is the equivalent
of
> a profile. It is possible to create this before you open the new window, in
case
> you need this earlier.

Doesn't change the test much, but moved above.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:461:
EXPECT_TRUE(new_shell->web_contents()->GetURL().spec().empty());
On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> nit: GetLastCommittedURL, GetURL is deprecated.

Removed GetURL from here and elsewhere.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:499:
base::ScopedTempDir downloads_directory_;
On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> Why does this need to be protected, can't it be private like the data_url_?

Done.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:529: // Tests that
a direct navigation to a data URL doesn't show a console warning
On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> nit: s/direct/browser-initiated/

Done.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:691:
////////////////////////////////////////////////////////////////////////////////
On 2017/04/05 23:55:19, nasko (out until Apr 17th) wrote:
> nit: Empty line above this one.

Done.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:833: // Test that
a navigation to a data URL is blocked if the data URL has a mime
On 2017/04/05 23:55:19, nasko (out until Apr 17th) wrote:
> nit: Did you mean s/navigation/form post/?

Done.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_throttle.cc (right):

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_throttle.cc:36: if
(!handle->is_download()) {
On 2017/04/05 23:55:19, nasko (out until Apr 17th) wrote:
> Let's avoid a level of indentation and use:
> 
> if (handle->is_download())
>   return PROCEED;
> 
> ... the rest of the code. It also fails closed, which is always a nice way to
do
> security checks ;).

Sure, looks nicer :)

https://codereview.chromium.org/2702503002/diff/180001/content/public/common/...
File content/public/common/content_features.cc (right):

https://codereview.chromium.org/2702503002/diff/180001/content/public/common/...
content/public/common/content_features.cc:23: "AllowInsecureDataUrlNavigations",
base::FEATURE_DISABLED_BY_DEFAULT};
On 2017/04/05 23:55:19, nasko (out until Apr 17th) wrote:
> Technically, content-initiated is a bit better term than insecure, as they are
> just as "secure" as the document that delivered them. However, they are being
> abused, which might link them to security.

I wanted to name the flag insecure to discourage people from using it, but I
don't have a strong opinion. 
 AllowContentInititatedDataUrlNavigations also sounds good.

https://codereview.chromium.org/2702503002/diff/180001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2702503002/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4713: return url.string() ==
kUnreachableWebDataURL;
On 2017/04/05 23:55:19, nasko (out until Apr 17th) wrote:
> Given this implementation, naming this method
> "allowContentInitiatedDataUrlNavigations" sounds a bit better to me. Error
pages
> are pretty much secure as they are local : ).

Done.

https://codereview.chromium.org/2702503002/diff/180001/content/test/data/data...
File content/test/data/data_url_navigations.html (right):

https://codereview.chromium.org/2702503002/diff/180001/content/test/data/data...
content/test/data/data_url_navigations.html:88:
onclick='top.location.href=`data:application/octet-stream,test`'>
On 2017/04/05 23:55:19, nasko (out until Apr 17th) wrote:
> Shouldn't this be a different mime type? We already test the octet-stream
above
> in the binary section of the file.

Done.

[nasko, 2017-04-06 16:57:26]

content/ LGTM with a couple of comments.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_browsertest.cc (right):

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:105:
message_loop_runner_(new MessageLoopRunner()),
On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > You need to pass IMMEDIATE for QuitMode, otherwise the event loop will still
> > spin for a bit, which is contrary to what your description of the class
> states.
> 
> Done. I suppose we want the same for the other ConsoleObserverDelegate?

Probably, but I'd suggest doing it separately.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:144: };
On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > Why does this enum differ from the one below in terms of one line vs 4
lines?
> 
> Looks like the extra comma caused clang-format to write in 4 lines.

Can we remove that and make it match the one below? It looks strange to have it
differently formatted.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:416: // printed on
that new shell).
On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > Why not? Once you get the shell from the observer, can't you attach your
> > delegate and wait for the message?
> 
> Doesn't work unfortunately. The console message arrives after the shell is
> created but before the console observer can wait for incoming messages.

Hmm, maybe we can follow up here once I'm back, but it doesn't seem like that
should be the case.

[meacer, 2017-04-06 18:14:17]

meacer@chromium.org changed reviewers:
+ avi@chromium.org, dglazkov@chromium.org, fsamuel@chromium.org,
tedchoc@chromium.org

[meacer, 2017-04-06 18:14:19]

Owners, can you PTAL?

tedchoc:
chrome/android/
chrome/test/android/
chrome/test/data/android/

fsamuel:
extensions/browser/guest_view
chrome/browser/apps/guest_view
extensions/test/data/web_view

avi:
chrome/browser/tab_contents/

dglazkov:
third_party/WebKit

Thanks!

https://codereview.chromium.org/2702503002/diff/180001/chrome/test/data/exten...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/2702503002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1333: function
testExecuteScriptIsAbortedWhenWebViewSourceIsChanged() {
This test is flaky and will be disabled in https://crrev.com/2803813002

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_browsertest.cc (right):

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:105:
message_loop_runner_(new MessageLoopRunner()),
On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > You need to pass IMMEDIATE for QuitMode, otherwise the event loop will
still
> > > spin for a bit, which is contrary to what your description of the class
> > states.
> > 
> > Done. I suppose we want the same for the other ConsoleObserverDelegate?
> 
> Probably, but I'd suggest doing it separately.

Will do it as part of https://crbug.com/708831

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:144: };
On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > Why does this enum differ from the one below in terms of one line vs 4
> lines?
> > 
> > Looks like the extra comma caused clang-format to write in 4 lines.
> 
> Can we remove that and make it match the one below? It looks strange to have
it
> differently formatted.

I removed the test parameters from the last patchset. It now relies on the
trybots to test with site isolation and plznavigate.

https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_browsertest.cc:416: // printed on
that new shell).
On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > Why not? Once you get the shell from the observer, can't you attach your
> > > delegate and wait for the message?
> > 
> > Doesn't work unfortunately. The console message arrives after the shell is
> > created but before the console observer can wait for incoming messages.
> 
> Hmm, maybe we can follow up here once I'm back, but it doesn't seem like that
> should be the case.

An alternative is to print the logging message to the opener's console. We can
then wait on the message, but it'll be inconsistent with console messages for
other blocking such as chrome URLs.

https://codereview.chromium.org/2702503002/diff/220001/chrome/test/data/exten...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/2702503002/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1333: function
testExecuteScriptIsAbortedWhenWebViewSourceIsChanged() {
This test is flaky and will be disabled in https://crrev.com/2803813002

[Avi (use Gerrit), 2017-04-06 18:20:09]

On 2017/04/06 18:14:19, Mustafa Emre Acer wrote:
> Owners, can you PTAL?
> 
> tedchoc:
> chrome/android/
> chrome/test/android/
> chrome/test/data/android/
> 
> fsamuel:
> extensions/browser/guest_view
> chrome/browser/apps/guest_view
> extensions/test/data/web_view
> 
> avi:
> chrome/browser/tab_contents/
> 
> dglazkov:
> third_party/WebKit
> 
> Thanks!
> 
>
https://codereview.chromium.org/2702503002/diff/180001/chrome/test/data/exten...
> File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):
> 
>
https://codereview.chromium.org/2702503002/diff/180001/chrome/test/data/exten...
> chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1333: function
> testExecuteScriptIsAbortedWhenWebViewSourceIsChanged() {
> This test is flaky and will be disabled in https://crrev.com/2803813002
> 
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> File content/browser/frame_host/data_url_navigation_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> content/browser/frame_host/data_url_navigation_browsertest.cc:105:
> message_loop_runner_(new MessageLoopRunner()),
> On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > You need to pass IMMEDIATE for QuitMode, otherwise the event loop will
> still
> > > > spin for a bit, which is contrary to what your description of the class
> > > states.
> > > 
> > > Done. I suppose we want the same for the other ConsoleObserverDelegate?
> > 
> > Probably, but I'd suggest doing it separately.
> 
> Will do it as part of https://crbug.com/708831
> 
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> content/browser/frame_host/data_url_navigation_browsertest.cc:144: };
> On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > Why does this enum differ from the one below in terms of one line vs 4
> > lines?
> > > 
> > > Looks like the extra comma caused clang-format to write in 4 lines.
> > 
> > Can we remove that and make it match the one below? It looks strange to have
> it
> > differently formatted.
> 
> I removed the test parameters from the last patchset. It now relies on the
> trybots to test with site isolation and plznavigate.
> 
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> content/browser/frame_host/data_url_navigation_browsertest.cc:416: // printed
on
> that new shell).
> On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > Why not? Once you get the shell from the observer, can't you attach your
> > > > delegate and wait for the message?
> > > 
> > > Doesn't work unfortunately. The console message arrives after the shell is
> > > created but before the console observer can wait for incoming messages.
> > 
> > Hmm, maybe we can follow up here once I'm back, but it doesn't seem like
that
> > should be the case.
> 
> An alternative is to print the logging message to the opener's console. We can
> then wait on the message, but it'll be inconsistent with console messages for
> other blocking such as chrome URLs.
> 
>
https://codereview.chromium.org/2702503002/diff/220001/chrome/test/data/exten...
> File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):
> 
>
https://codereview.chromium.org/2702503002/diff/220001/chrome/test/data/exten...
> chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1333: function
> testExecuteScriptIsAbortedWhenWebViewSourceIsChanged() {
> This test is flaky and will be disabled in https://crrev.com/2803813002

chrome/browser/tab_contents/ LGTM

[Ted C, 2017-04-06 18:53:21]

On 2017/04/06 18:20:09, Avi (slow and ooo 10-23 April) wrote:
> On 2017/04/06 18:14:19, Mustafa Emre Acer wrote:
> > Owners, can you PTAL?
> > 
> > tedchoc:
> > chrome/android/
> > chrome/test/android/
> > chrome/test/data/android/
> > 
> > fsamuel:
> > extensions/browser/guest_view
> > chrome/browser/apps/guest_view
> > extensions/test/data/web_view
> > 
> > avi:
> > chrome/browser/tab_contents/
> > 
> > dglazkov:
> > third_party/WebKit
> > 
> > Thanks!
> > 
> >
>
https://codereview.chromium.org/2702503002/diff/180001/chrome/test/data/exten...
> > File chrome/test/data/extensions/platform_apps/web_view/shim/main.js
(right):
> > 
> >
>
https://codereview.chromium.org/2702503002/diff/180001/chrome/test/data/exten...
> > chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1333:
function
> > testExecuteScriptIsAbortedWhenWebViewSourceIsChanged() {
> > This test is flaky and will be disabled in https://crrev.com/2803813002
> > 
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > File content/browser/frame_host/data_url_navigation_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > content/browser/frame_host/data_url_navigation_browsertest.cc:105:
> > message_loop_runner_(new MessageLoopRunner()),
> > On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > > You need to pass IMMEDIATE for QuitMode, otherwise the event loop will
> > still
> > > > > spin for a bit, which is contrary to what your description of the
class
> > > > states.
> > > > 
> > > > Done. I suppose we want the same for the other ConsoleObserverDelegate?
> > > 
> > > Probably, but I'd suggest doing it separately.
> > 
> > Will do it as part of https://crbug.com/708831
> > 
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > content/browser/frame_host/data_url_navigation_browsertest.cc:144: };
> > On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > > Why does this enum differ from the one below in terms of one line vs 4
> > > lines?
> > > > 
> > > > Looks like the extra comma caused clang-format to write in 4 lines.
> > > 
> > > Can we remove that and make it match the one below? It looks strange to
have
> > it
> > > differently formatted.
> > 
> > I removed the test parameters from the last patchset. It now relies on the
> > trybots to test with site isolation and plznavigate.
> > 
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > content/browser/frame_host/data_url_navigation_browsertest.cc:416: //
printed
> on
> > that new shell).
> > On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > > Why not? Once you get the shell from the observer, can't you attach
your
> > > > > delegate and wait for the message?
> > > > 
> > > > Doesn't work unfortunately. The console message arrives after the shell
is
> > > > created but before the console observer can wait for incoming messages.
> > > 
> > > Hmm, maybe we can follow up here once I'm back, but it doesn't seem like
> that
> > > should be the case.
> > 
> > An alternative is to print the logging message to the opener's console. We
can
> > then wait on the message, but it'll be inconsistent with console messages
for
> > other blocking such as chrome URLs.
> > 
> >
>
https://codereview.chromium.org/2702503002/diff/220001/chrome/test/data/exten...
> > File chrome/test/data/extensions/platform_apps/web_view/shim/main.js
(right):
> > 
> >
>
https://codereview.chromium.org/2702503002/diff/220001/chrome/test/data/exten...
> > chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1333:
function
> > testExecuteScriptIsAbortedWhenWebViewSourceIsChanged() {
> > This test is flaky and will be disabled in https://crrev.com/2803813002
> 
> chrome/browser/tab_contents/ LGTM

android lgtm

I'm assuming this does not break open in new tab for data URIs as discussed
in the blink thread right?

https://groups.google.com/a/chromium.org/d/msg/blink-dev/GbVcuwg_QjM/3XKNPVhR...

When Elliot says that image search used to use data URIs, that is actually
incorrect
as they still use them on mobile.  If you do an image search, click on an image,
click on it again, you should be in a black letter boxed image viewer.  At this
point,
you can right click open in new tab and it is a data URI.  Breaking that has the
potential of breaking top level google properties that we should be very
cautious
of doing without reaching out to them first.

Again, my approval is based on the assumption that is the case from the thread.

[meacer, 2017-04-06 19:23:40]

On 2017/04/06 18:53:21, Ted C wrote:
> On 2017/04/06 18:20:09, Avi (slow and ooo 10-23 April) wrote:
> > On 2017/04/06 18:14:19, Mustafa Emre Acer wrote:
> > > Owners, can you PTAL?
> > > 
> > > tedchoc:
> > > chrome/android/
> > > chrome/test/android/
> > > chrome/test/data/android/
> > > 
> > > fsamuel:
> > > extensions/browser/guest_view
> > > chrome/browser/apps/guest_view
> > > extensions/test/data/web_view
> > > 
> > > avi:
> > > chrome/browser/tab_contents/
> > > 
> > > dglazkov:
> > > third_party/WebKit
> > > 
> > > Thanks!
> > > 
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/chrome/test/data/exten...
> > > File chrome/test/data/extensions/platform_apps/web_view/shim/main.js
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/chrome/test/data/exten...
> > > chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1333:
> function
> > > testExecuteScriptIsAbortedWhenWebViewSourceIsChanged() {
> > > This test is flaky and will be disabled in https://crrev.com/2803813002
> > > 
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > > File content/browser/frame_host/data_url_navigation_browsertest.cc
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > > content/browser/frame_host/data_url_navigation_browsertest.cc:105:
> > > message_loop_runner_(new MessageLoopRunner()),
> > > On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > > > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > > > You need to pass IMMEDIATE for QuitMode, otherwise the event loop
will
> > > still
> > > > > > spin for a bit, which is contrary to what your description of the
> class
> > > > > states.
> > > > > 
> > > > > Done. I suppose we want the same for the other
ConsoleObserverDelegate?
> > > > 
> > > > Probably, but I'd suggest doing it separately.
> > > 
> > > Will do it as part of https://crbug.com/708831
> > > 
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > > content/browser/frame_host/data_url_navigation_browsertest.cc:144: };
> > > On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > > > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > > > Why does this enum differ from the one below in terms of one line vs
4
> > > > lines?
> > > > > 
> > > > > Looks like the extra comma caused clang-format to write in 4 lines.
> > > > 
> > > > Can we remove that and make it match the one below? It looks strange to
> have
> > > it
> > > > differently formatted.
> > > 
> > > I removed the test parameters from the last patchset. It now relies on the
> > > trybots to test with site isolation and plznavigate.
> > > 
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > > content/browser/frame_host/data_url_navigation_browsertest.cc:416: //
> printed
> > on
> > > that new shell).
> > > On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > > > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > > > Why not? Once you get the shell from the observer, can't you attach
> your
> > > > > > delegate and wait for the message?
> > > > > 
> > > > > Doesn't work unfortunately. The console message arrives after the
shell
> is
> > > > > created but before the console observer can wait for incoming
messages.
> > > > 
> > > > Hmm, maybe we can follow up here once I'm back, but it doesn't seem like
> > that
> > > > should be the case.
> > > 
> > > An alternative is to print the logging message to the opener's console. We
> can
> > > then wait on the message, but it'll be inconsistent with console messages
> for
> > > other blocking such as chrome URLs.
> > > 
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/220001/chrome/test/data/exten...
> > > File chrome/test/data/extensions/platform_apps/web_view/shim/main.js
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/220001/chrome/test/data/exten...
> > > chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1333:
> function
> > > testExecuteScriptIsAbortedWhenWebViewSourceIsChanged() {
> > > This test is flaky and will be disabled in https://crrev.com/2803813002
> > 
> > chrome/browser/tab_contents/ LGTM
> 
> android lgtm
> 
> I'm assuming this does not break open in new tab for data URIs as discussed
> in the blink thread right?
> 
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/GbVcuwg_QjM/3XKNPVhR...
> 
> When Elliot says that image search used to use data URIs, that is actually
> incorrect
> as they still use them on mobile.  If you do an image search, click on an
image,
> click on it again, you should be in a black letter boxed image viewer.  At
this
> point,
> you can right click open in new tab and it is a data URI.  Breaking that has
the
> potential of breaking top level google properties that we should be very
> cautious
> of doing without reaching out to them first.
> 
> Again, my approval is based on the assumption that is the case from the
thread.

Correct, this doesn't block right click and open in new tab/window. However, I
can't get that behavior with image search on mobile: The image opened in the new
tab is an HTTP URL for me. My steps are:
1. Search for test in Google image search
2. Tap on one of the results
3. Tap on the image so that it's put inside a black box
4. Long tap to open "Image result for test" menu
5. Tap "Open image in new tab" -> Opens a non-data image URL in a new tab.

Perhaps I'm missing a step?

[Ted C, 2017-04-06 20:10:58]

On 2017/04/06 19:23:40, Mustafa Emre Acer wrote:
> On 2017/04/06 18:53:21, Ted C wrote:
> > On 2017/04/06 18:20:09, Avi (slow and ooo 10-23 April) wrote:
> > > On 2017/04/06 18:14:19, Mustafa Emre Acer wrote:
> > > > Owners, can you PTAL?
> > > > 
> > > > tedchoc:
> > > > chrome/android/
> > > > chrome/test/android/
> > > > chrome/test/data/android/
> > > > 
> > > > fsamuel:
> > > > extensions/browser/guest_view
> > > > chrome/browser/apps/guest_view
> > > > extensions/test/data/web_view
> > > > 
> > > > avi:
> > > > chrome/browser/tab_contents/
> > > > 
> > > > dglazkov:
> > > > third_party/WebKit
> > > > 
> > > > Thanks!
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/chrome/test/data/exten...
> > > > File chrome/test/data/extensions/platform_apps/web_view/shim/main.js
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/chrome/test/data/exten...
> > > > chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1333:
> > function
> > > > testExecuteScriptIsAbortedWhenWebViewSourceIsChanged() {
> > > > This test is flaky and will be disabled in https://crrev.com/2803813002
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > > > File content/browser/frame_host/data_url_navigation_browsertest.cc
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > > > content/browser/frame_host/data_url_navigation_browsertest.cc:105:
> > > > message_loop_runner_(new MessageLoopRunner()),
> > > > On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > > > > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > > > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > > > > You need to pass IMMEDIATE for QuitMode, otherwise the event loop
> will
> > > > still
> > > > > > > spin for a bit, which is contrary to what your description of the
> > class
> > > > > > states.
> > > > > > 
> > > > > > Done. I suppose we want the same for the other
> ConsoleObserverDelegate?
> > > > > 
> > > > > Probably, but I'd suggest doing it separately.
> > > > 
> > > > Will do it as part of https://crbug.com/708831
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > > > content/browser/frame_host/data_url_navigation_browsertest.cc:144: };
> > > > On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > > > > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > > > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > > > > Why does this enum differ from the one below in terms of one line
vs
> 4
> > > > > lines?
> > > > > > 
> > > > > > Looks like the extra comma caused clang-format to write in 4 lines.
> > > > > 
> > > > > Can we remove that and make it match the one below? It looks strange
to
> > have
> > > > it
> > > > > differently formatted.
> > > > 
> > > > I removed the test parameters from the last patchset. It now relies on
the
> > > > trybots to test with site isolation and plznavigate.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > > > content/browser/frame_host/data_url_navigation_browsertest.cc:416: //
> > printed
> > > on
> > > > that new shell).
> > > > On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > > > > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > > > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > > > > Why not? Once you get the shell from the observer, can't you
attach
> > your
> > > > > > > delegate and wait for the message?
> > > > > > 
> > > > > > Doesn't work unfortunately. The console message arrives after the
> shell
> > is
> > > > > > created but before the console observer can wait for incoming
> messages.
> > > > > 
> > > > > Hmm, maybe we can follow up here once I'm back, but it doesn't seem
like
> > > that
> > > > > should be the case.
> > > > 
> > > > An alternative is to print the logging message to the opener's console.
We
> > can
> > > > then wait on the message, but it'll be inconsistent with console
messages
> > for
> > > > other blocking such as chrome URLs.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/220001/chrome/test/data/exten...
> > > > File chrome/test/data/extensions/platform_apps/web_view/shim/main.js
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/220001/chrome/test/data/exten...
> > > > chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1333:
> > function
> > > > testExecuteScriptIsAbortedWhenWebViewSourceIsChanged() {
> > > > This test is flaky and will be disabled in https://crrev.com/2803813002
> > > 
> > > chrome/browser/tab_contents/ LGTM
> > 
> > android lgtm
> > 
> > I'm assuming this does not break open in new tab for data URIs as discussed
> > in the blink thread right?
> > 
> >
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/GbVcuwg_QjM/3XKNPVhR...
> > 
> > When Elliot says that image search used to use data URIs, that is actually
> > incorrect
> > as they still use them on mobile.  If you do an image search, click on an
> image,
> > click on it again, you should be in a black letter boxed image viewer.  At
> this
> > point,
> > you can right click open in new tab and it is a data URI.  Breaking that has
> the
> > potential of breaking top level google properties that we should be very
> > cautious
> > of doing without reaching out to them first.
> > 
> > Again, my approval is based on the assumption that is the case from the
> thread.
> 
> Correct, this doesn't block right click and open in new tab/window. However, I
> can't get that behavior with image search on mobile: The image opened in the
new
> tab is an HTTP URL for me. My steps are:
> 1. Search for test in Google image search
> 2. Tap on one of the results
> 3. Tap on the image so that it's put inside a black box
> 4. Long tap to open "Image result for test" menu
> 5. Tap "Open image in new tab" -> Opens a non-data image URL in a new tab.
> 
> Perhaps I'm missing a step?

Ok...google is obviously mocking me because I don't see it anymore either.

I was doing the exact same steps earlier in the day (albeit with my
default search "cows") and getting data URIs.  No longer.  I'm seeing
proper image urls now.  I'll hold onto the belief that they experiment
with this or use data URIs in certain circumstances over the idea that
I'm going crazy.

Sorry for the noise and proceed :-P

[meacer, 2017-04-06 21:20:32]

On 2017/04/06 20:10:58, Ted C wrote:
> On 2017/04/06 19:23:40, Mustafa Emre Acer wrote:
> > On 2017/04/06 18:53:21, Ted C wrote:
> > > On 2017/04/06 18:20:09, Avi (slow and ooo 10-23 April) wrote:
> > > > On 2017/04/06 18:14:19, Mustafa Emre Acer wrote:
> > > > > Owners, can you PTAL?
> > > > > 
> > > > > tedchoc:
> > > > > chrome/android/
> > > > > chrome/test/android/
> > > > > chrome/test/data/android/
> > > > > 
> > > > > fsamuel:
> > > > > extensions/browser/guest_view
> > > > > chrome/browser/apps/guest_view
> > > > > extensions/test/data/web_view
> > > > > 
> > > > > avi:
> > > > > chrome/browser/tab_contents/
> > > > > 
> > > > > dglazkov:
> > > > > third_party/WebKit
> > > > > 
> > > > > Thanks!
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/chrome/test/data/exten...
> > > > > File chrome/test/data/extensions/platform_apps/web_view/shim/main.js
> > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/chrome/test/data/exten...
> > > > > chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1333:
> > > function
> > > > > testExecuteScriptIsAbortedWhenWebViewSourceIsChanged() {
> > > > > This test is flaky and will be disabled in
https://crrev.com/2803813002
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > > > > File content/browser/frame_host/data_url_navigation_browsertest.cc
> > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > > > > content/browser/frame_host/data_url_navigation_browsertest.cc:105:
> > > > > message_loop_runner_(new MessageLoopRunner()),
> > > > > On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > > > > > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > > > > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > > > > > You need to pass IMMEDIATE for QuitMode, otherwise the event
loop
> > will
> > > > > still
> > > > > > > > spin for a bit, which is contrary to what your description of
the
> > > class
> > > > > > > states.
> > > > > > > 
> > > > > > > Done. I suppose we want the same for the other
> > ConsoleObserverDelegate?
> > > > > > 
> > > > > > Probably, but I'd suggest doing it separately.
> > > > > 
> > > > > Will do it as part of https://crbug.com/708831
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > > > > content/browser/frame_host/data_url_navigation_browsertest.cc:144: };
> > > > > On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > > > > > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > > > > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > > > > > Why does this enum differ from the one below in terms of one
line
> vs
> > 4
> > > > > > lines?
> > > > > > > 
> > > > > > > Looks like the extra comma caused clang-format to write in 4
lines.
> > > > > > 
> > > > > > Can we remove that and make it match the one below? It looks strange
> to
> > > have
> > > > > it
> > > > > > differently formatted.
> > > > > 
> > > > > I removed the test parameters from the last patchset. It now relies on
> the
> > > > > trybots to test with site isolation and plznavigate.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/180001/content/browser/frame_...
> > > > > content/browser/frame_host/data_url_navigation_browsertest.cc:416: //
> > > printed
> > > > on
> > > > > that new shell).
> > > > > On 2017/04/06 16:57:26, nasko (out until Apr 17th) wrote:
> > > > > > On 2017/04/06 01:25:42, Mustafa Emre Acer wrote:
> > > > > > > On 2017/04/05 23:55:18, nasko (out until Apr 17th) wrote:
> > > > > > > > Why not? Once you get the shell from the observer, can't you
> attach
> > > your
> > > > > > > > delegate and wait for the message?
> > > > > > > 
> > > > > > > Doesn't work unfortunately. The console message arrives after the
> > shell
> > > is
> > > > > > > created but before the console observer can wait for incoming
> > messages.
> > > > > > 
> > > > > > Hmm, maybe we can follow up here once I'm back, but it doesn't seem
> like
> > > > that
> > > > > > should be the case.
> > > > > 
> > > > > An alternative is to print the logging message to the opener's
console.
> We
> > > can
> > > > > then wait on the message, but it'll be inconsistent with console
> messages
> > > for
> > > > > other blocking such as chrome URLs.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/220001/chrome/test/data/exten...
> > > > > File chrome/test/data/extensions/platform_apps/web_view/shim/main.js
> > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2702503002/diff/220001/chrome/test/data/exten...
> > > > > chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1333:
> > > function
> > > > > testExecuteScriptIsAbortedWhenWebViewSourceIsChanged() {
> > > > > This test is flaky and will be disabled in
https://crrev.com/2803813002
> > > > 
> > > > chrome/browser/tab_contents/ LGTM
> > > 
> > > android lgtm
> > > 
> > > I'm assuming this does not break open in new tab for data URIs as
discussed
> > > in the blink thread right?
> > > 
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/GbVcuwg_QjM/3XKNPVhR...
> > > 
> > > When Elliot says that image search used to use data URIs, that is actually
> > > incorrect
> > > as they still use them on mobile.  If you do an image search, click on an
> > image,
> > > click on it again, you should be in a black letter boxed image viewer.  At
> > this
> > > point,
> > > you can right click open in new tab and it is a data URI.  Breaking that
has
> > the
> > > potential of breaking top level google properties that we should be very
> > > cautious
> > > of doing without reaching out to them first.
> > > 
> > > Again, my approval is based on the assumption that is the case from the
> > thread.
> > 
> > Correct, this doesn't block right click and open in new tab/window. However,
I
> > can't get that behavior with image search on mobile: The image opened in the
> new
> > tab is an HTTP URL for me. My steps are:
> > 1. Search for test in Google image search
> > 2. Tap on one of the results
> > 3. Tap on the image so that it's put inside a black box
> > 4. Long tap to open "Image result for test" menu
> > 5. Tap "Open image in new tab" -> Opens a non-data image URL in a new tab.
> > 
> > Perhaps I'm missing a step?
> 
> Ok...google is obviously mocking me because I don't see it anymore either.
> 
> I was doing the exact same steps earlier in the day (albeit with my
> default search "cows") and getting data URIs.  No longer.  I'm seeing
> proper image urls now.  I'll hold onto the belief that they experiment
> with this or use data URIs in certain circumstances over the idea that
> I'm going crazy.
> 
> Sorry for the noise and proceed :-P

Sure, no problem :) Pure speculation: They might have been experimenting but
stopped it now that we mark data URLs as "not secure".

[kinuko, 2017-04-07 07:52:32]

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

[kinuko, 2017-04-07 07:52:34]

Drive-by

https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right):

https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkUtils.cpp:113: if
(net::DataURL::Parse(WebStringToGURL(url.getString()), &utf8MimeType,
WebURL(url) should work (and more efficient)

https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:348: if
(url.protocol() == "data") {
nit: protocolIsData()

https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:679: }
This should not be in 'WebGL' category, please move or split the section.

Is this only to check error page URL? It feels a bit sad to have all this
plumbing just for that.

[meacer, 2017-04-11 01:08:51]

https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right):

https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkUtils.cpp:113: if
(net::DataURL::Parse(WebStringToGURL(url.getString()), &utf8MimeType,
On 2017/04/07 07:52:34, kinuko wrote:
> WebURL(url) should work (and more efficient)

I'm getting 

error: no viable conversion from 'blink::WebURL' to 'const GURL'

Did you mean something else?

https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:348: if
(url.protocol() == "data") {
On 2017/04/07 07:52:34, kinuko wrote:
> nit: protocolIsData()

Done.

https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:679: }
On 2017/04/07 07:52:34, kinuko wrote:
> This should not be in 'WebGL' category, please move or split the section.

Done.

> 
> Is this only to check error page URL? It feels a bit sad to have all this
> plumbing just for that.

Yes, unfortunately it's only for the error page.

[meacer, 2017-04-11 18:02:48]

On 2017/04/11 01:08:51, Mustafa Emre Acer wrote:
>
https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right):
> 
>
https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/network/NetworkUtils.cpp:113: if
> (net::DataURL::Parse(WebStringToGURL(url.getString()), &utf8MimeType,
> On 2017/04/07 07:52:34, kinuko wrote:
> > WebURL(url) should work (and more efficient)
> 
> I'm getting 
> 
> error: no viable conversion from 'blink::WebURL' to 'const GURL'
> 
> Did you mean something else?
> 
>
https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):
> 
>
https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:348: if
> (url.protocol() == "data") {
> On 2017/04/07 07:52:34, kinuko wrote:
> > nit: protocolIsData()
> 
> Done.
> 
>
https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/pub...
> File third_party/WebKit/public/web/WebFrameClient.h (right):
> 
>
https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/pub...
> third_party/WebKit/public/web/WebFrameClient.h:679: }
> On 2017/04/07 07:52:34, kinuko wrote:
> > This should not be in 'WebGL' category, please move or split the section.
> 
> Done.
> 
> > 
> > Is this only to check error page URL? It feels a bit sad to have all this
> > plumbing just for that.
> 
> Yes, unfortunately it's only for the error page.

dglazkov: Ping :)

[meacer, 2017-04-12 23:40:03]

meacer@chromium.org changed reviewers:
+ dcheng@chromium.org

[meacer, 2017-04-12 23:40:04]

I'd love to land this soon. dcheng@: Would you mind taking a look at
third_party/WebKit/Source/web/*?

[dcheng, 2017-04-12 23:51:24]

I didn't look at anything outside Blink (and I think you will still need another
reviewer, as I am not a Source/platform OWNER)

https://codereview.chromium.org/2702503002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right):

https://codereview.chromium.org/2702503002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkUtils.cpp:110: bool*
isSupportedMimeType) {
Nit: please update the naming style to be GetDataURLMimeType(const KURL& url,
..., bool* is_supported_mime_type) {

Same elsewhere. Also, standard Blink convention would be to pass the bool
argument by reference here, though I think in this case, we don't need the extra
parameter and can just use the return value directly? e.g. make this

if (net::DataURL::Parse(...))
  return mime_util::IsSupportedMimeType(...);
return false;

https://codereview.chromium.org/2702503002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/2702503002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:344: bool
SecurityOrigin::canNavigateInTopFrame(const KURL& url) const {
Will this be called eventually?

[kinuko, 2017-04-13 07:28:18]

I can review platform part but after dcheng's comment is addressed.

https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right):

https://codereview.chromium.org/2702503002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkUtils.cpp:113: if
(net::DataURL::Parse(WebStringToGURL(url.getString()), &utf8MimeType,
On 2017/04/11 01:08:51, Mustafa Emre Acer wrote:
> On 2017/04/07 07:52:34, kinuko wrote:
> > WebURL(url) should work (and more efficient)
> 
> I'm getting 
> 
> error: no viable conversion from 'blink::WebURL' to 'const GURL'
> 
> Did you mean something else?

Um, ok sorry we're in blink where operator GURL is not available.

[meacer, 2017-04-13 18:06:36]

kinuko: Thanks, and sorry I rebased in the middle of the review and pulled in
the blink rename changes.

https://codereview.chromium.org/2702503002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right):

https://codereview.chromium.org/2702503002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkUtils.cpp:110: bool*
isSupportedMimeType) {
On 2017/04/12 23:51:24, dcheng wrote:
> Nit: please update the naming style to be GetDataURLMimeType(const KURL& url,
> ..., bool* is_supported_mime_type) {

Done.

> 
> Same elsewhere. Also, standard Blink convention would be to pass the bool
> argument by reference here, though I think in this case, we don't need the
extra
> parameter and can just use the return value directly? e.g. make this
> 
> if (net::DataURL::Parse(...))
>   return mime_util::IsSupportedMimeType(...);
> return false;

It sounds like you prefer this to be renamed to IsDataURLMimeTypeSupported() or
something similar? Or I can move all this into SecurityOrigin.cpp since that's
the only place calling this method.

https://codereview.chromium.org/2702503002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/2702503002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:344: bool
SecurityOrigin::canNavigateInTopFrame(const KURL& url) const {
On 2017/04/12 23:51:24, dcheng wrote:
> Will this be called eventually?

It's called from FrameLoader.cpp (line 751).

[meacer, 2017-04-13 18:27:26]

meacer@chromium.org changed reviewers:
+ wjmaclean@chromium.org

[meacer, 2017-04-13 18:27:28]

wjmaclean: Can you PTAL at the webview changes:

chrome/browser/apps/guest_view/web_view_browsertest.cc
extensions/browser/guest_view/web_view/web_view_apitest.cc
chrome/test/data/extensions/platform_apps/web_view/shim/main.js
extensions/test/data/web_view/apitest/main.js

(The other webview related files are changed in 2818663002, I'll rebase this
once it lands)

[wjmaclean, 2017-04-13 18:36:08]

On 2017/04/13 18:27:28, Mustafa Emre Acer wrote:
> wjmaclean: Can you PTAL at the webview changes:
> 
> chrome/browser/apps/guest_view/web_view_browsertest.cc
> extensions/browser/guest_view/web_view/web_view_apitest.cc
> chrome/test/data/extensions/platform_apps/web_view/shim/main.js
> extensions/test/data/web_view/apitest/main.js
> 
> (The other webview related files are changed in 2818663002, I'll rebase this
> once it lands)

Sure, lgtm

[kinuko, 2017-04-14 14:29:53]

https://codereview.chromium.org/2702503002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right):

https://codereview.chromium.org/2702503002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkUtils.cpp:110: bool*
isSupportedMimeType) {
On 2017/04/13 18:06:36, Mustafa Emre Acer wrote:
> On 2017/04/12 23:51:24, dcheng wrote:
> > Nit: please update the naming style to be GetDataURLMimeType(const KURL&
url,
> > ..., bool* is_supported_mime_type) {
> 
> Done.
> 
> > 
> > Same elsewhere. Also, standard Blink convention would be to pass the bool
> > argument by reference here, though I think in this case, we don't need the
> extra
> > parameter and can just use the return value directly? e.g. make this
> > 
> > if (net::DataURL::Parse(...))
> >   return mime_util::IsSupportedMimeType(...);
> > return false;
> 
> It sounds like you prefer this to be renamed to IsDataURLMimeTypeSupported()
or
> something similar?

We're neither populating the mime_type nor using it in the call site, are we? 
If not I think just making this IsDataURLMimeTypeSupported() makes more sense.

> Or I can move all this into SecurityOrigin.cpp since that's
> the only place calling this method.

I prefer having this here so that we don't need to add mime_util code / DEPS
scattered around.

[meacer, 2017-04-14 17:51:19]

https://codereview.chromium.org/2702503002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right):

https://codereview.chromium.org/2702503002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkUtils.cpp:110: bool*
isSupportedMimeType) {
On 2017/04/14 14:29:53, kinuko wrote:
> On 2017/04/13 18:06:36, Mustafa Emre Acer wrote:
> > On 2017/04/12 23:51:24, dcheng wrote:
> > > Nit: please update the naming style to be GetDataURLMimeType(const KURL&
> url,
> > > ..., bool* is_supported_mime_type) {
> > 
> > Done.
> > 
> > > 
> > > Same elsewhere. Also, standard Blink convention would be to pass the bool
> > > argument by reference here, though I think in this case, we don't need the
> > extra
> > > parameter and can just use the return value directly? e.g. make this
> > > 
> > > if (net::DataURL::Parse(...))
> > >   return mime_util::IsSupportedMimeType(...);
> > > return false;
> > 
> > It sounds like you prefer this to be renamed to IsDataURLMimeTypeSupported()
> or
> > something similar?
> 
> We're neither populating the mime_type nor using it in the call site, are we? 
> If not I think just making this IsDataURLMimeTypeSupported() makes more sense.

Yes, I copied this over from net and missed that param. It now simply takes a
url and returns a bool.

> 
> > Or I can move all this into SecurityOrigin.cpp since that's
> > the only place calling this method.
> 
> I prefer having this here so that we don't need to add mime_util code / DEPS
> scattered around.

Sure, keeping here.

[dcheng, 2017-04-15 00:11:24]

Sorry for the review delay, I was tracing through a bunch of the loading code.

I have one minor nit and one question: I always feel nervous when I see behavior
gated on universal access. So if the answer is "yes, this is primarily needed
for layout tests", then it seems like we can remove the dependency on
SecurityOrigin. And if we move that, maybe we can just use the standard
DecidePolicyForNavigation() handler that already exists?

https://codereview.chromium.org/2702503002/diff/280001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_throttle.cc (right):

https://codereview.chromium.org/2702503002/diff/280001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_throttle.cc:57: return
base::WrapUnique(new DataUrlNavigationThrottle(navigation_handle));
Nit: Prefer base::MakeUnique<DataUrlNavigationThrottle>(navigation_handle)

https://codereview.chromium.org/2702503002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/2702503002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:341: if
(universal_access_)
I assume this exception is required for layout tests?

If so, maybe we can use LayoutTestSupport::IsRunningLayoutTest() to avoid having
to add this to SecurityOrigin?

(It seems like the rest of this logic is not SecurityOrigin dependent)

[meacer, 2017-04-15 00:47:43]

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

[commit-bot: I haz the power, 2017-04-15 00:48:02]

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

[meacer, 2017-04-15 00:48:39]

The CQ bit was unchecked by meacer@chromium.org

[meacer, 2017-04-15 00:48:43]

Patchset #15 (id:300001) has been deleted

[meacer, 2017-04-15 00:53:53]

https://codereview.chromium.org/2702503002/diff/280001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_throttle.cc (right):

https://codereview.chromium.org/2702503002/diff/280001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_throttle.cc:57: return
base::WrapUnique(new DataUrlNavigationThrottle(navigation_handle));
On 2017/04/15 00:11:24, dcheng wrote:
> Nit: Prefer base::MakeUnique<DataUrlNavigationThrottle>(navigation_handle)

Done.

https://codereview.chromium.org/2702503002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/2702503002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:341: if
(universal_access_)
On 2017/04/15 00:11:24, dcheng wrote:
> I assume this exception is required for layout tests?
> 
> If so, maybe we can use LayoutTestSupport::IsRunningLayoutTest() to avoid
having
> to add this to SecurityOrigin?
> 
> (It seems like the rest of this logic is not SecurityOrigin dependent)

As we discussed offline, it's not required for layout tests. I kept it in to be
consistent with other checks (e.g. canDisplay method above). Since there is no
equivalent of this bit on the browser side, I'm removing this and moving the
rest of the checks to FrameLoader.cpp.

[dcheng, 2017-04-15 01:09:38]

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:759:
!frame_->Client()->AllowContentInitiatedDataUrlNavigations(
Sorry, to follow up on my other question: would it make sense to move this check
into DecidePolicyForNavigation() rather than adding another entry point for
navigation policy checks?

[meacer, 2017-04-17 22:21:58]

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:759:
!frame_->Client()->AllowContentInitiatedDataUrlNavigations(
On 2017/04/15 01:09:38, dcheng wrote:
> Sorry, to follow up on my other question: would it make sense to move this
check
> into DecidePolicyForNavigation() rather than adding another entry point for
> navigation policy checks?

Quick update: I moved these to DecidePolicyForNavigation() but it broke a large
number of tests that are using RenderFrameTest::LoadHTML. Currently looking into
it.

[meacer, 2017-04-19 00:05:54]

dcheng is OOO, but the suggestion in his last comment doesn't seam feasible
without breaking a large number of tests. I'd like to avoid fixing another set
of tests.

kinuko: Does the blink parts LGTY?

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:759:
!frame_->Client()->AllowContentInitiatedDataUrlNavigations(
On 2017/04/17 22:21:58, Mustafa Emre Acer wrote:
> On 2017/04/15 01:09:38, dcheng wrote:
> > Sorry, to follow up on my other question: would it make sense to move this
> check
> > into DecidePolicyForNavigation() rather than adding another entry point for
> > navigation policy checks?
> 
> Quick update: I moved these to DecidePolicyForNavigation() but it broke a
large
> number of tests that are using RenderFrameTest::LoadHTML. Currently looking
into
> it.

Okay, I've been looking at this, and there are tests that I don't know how to
fix. The latest CL that does these checks in DecidePolicyForNavigation is at
https://codereview.chromium.org/2787123005/. So I'd like to land this as is.

[kinuko, 2017-04-19 07:07:44]

lgtm (but I'm not web owner, so you need one more stamp)

https://codereview.chromium.org/2702503002/diff/320001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_throttle.h (right):

https://codereview.chromium.org/2702503002/diff/320001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_throttle.h:17:
DataUrlNavigationThrottle(NavigationHandle* navigation_handle);
nit: explicit

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:330: // to a data URL.
Please add a TODO to consider converging this into DecidePolicyForNavigation

[dcheng, 2017-04-19 11:59:49]

LGTM

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1008: if (!frame)
Nit: just make this a non-static method (unlike ReportLocalLoadFailed, we don't
need to call it from elsewhere--and probably ReportLocalLoadFailed should just
be non-static, but that's out of scope here)

And frame (and frame_) if not static can never be null here.

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right):

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkUtils.cpp:111: if
(net::DataURL::Parse(WebStringToGURL(url.GetString()), &utf8_mime_type,
Hmm... it would be kind of nice if we had a way to turn KURL into GURL without
reparsing here (out of scope of this CL though)

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:330: // to a data URL.
On 2017/04/19 07:07:44, kinuko wrote:
> Please add a TODO to consider converging this into DecidePolicyForNavigation

+1

[kinuko, 2017-04-20 02:27:00]

On 2017/04/19 11:59:49, dcheng (OOO through May 2) wrote:
> LGTM
> 
>
https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):
> 
>
https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/loader/FrameLoader.cpp:1008: if (!frame)
> Nit: just make this a non-static method (unlike ReportLocalLoadFailed, we
don't
> need to call it from elsewhere--and probably ReportLocalLoadFailed should just
> be non-static, but that's out of scope here)
> 
> And frame (and frame_) if not static can never be null here.
> 
>
https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right):
> 
>
https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/network/NetworkUtils.cpp:111: if
> (net::DataURL::Parse(WebStringToGURL(url.GetString()), &utf8_mime_type,
> Hmm... it would be kind of nice if we had a way to turn KURL into GURL without
> reparsing here (out of scope of this CL though)

I agree.  We can try removing ifdefs in WebURL for one thing, but anyways it can
be out of scope this CL

>
https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/pub...
> File third_party/WebKit/public/web/WebFrameClient.h (right):
> 
>
https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/pub...
> third_party/WebKit/public/web/WebFrameClient.h:330: // to a data URL.
> On 2017/04/19 07:07:44, kinuko wrote:
> > Please add a TODO to consider converging this into DecidePolicyForNavigation
> 
> +1

[meacer, 2017-04-21 01:30:37]

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

[commit-bot: I haz the power, 2017-04-21 01:30:56]

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

[meacer, 2017-04-21 01:31:21]

Addressed comments, rebased, changed the console message slightly and fixed
remaining layout tests that I missed earlier.

https://codereview.chromium.org/2702503002/diff/320001/content/browser/frame_...
File content/browser/frame_host/data_url_navigation_throttle.h (right):

https://codereview.chromium.org/2702503002/diff/320001/content/browser/frame_...
content/browser/frame_host/data_url_navigation_throttle.h:17:
DataUrlNavigationThrottle(NavigationHandle* navigation_handle);
On 2017/04/19 07:07:44, kinuko wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1008: if (!frame)
On 2017/04/19 11:59:49, dcheng (OOO through May 2) wrote:
> Nit: just make this a non-static method (unlike ReportLocalLoadFailed, we
don't
> need to call it from elsewhere--and probably ReportLocalLoadFailed should just
> be non-static, but that's out of scope here)
> 
> And frame (and frame_) if not static can never be null here.

This used to be called from outside FrameLoader but not anymore, so I just
inlined it into PrepareRequestForThisFrame.

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right):

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkUtils.cpp:111: if
(net::DataURL::Parse(WebStringToGURL(url.GetString()), &utf8_mime_type,
On 2017/04/19 11:59:49, dcheng (OOO through May 2) wrote:
> Hmm... it would be kind of nice if we had a way to turn KURL into GURL without
> reparsing here (out of scope of this CL though)

Acknowledged. I can file a bug, but there doesn't seem to be many places doing
this conversion.

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2702503002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:330: // to a data URL.
On 2017/04/19 11:59:49, dcheng (OOO through May 2) wrote:
> On 2017/04/19 07:07:44, kinuko wrote:
> > Please add a TODO to consider converging this into DecidePolicyForNavigation
> 
> +1

Done.

[commit-bot: I haz the power, 2017-04-21 02:30:21]

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

[commit-bot: I haz the power, 2017-04-21 02:30:23]

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_...)

[meacer, 2017-04-21 21:17:48]

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

[commit-bot: I haz the power, 2017-04-21 21:18:18]

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, 2017-04-22 00:00:33]

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

[commit-bot: I haz the power, 2017-04-22 00:00:35]

Dry run: This issue passed the CQ dry run.

[meacer, 2017-04-22 00:01:06]

Tests fixed, landing. Thanks everyone.

[meacer, 2017-04-22 00:01:19]

The CQ bit was checked by meacer@chromium.org

[meacer, 2017-04-22 00:01:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, avi@chromium.org, nasko@chromium.org,
wjmaclean@chromium.org, dcheng@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2702503002/#ps360001
(title: "Fix Android PDF tests where PDFs should be downloaded")

[commit-bot: I haz the power, 2017-04-22 00:02:27]

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, 2017-04-22 00:08:16]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1492819279800030,
"parent_rev": "8ee6041580e6fa3d6bff3b86088491f9d0291fc0", "commit_rev":
"ba52f56207a4b9d70b34880fbff2352e71a06422"}

[commit-bot: I haz the power, 2017-04-22 00:08:49]

Description was changed from

==========
Block renderer-initiated main frame navigations to data URLs

This CL implements the blocking of data URL navigations as described in the
blink intent to deprecate and remove thread at https://goo.gl/BaZAea.

The blocking is done in two separate places:

- blink::FrameLoader: This is a renderer side check that blocks all top-frame
  loads for known mime types. This check ignores unknown mime types as those
  can end up as downloads or be handled by plugins.

- content::DataURLNavigationThrottler: This is a browser side check that
  handles data URLs that were ignored by the renderer check. By this point, the
  determination of whether the URL is a download or not has already been made.
  This check allows downloads and blocks remaining URLs (ie. mime types that
  are handled by plugins). When browser side navigation is enabled, all blocking
  is done in this throttler instead of some checks being in blink::FrameLoader.

This CL moves data URL navigation tests to a separate file, and removes layout
tests that are no longer realistic.

TEST=data_url_navigation_browsertest.cc
BUG=594215
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Block renderer-initiated main frame navigations to data URLs

This CL implements the blocking of data URL navigations as described in the
blink intent to deprecate and remove thread at https://goo.gl/BaZAea.

The blocking is done in two separate places:

- blink::FrameLoader: This is a renderer side check that blocks all top-frame
  loads for known mime types. This check ignores unknown mime types as those
  can end up as downloads or be handled by plugins.

- content::DataURLNavigationThrottler: This is a browser side check that
  handles data URLs that were ignored by the renderer check. By this point, the
  determination of whether the URL is a download or not has already been made.
  This check allows downloads and blocks remaining URLs (ie. mime types that
  are handled by plugins). When browser side navigation is enabled, all blocking
  is done in this throttler instead of some checks being in blink::FrameLoader.

This CL moves data URL navigation tests to a separate file, and removes layout
tests that are no longer realistic.

TEST=data_url_navigation_browsertest.cc
BUG=594215
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2702503002
Cr-Commit-Position: refs/heads/master@{#466504}
Committed:
https://chromium.googlesource.com/chromium/src/+/ba52f56207a4b9d70b34880fbff2...
==========

[commit-bot: I haz the power, 2017-04-22 00:08:51]

Committed patchset #17 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/ba52f56207a4b9d70b34880fbff2...