Make the content scheme local

Make the content scheme local

WebContent should not be able to embed or redirect to content scheme.

BUG=659492
Committed: https://crrev.com/c7c7daa089804c779c4ebfb2a2b5793be6b47090
Cr-Commit-Position: refs/heads/master@{#428222}

[sgurun-gerrit only, 2016-10-27 17:50:16]

Description was changed from

==========
Make the content scheme local

WebContent should not be able to embed or redirect to content scheme.

BUG=659492
==========

to

==========
Make the content scheme local

WebContent should not be able to embed or redirect to content scheme.

BUG=659492
==========

[sgurun-gerrit only, 2016-10-27 17:50:16]

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

[sgurun-gerrit only, 2016-10-27 17:50:39]

On 2016/10/27 17:50:16, sgurun wrote:
> mailto:sgurun@chromium.org changed reviewers:
> + mailto:dcheng@chromium.org

dcheng, PTAL.

I will ask for an owner's stamp after.

[dcheng, 2016-10-27 18:20:09]

LGTM, but please get creis@ to review: I'm considered about his GrantScheme
comment on the bug, and we should verify that there's no other issues.

[dcheng, 2016-10-27 18:20:42]

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

[dcheng, 2016-10-27 18:20:43]

+creis for real

[sgurun-gerrit only, 2016-10-27 18:22:34]

On 2016/10/27 18:20:43, dcheng wrote:
> +creis for real

thanks dcheng!

creis, PTAL. 

If the only concern is for GrantScheme, I think this is webview specific and we
may need a little more investigation there to address properly.

[Charlie Reis, 2016-10-27 18:25:37]

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

[dcheng, 2016-10-27 18:26:15]

Actually, did you test if this is sufficient to make content:// URLs
cross-origin from each other? It doesn't seem obvious to me that that's the case
from reading through SecurityOrigin. A simple test should be able to determine
this.

[commit-bot: I haz the power, 2016-10-27 18:26:38]

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

[Charlie Reis, 2016-10-27 18:29:51]

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

[Charlie Reis, 2016-10-27 18:29:51]

[+nick]

Adding Nick, who's writing a response on the bug.  (He knows these checks better
than I do.)

This seems ok, and the GrantScheme thing probably isn't needed.

On 2016/10/27 18:26:15, dcheng wrote:
> Actually, did you test if this is sufficient to make content:// URLs
> cross-origin from each other? It doesn't seem obvious to me that that's the
case
> from reading through SecurityOrigin. A simple test should be able to determine
> this.

My guess is no, and we should try to fix that as well.  We'll want content://
URLs to be treated as unique origins if possible.

Any chance of getting a test included in this CL?  I'd like to verify that this
disrupts the attack.  I also kicked off a CQ dry run to see what the current try
jobs think about it.

[qinmin, 2016-10-27 18:31:52]

On 2016/10/27 18:29:51, Charlie Reis wrote:
> [+nick]
> 
> Adding Nick, who's writing a response on the bug.  (He knows these checks
better
> than I do.)
> 
> This seems ok, and the GrantScheme thing probably isn't needed.
> 
> On 2016/10/27 18:26:15, dcheng wrote:
> > Actually, did you test if this is sufficient to make content:// URLs
> > cross-origin from each other? It doesn't seem obvious to me that that's the
> case
> > from reading through SecurityOrigin. A simple test should be able to
determine
> > this.
> 
> My guess is no, and we should try to fix that as well.  We'll want content://
> URLs to be treated as unique origins if possible.
> 
> Any chance of getting a test included in this CL?  I'd like to verify that
this
> disrupts the attack.  I also kicked off a CQ dry run to see what the current
try
> jobs think about it.

Can we add the test in a separate CL? This is for cherry-pick on to the release
branch, so it would be cool if we can get this in first to catch today's build

[Charlie Reis, 2016-10-27 18:34:54]

On 2016/10/27 18:31:52, qinmin wrote:
> On 2016/10/27 18:29:51, Charlie Reis wrote:
> > [+nick]
> > 
> > Adding Nick, who's writing a response on the bug.  (He knows these checks
> better
> > than I do.)
> > 
> > This seems ok, and the GrantScheme thing probably isn't needed.
> > 
> > On 2016/10/27 18:26:15, dcheng wrote:
> > > Actually, did you test if this is sufficient to make content:// URLs
> > > cross-origin from each other? It doesn't seem obvious to me that that's
the
> > case
> > > from reading through SecurityOrigin. A simple test should be able to
> determine
> > > this.
> > 
> > My guess is no, and we should try to fix that as well.  We'll want
content://
> > URLs to be treated as unique origins if possible.
> > 
> > Any chance of getting a test included in this CL?  I'd like to verify that
> this
> > disrupts the attack.  I also kicked off a CQ dry run to see what the current
> try
> > jobs think about it.
> 
> Can we add the test in a separate CL? This is for cherry-pick on to the
release
> branch, so it would be cool if we can get this in first to catch today's build

That's a bad idea in general.  Rushing a fix without having a test means it's
much more likely to introduce regressions (or not actually fix the bug if we
make tweaks during review).  Having a test shouldn't make the merge much harder,
but if it does, I'd be ok with having the test ready to land in a separate CL as
long as we can verify the fix with it.

[dcheng, 2016-10-27 18:37:04]

On 2016/10/27 18:29:51, Charlie Reis wrote:
> [+nick]
> 
> Adding Nick, who's writing a response on the bug.  (He knows these checks
better
> than I do.)
> 
> This seems ok, and the GrantScheme thing probably isn't needed.
> 
> On 2016/10/27 18:26:15, dcheng wrote:
> > Actually, did you test if this is sufficient to make content:// URLs
> > cross-origin from each other? It doesn't seem obvious to me that that's the
> case
> > from reading through SecurityOrigin. A simple test should be able to
determine
> > this.
> 
> My guess is no, and we should try to fix that as well.  We'll want content://
> URLs to be treated as unique origins if possible.
> 
> Any chance of getting a test included in this CL?  I'd like to verify that
this
> disrupts the attack.  I also kicked off a CQ dry run to see what the current
try
> jobs think about it.

I did some more reading of SecurityOrigin::canAccess, and I *think* it's
probably safe, but it's a bit convoluted.

When we create the URL, shouldTreatAsUniqueOrigin returns false. However, it's
marked as a local URL.

Later on, in canAccess(), there's this block:

  if (canAccess && isLocal())
    canAccess = passesFileCheck(other);

|passesFileCheck| is defined like this:

bool SecurityOrigin::passesFileCheck(const SecurityOrigin* other) const {
  ASSERT(isLocal() && other->isLocal());

  return !m_blockLocalAccessFromLocalOrigin &&
         !other->m_blockLocalAccessFromLocalOrigin;
}

m_blockLocalAccessFromLocalOrigin is set to true if allowFileAccessFromFileURLs
is *false*. The default in Blink is true, but the default value
WebPreferences::allow_file_access_from_files_urls is false. In Chromium, we
shouldn't change this default, which means the Blink default is overwritten...

So in summary, it's safe but very complicated.

[dcheng, 2016-10-27 18:39:15]

On 2016/10/27 18:37:04, dcheng wrote:
> On 2016/10/27 18:29:51, Charlie Reis wrote:
> > [+nick]
> > 
> > Adding Nick, who's writing a response on the bug.  (He knows these checks
> better
> > than I do.)
> > 
> > This seems ok, and the GrantScheme thing probably isn't needed.
> > 
> > On 2016/10/27 18:26:15, dcheng wrote:
> > > Actually, did you test if this is sufficient to make content:// URLs
> > > cross-origin from each other? It doesn't seem obvious to me that that's
the
> > case
> > > from reading through SecurityOrigin. A simple test should be able to
> determine
> > > this.
> > 
> > My guess is no, and we should try to fix that as well.  We'll want
content://
> > URLs to be treated as unique origins if possible.
> > 
> > Any chance of getting a test included in this CL?  I'd like to verify that
> this
> > disrupts the attack.  I also kicked off a CQ dry run to see what the current
> try
> > jobs think about it.
> 
> I did some more reading of SecurityOrigin::canAccess, and I *think* it's
> probably safe, but it's a bit convoluted.
> 
> When we create the URL, shouldTreatAsUniqueOrigin returns false. However, it's
> marked as a local URL.
> 
> Later on, in canAccess(), there's this block:
> 
>   if (canAccess && isLocal())
>     canAccess = passesFileCheck(other);
> 
> |passesFileCheck| is defined like this:
> 
> bool SecurityOrigin::passesFileCheck(const SecurityOrigin* other) const {
>   ASSERT(isLocal() && other->isLocal());
> 
>   return !m_blockLocalAccessFromLocalOrigin &&
>          !other->m_blockLocalAccessFromLocalOrigin;
> }
> 
> m_blockLocalAccessFromLocalOrigin is set to true if
allowFileAccessFromFileURLs
> is *false*. The default in Blink is true, but the default value
> WebPreferences::allow_file_access_from_files_urls is false. In Chromium, we
> shouldn't change this default, which means the Blink default is overwritten...
> 
> So in summary, it's safe but very complicated.

To clarify, the default value of allowFileAccessFromFileURLs is *true* in Blink,
which means m_blockLocalAccessFromLocalOrigin is *false* by default. This means
local files can access each other.

However, because we always overwrite the settings in the renderer based on the
values in WebPreferences, Blink as used by the Chromium browser always sets
allowFileAccessFromFileURLS to false, which means
m_blockLocalAccessFromLocalOrigin should always be true. Thus, in the actual
browser, this access check should fail...

[sgurun-gerrit only, 2016-10-27 18:42:11]

On 2016/10/27 18:39:15, dcheng wrote:
> On 2016/10/27 18:37:04, dcheng wrote:
> > On 2016/10/27 18:29:51, Charlie Reis wrote:
> > > [+nick]
> > > 
> > > Adding Nick, who's writing a response on the bug.  (He knows these checks
> > better
> > > than I do.)
> > > 
> > > This seems ok, and the GrantScheme thing probably isn't needed.
> > > 
> > > On 2016/10/27 18:26:15, dcheng wrote:
> > > > Actually, did you test if this is sufficient to make content:// URLs
> > > > cross-origin from each other? It doesn't seem obvious to me that that's
> the
> > > case
> > > > from reading through SecurityOrigin. A simple test should be able to
> > determine
> > > > this.
> > > 
> > > My guess is no, and we should try to fix that as well.  We'll want
> content://
> > > URLs to be treated as unique origins if possible.
> > > 
> > > Any chance of getting a test included in this CL?  I'd like to verify that
> > this
> > > disrupts the attack.  I also kicked off a CQ dry run to see what the
current
> > try
> > > jobs think about it.
> > 
> > I did some more reading of SecurityOrigin::canAccess, and I *think* it's
> > probably safe, but it's a bit convoluted.
> > 
> > When we create the URL, shouldTreatAsUniqueOrigin returns false. However,
it's
> > marked as a local URL.
> > 
> > Later on, in canAccess(), there's this block:
> > 
> >   if (canAccess && isLocal())
> >     canAccess = passesFileCheck(other);
> > 
> > |passesFileCheck| is defined like this:
> > 
> > bool SecurityOrigin::passesFileCheck(const SecurityOrigin* other) const {
> >   ASSERT(isLocal() && other->isLocal());
> > 
> >   return !m_blockLocalAccessFromLocalOrigin &&
> >          !other->m_blockLocalAccessFromLocalOrigin;
> > }
> > 
> > m_blockLocalAccessFromLocalOrigin is set to true if
> allowFileAccessFromFileURLs
> > is *false*. The default in Blink is true, but the default value
> > WebPreferences::allow_file_access_from_files_urls is false. In Chromium, we
> > shouldn't change this default, which means the Blink default is
overwritten...
> > 
> > So in summary, it's safe but very complicated.
> 
> To clarify, the default value of allowFileAccessFromFileURLs is *true* in
Blink,
> which means m_blockLocalAccessFromLocalOrigin is *false* by default. This
means
> local files can access each other.
> 
> However, because we always overwrite the settings in the renderer based on the
> values in WebPreferences, Blink as used by the Chromium browser always sets
> allowFileAccessFromFileURLS to false, which means
> m_blockLocalAccessFromLocalOrigin should always be true. Thus, in the actual
> browser, this access check should fail...

I did a manual test, I will update the bug with details on it. I can write a
test, are there any pointers to similar tests?

[ncarter (slow), 2016-10-27 18:58:08]

On 2016/10/27 18:42:11, sgurun wrote:
> On 2016/10/27 18:39:15, dcheng wrote:
> > On 2016/10/27 18:37:04, dcheng wrote:
> > > On 2016/10/27 18:29:51, Charlie Reis wrote:
> > > > [+nick]
> > > > 
> > > > Adding Nick, who's writing a response on the bug.  (He knows these
checks
> > > better
> > > > than I do.)
> > > > 
> > > > This seems ok, and the GrantScheme thing probably isn't needed.
> > > > 
> > > > On 2016/10/27 18:26:15, dcheng wrote:
> > > > > Actually, did you test if this is sufficient to make content:// URLs
> > > > > cross-origin from each other? It doesn't seem obvious to me that
that's
> > the
> > > > case
> > > > > from reading through SecurityOrigin. A simple test should be able to
> > > determine
> > > > > this.
> > > > 
> > > > My guess is no, and we should try to fix that as well.  We'll want
> > content://
> > > > URLs to be treated as unique origins if possible.
> > > > 
> > > > Any chance of getting a test included in this CL?  I'd like to verify
that
> > > this
> > > > disrupts the attack.  I also kicked off a CQ dry run to see what the
> current
> > > try
> > > > jobs think about it.
> > > 
> > > I did some more reading of SecurityOrigin::canAccess, and I *think* it's
> > > probably safe, but it's a bit convoluted.
> > > 
> > > When we create the URL, shouldTreatAsUniqueOrigin returns false. However,
> it's
> > > marked as a local URL.
> > > 
> > > Later on, in canAccess(), there's this block:
> > > 
> > >   if (canAccess && isLocal())
> > >     canAccess = passesFileCheck(other);
> > > 
> > > |passesFileCheck| is defined like this:
> > > 
> > > bool SecurityOrigin::passesFileCheck(const SecurityOrigin* other) const {
> > >   ASSERT(isLocal() && other->isLocal());
> > > 
> > >   return !m_blockLocalAccessFromLocalOrigin &&
> > >          !other->m_blockLocalAccessFromLocalOrigin;
> > > }
> > > 
> > > m_blockLocalAccessFromLocalOrigin is set to true if
> > allowFileAccessFromFileURLs
> > > is *false*. The default in Blink is true, but the default value
> > > WebPreferences::allow_file_access_from_files_urls is false. In Chromium,
we
> > > shouldn't change this default, which means the Blink default is
> overwritten...
> > > 
> > > So in summary, it's safe but very complicated.
> > 
> > To clarify, the default value of allowFileAccessFromFileURLs is *true* in
> Blink,
> > which means m_blockLocalAccessFromLocalOrigin is *false* by default. This
> means
> > local files can access each other.
> > 
> > However, because we always overwrite the settings in the renderer based on
the
> > values in WebPreferences, Blink as used by the Chromium browser always sets
> > allowFileAccessFromFileURLS to false, which means
> > m_blockLocalAccessFromLocalOrigin should always be true. Thus, in the actual
> > browser, this access check should fail...
> 
> I did a manual test, I will update the bug with details on it. I can write a
> test, are there any pointers to similar tests?

I think the GrantScheme question is essentially an orthogonal issue. Or rather,
it's enforcement of the same issue at a different layer. It should be okay to
tighten the blink behavior without also tightening the
ChildProcessSecurityPolicy permissions, as the latter exist to enforce IPC layer
security in the browser process.

[Charlie Reis, 2016-10-27 19:18:33]

On 2016/10/27 18:37:04, dcheng wrote:
> On 2016/10/27 18:29:51, Charlie Reis wrote:
> > [+nick]
> > 
> > Adding Nick, who's writing a response on the bug.  (He knows these checks
> better
> > than I do.)
> > 
> > This seems ok, and the GrantScheme thing probably isn't needed.
> > 
> > On 2016/10/27 18:26:15, dcheng wrote:
> > > Actually, did you test if this is sufficient to make content:// URLs
> > > cross-origin from each other? It doesn't seem obvious to me that that's
the
> > case
> > > from reading through SecurityOrigin. A simple test should be able to
> determine
> > > this.
> > 
> > My guess is no, and we should try to fix that as well.  We'll want
content://
> > URLs to be treated as unique origins if possible.
> > 
> > Any chance of getting a test included in this CL?  I'd like to verify that
> this
> > disrupts the attack.  I also kicked off a CQ dry run to see what the current
> try
> > jobs think about it.
> 
> I did some more reading of SecurityOrigin::canAccess, and I *think* it's
> probably safe, but it's a bit convoluted.
> 
> When we create the URL, shouldTreatAsUniqueOrigin returns false. However, it's
> marked as a local URL.
> 
> Later on, in canAccess(), there's this block:
> 
>   if (canAccess && isLocal())
>     canAccess = passesFileCheck(other);
> 
> |passesFileCheck| is defined like this:
> 
> bool SecurityOrigin::passesFileCheck(const SecurityOrigin* other) const {
>   ASSERT(isLocal() && other->isLocal());
> 
>   return !m_blockLocalAccessFromLocalOrigin &&
>          !other->m_blockLocalAccessFromLocalOrigin;
> }
> 
> m_blockLocalAccessFromLocalOrigin is set to true if
allowFileAccessFromFileURLs
> is *false*. The default in Blink is true, but the default value
> WebPreferences::allow_file_access_from_files_urls is false. In Chromium, we
> shouldn't change this default, which means the Blink default is overwritten...
> 
> So in summary, it's safe but very complicated.

Yes, Mike West came to the same conclusion in comment 53 on the bug.

That means this fix LGTM.  (I feel a bit better after Nick's observation that
ChromeOS is doing something similar a few lines above for their
content::kExternalFileScheme.)

I would still recommend having a test if you can add one in time for today's
build.

On 2016/10/27 18:42:11, sgurun wrote:
> I did a manual test, I will update the bug with details on it. I can write a
> test, are there any pointers to similar tests?

In Android?  I don't know.  A simple browser test that tries to do a
script-initiated navigation to a known content:// URL would be sufficient, if we
can verify that the navigation works without your fix and fails with it.

[Charlie Reis, 2016-10-27 19:20:55]

Hmm, there's a concerning test failure: 
org.chromium.chrome.browser.UrlSchemeTest#testContentUrlFromData

Are data URLs or web pages supposed to be able to load content:// URLs in
images?  (I would hope that's only allowed for Android Webview, but the test
looks like it's for Chrome.)  That sounds concerning.

[commit-bot: I haz the power, 2016-10-27 19:36:36]

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

[commit-bot: I haz the power, 2016-10-27 19:36:37]

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

[sgurun-gerrit only, 2016-10-27 19:46:21]

On 2016/10/27 19:20:55, Charlie Reis wrote:
> Hmm, there's a concerning test failure: 
> org.chromium.chrome.browser.UrlSchemeTest#testContentUrlFromData
> 
> Are data URLs or web pages supposed to be able to load content:// URLs in
> images?  (I would hope that's only allowed for Android Webview, but the test
> looks like it's for Chrome.)  That sounds concerning.

that is a chrome test, looking. The change here does not impact webview anyway.

[qinmin, 2016-10-27 19:48:18]

On 2016/10/27 19:20:55, Charlie Reis wrote:
> Hmm, there's a concerning test failure: 
> org.chromium.chrome.browser.UrlSchemeTest#testContentUrlFromData
> 
> Are data URLs or web pages supposed to be able to load content:// URLs in
> images?  (I would hope that's only allowed for Android Webview, but the test
> looks like it's for Chrome.)  That sounds concerning.

We shouldn't support that inside chrome. These tests was introduced in
https://chrome-internal-review.googlesource.com/#/c/189506/, but I don't think
these are proper behaviors.

[sgurun-gerrit only, 2016-10-27 19:50:42]

On 2016/10/27 19:48:18, qinmin wrote:
> On 2016/10/27 19:20:55, Charlie Reis wrote:
> > Hmm, there's a concerning test failure: 
> > org.chromium.chrome.browser.UrlSchemeTest#testContentUrlFromData
> > 
> > Are data URLs or web pages supposed to be able to load content:// URLs in
> > images?  (I would hope that's only allowed for Android Webview, but the test
> > looks like it's for Chrome.)  That sounds concerning.
> 
> We shouldn't support that inside chrome. These tests was introduced in
> https://chrome-internal-review.googlesource.com/#/c/189506/, but I don't think
> these are proper behaviors.

I will delete the tests then

[sgurun-gerrit only, 2016-10-27 22:19:11]

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

[commit-bot: I haz the power, 2016-10-27 22:20:01]

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

[sgurun-gerrit only, 2016-10-27 22:27:10]

sgurun@chromium.org changed reviewers:
+ thestig@chromium.org

[sgurun-gerrit only, 2016-10-27 22:27:53]

On 2016/10/27 19:50:42, sgurun wrote:
> On 2016/10/27 19:48:18, qinmin wrote:
> > On 2016/10/27 19:20:55, Charlie Reis wrote:
> > > Hmm, there's a concerning test failure: 
> > > org.chromium.chrome.browser.UrlSchemeTest#testContentUrlFromData
> > > 
> > > Are data URLs or web pages supposed to be able to load content:// URLs in
> > > images?  (I would hope that's only allowed for Android Webview, but the
test
> > > looks like it's for Chrome.)  That sounds concerning.
> > 
> > We shouldn't support that inside chrome. These tests was introduced in
> > https://chrome-internal-review.googlesource.com/#/c/189506/, but I don't
think
> > these are proper behaviors.
> 
> I will delete the tests then

testInfoBarContainerSwapsWebContents test was failing in Nick's CL here
https://codereview.chromium.org/2456123002 but locally it is passing. Also
looking at the code, there is no relation, so I will assume it is a flake.
thestig, can we get a owner's review please?

[Lei Zhang, 2016-10-27 22:34:34]

lgtm stamp

https://codereview.chromium.org/2456883002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2456883002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:730: static const char* const
kProtocolList[] = {
We really should sort this list some other time.

[ncarter (slow), 2016-10-27 22:36:13]

lgtm

[commit-bot: I haz the power, 2016-10-27 23:38:39]

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

[commit-bot: I haz the power, 2016-10-27 23:38:39]

Dry run: This issue passed the CQ dry run.

[sgurun-gerrit only, 2016-10-27 23:40:45]

The CQ bit was checked by sgurun@chromium.org

[sgurun-gerrit only, 2016-10-27 23:40:45]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2456883002/#ps40001
(title: "added content scheme to handled protocol list")

[commit-bot: I haz the power, 2016-10-27 23:41: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, 2016-10-27 23:48:40]

Description was changed from

==========
Make the content scheme local

WebContent should not be able to embed or redirect to content scheme.

BUG=659492
==========

to

==========
Make the content scheme local

WebContent should not be able to embed or redirect to content scheme.

BUG=659492
==========

[commit-bot: I haz the power, 2016-10-27 23:48:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-27 23:52:45]

Description was changed from

==========
Make the content scheme local

WebContent should not be able to embed or redirect to content scheme.

BUG=659492
==========

to

==========
Make the content scheme local

WebContent should not be able to embed or redirect to content scheme.

BUG=659492

Committed: https://crrev.com/c7c7daa089804c779c4ebfb2a2b5793be6b47090
Cr-Commit-Position: refs/heads/master@{#428222}
==========

[commit-bot: I haz the power, 2016-10-27 23:52:46]

Patchset 3 (id:??) landed as
https://crrev.com/c7c7daa089804c779c4ebfb2a2b5793be6b47090
Cr-Commit-Position: refs/heads/master@{#428222}