Tighten IO thread blob/filesystem URL checks for apps with webview permission.

Tighten IO thread blob/filesystem URL checks for apps with webview permission.

The IO thread blob/filesystem URL security checks were originally
introduced in r419019 and r422954.  This was merged back to M54 and
M55.

They were later moved to the UI thread (into
ExtensionNavigationThrottle) in r424937 to make them compatible with
PlzNavigate.  However, that only exists in M56.

Per linked bug, we need to tighten the security check currently in M54
and M55 for apps with a "webview" permission where the problematic
blob/filesystem URL requests aren't made from the guest process.

This is the plan for fixing this:
1.  https://codereview.chromium.org/2435593007/: Reintroduce these IO
    thread checks on M56.
2.  <This CL> Tighten the checks for the scenario above using
    ChildProcessSecurityPolicy.
3.  Merge the CL from step 2 back to M55 and M54.
4.  Remove these checks from M56 and tighten the checks in
    ExtensionNavigationThrottle.

The goal of this CL is to determine if a request is being made by a
guest process on the IO thread.  It relies on the fact that
ChildProcessSecurityPolicyImpl already has the origin
"chrome-extension://<appid>" in the origin_set_ for the guest process,
thanks to the GrantOrigin that was added to
WebViewGuest::CreateWebContents in r422976 as part of fixing issue
652784.  Therefore, it's sufficient to check whether the requested
nested URL's origin is in the origin_set_ for the process making the
request.

BUG=656752
Committed: https://crrev.com/f7af135c51a99daba0cdb5ef204465ff995802e4
Cr-Commit-Position: refs/heads/master@{#426870}

[alexmos, 2016-10-20 00:46:03]

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

[commit-bot: I haz the power, 2016-10-20 00:46:46]

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

[commit-bot: I haz the power, 2016-10-20 04:16:17]

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

[commit-bot: I haz the power, 2016-10-20 04:16:19]

Dry run: This issue passed the CQ dry run.

[alexmos, 2016-10-20 16:35:25]

Description was changed from

==========
Tighten IO thread blob/filesystem URL checks for apps with webview permission.

The IO thread blob/filesystem URL security checks were originally
introduced in r419019 and r422954.  This was merged back to M54 and
M55.

They were later moved to the UI thread (into
ExtensionNavigationThrottle) in r424937 to make them compatible with
PlzNavigate.  However, that only exists in M56.

Per linked bug, we need to tighten the security check currently in M54
and M55 for apps with a "webview" permission where the problematic
blob/filesystem URL requests aren't made from the guest process.

This is the plan for fixing this:
1.  <https://codereview.chromium.org/2435593007/> Reintroduce these IO
    thread checks on M56.
2.  <this CL> Tighten the checks for the scenario above using
    ChildProcessSecurityPolicy.
3.  Merge the CL from step 2 back to M55 and M54.
4.  Remove these checks from M56 and tighten the checks in
    ExtensionNavigationThrottle.

The goal of this CL is to determine if a request is being made by a
guest process on the IO thread.  It relies on the fact that
ChildProcessSecurityPolicyImpl already has the origin
"chrome-extension://<appid>" in the origin_set_ for the guest process,
thanks to the GrantOrigin that was added to
WebViewGuest::CreateWebContents in r422976 as part of fixing issue
652784.  Therefore, it's sufficient to check whether the requested
nested URL's origin is in the origin_set_ for the process making the
request.

BUG=656752
==========

to

==========
Tighten IO thread blob/filesystem URL checks for apps with webview permission.

The IO thread blob/filesystem URL security checks were originally
introduced in r419019 and r422954.  This was merged back to M54 and
M55.

They were later moved to the UI thread (into
ExtensionNavigationThrottle) in r424937 to make them compatible with
PlzNavigate.  However, that only exists in M56.

Per linked bug, we need to tighten the security check currently in M54
and M55 for apps with a "webview" permission where the problematic
blob/filesystem URL requests aren't made from the guest process.

This is the plan for fixing this:
1.  https://codereview.chromium.org/2435593007/: Reintroduce these IO
    thread checks on M56.
2.  <This CL> Tighten the checks for the scenario above using
    ChildProcessSecurityPolicy.
3.  Merge the CL from step 2 back to M55 and M54.
4.  Remove these checks from M56 and tighten the checks in
    ExtensionNavigationThrottle.

The goal of this CL is to determine if a request is being made by a
guest process on the IO thread.  It relies on the fact that
ChildProcessSecurityPolicyImpl already has the origin
"chrome-extension://<appid>" in the origin_set_ for the guest process,
thanks to the GrantOrigin that was added to
WebViewGuest::CreateWebContents in r422976 as part of fixing issue
652784.  Therefore, it's sufficient to check whether the requested
nested URL's origin is in the origin_set_ for the process making the
request.

BUG=656752
==========

[alexmos, 2016-10-20 16:42:36]

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

[alexmos, 2016-10-20 16:42:37]

Nick, can you please take a look?  This is the approach we coded up along with a
test.  This follows up on reintroducing the check to the IO thread in
https://codereview.chromium.org/2435593007/.

[ncarter (slow), 2016-10-20 17:46:51]

lgtm

3 points of discussion and 1 suggestion for debug info.

https://codereview.chromium.org/2437753003/diff/1/chrome/browser/net/chrome_e...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2437753003/diff/1/chrome/browser/net/chrome_e...
chrome/browser/net/chrome_extensions_network_delegate.cc:220:
policy->HasSpecificPermissionForOrigin(info->GetChildID(), origin);
Does correctness here depend on my fix for the too-early
IsIsolateExtensionsEnabled() call?

I think it doesn't, but interaction is that other the existing tests (which we
want to validate this change against) fail because of that bug?

https://codereview.chromium.org/2437753003/diff/1/chrome/browser/net/chrome_e...
chrome/browser/net/chrome_extensions_network_delegate.cc:224: // legitimate use
cases.
I recommend:

base::debug::Alias(&origin);
base::debug::Alias(&from_guest);

https://codereview.chromium.org/2437753003/diff/1/chrome/browser/net/chrome_e...
chrome/browser/net/chrome_extensions_network_delegate.cc:225:
base::debug::DumpWithoutCrashing();
There is some risk of ddos from really prolific DWOCs. But since this is
guaranteed to be an extension blob navigation I think it's okay. If it happens
in practice, I don't worry about a storm of them making a bad situation worse
for the user.

I'm thinking we'll need at least 24h bake time on a M55 release, to convince
ourselves that this isn't happening en mass for the non-oopif group. And maybe
we can reach out to the kiosk folks for manual vetting?

https://codereview.chromium.org/2437753003/diff/1/content/browser/child_proce...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/2437753003/diff/1/content/browser/child_proce...
content/browser/child_process_security_policy_impl.cc:947: return false;
In previous interactions with CPSP, these |return false| cases were the source
of headaches (hence the lame return true on line 936).

I'm mentioning this so that we can remember it as a possible cause, if the DWOC
happens in practice -- races between the final few IPCs before a process dies,
and ChildProcessSecurityPolicyImpl::Remove() for that process_id.

[alexmos, 2016-10-20 18:31:49]

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

[commit-bot: I haz the power, 2016-10-20 18:32:39]

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

[alexmos, 2016-10-20 18:40:23]

Thanks!

https://codereview.chromium.org/2437753003/diff/1/chrome/browser/net/chrome_e...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2437753003/diff/1/chrome/browser/net/chrome_e...
chrome/browser/net/chrome_extensions_network_delegate.cc:220:
policy->HasSpecificPermissionForOrigin(info->GetChildID(), origin);
On 2016/10/20 17:46:50, ncarter wrote:
> Does correctness here depend on my fix for the too-early
> IsIsolateExtensionsEnabled() call?
> 
> I think it doesn't, but interaction is that other the existing tests (which we
> want to validate this change against) fail because of that bug?

I think this is independent.  The affected test for that bug
(NestedURLNavigationsToExtensionBlocked) fails with
--force-fieldtrials=SiteIsolationExtensions/Control independently of this CL,
and the new test here actually passes with
--force-fieldtrials=SiteIsolationExtensions/Control.    In practice, the bug
would cause a user in the Control group to kill the renderer when it tries to
create a filesystem URL from an unblessed extension subframe, before it tries to
navigate a main frame to it (so before we get here), right?

https://codereview.chromium.org/2437753003/diff/1/chrome/browser/net/chrome_e...
chrome/browser/net/chrome_extensions_network_delegate.cc:224: // legitimate use
cases.
On 2016/10/20 17:46:50, ncarter wrote:
> I recommend:
> 
> base::debug::Alias(&origin);
> base::debug::Alias(&from_guest);

Done.  Good idea.

https://codereview.chromium.org/2437753003/diff/1/chrome/browser/net/chrome_e...
chrome/browser/net/chrome_extensions_network_delegate.cc:225:
base::debug::DumpWithoutCrashing();
On 2016/10/20 17:46:50, ncarter wrote:
> There is some risk of ddos from really prolific DWOCs. But since this is
> guaranteed to be an extension blob navigation I think it's okay. If it happens
> in practice, I don't worry about a storm of them making a bad situation worse
> for the user.
> 
> I'm thinking we'll need at least 24h bake time on a M55 release, to convince
> ourselves that this isn't happening en mass for the non-oopif group. And maybe
> we can reach out to the kiosk folks for manual vetting?

That sounds good.  We'll definitely want enough baking time on M55 to look for
these crashes.  I'll also reach out to my kiosk contacts to make sure they test
this.   If we're concerned about volume, we could just not merge the DWOC to
M54, though I agree that it's unlikely to be a problem.  But we can see what
happens in M55 and go from there.

https://codereview.chromium.org/2437753003/diff/1/content/browser/child_proce...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/2437753003/diff/1/content/browser/child_proce...
content/browser/child_process_security_policy_impl.cc:947: return false;
On 2016/10/20 17:46:50, ncarter wrote:
> In previous interactions with CPSP, these |return false| cases were the source
> of headaches (hence the lame return true on line 936).
> 
> I'm mentioning this so that we can remember it as a possible cause, if the
DWOC
> happens in practice -- races between the final few IPCs before a process dies,
> and ChildProcessSecurityPolicyImpl::Remove() for that process_id.

Acknowledged.

[ncarter (slow), 2016-10-20 19:05:56]

https://codereview.chromium.org/2437753003/diff/1/chrome/browser/net/chrome_e...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2437753003/diff/1/chrome/browser/net/chrome_e...
chrome/browser/net/chrome_extensions_network_delegate.cc:225:
base::debug::DumpWithoutCrashing();
On 2016/10/20 18:40:23, alexmos wrote:
> On 2016/10/20 17:46:50, ncarter wrote:
> > There is some risk of ddos from really prolific DWOCs. But since this is
> > guaranteed to be an extension blob navigation I think it's okay. If it
happens
> > in practice, I don't worry about a storm of them making a bad situation
worse
> > for the user.
> > 
> > I'm thinking we'll need at least 24h bake time on a M55 release, to convince
> > ourselves that this isn't happening en mass for the non-oopif group. And
maybe
> > we can reach out to the kiosk folks for manual vetting?
> 
> That sounds good.  We'll definitely want enough baking time on M55 to look for
> these crashes.  I'll also reach out to my kiosk contacts to make sure they
test
> this.   If we're concerned about volume, we could just not merge the DWOC to
> M54, though I agree that it's unlikely to be a problem.  But we can see what
> happens in M55 and go from there.

Good, we are on the same page. The DWOC should be fine in M54 -- the ifs()s
above are very specific. I made the opposite decision with the
filesystem-url-origin DWOC that's deep in ChildProcessSecurityPolicy, because it
was called from so many different contexts.

[alexmos, 2016-10-20 19:18:36]

alexmos@chromium.org changed reviewers:
+ sky@chromium.org

[alexmos, 2016-10-20 19:18:37]

sky@, can you please review this for OWNERS?  This is a follow-up to
https://codereview.chromium.org/2435593007.

[commit-bot: I haz the power, 2016-10-20 19:49:40]

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

[commit-bot: I haz the power, 2016-10-20 19:49:41]

Dry run: This issue passed the CQ dry run.

[sky, 2016-10-20 20:45:08]

sky@chromium.org changed reviewers:
+ asargent@chromium.org, mmenke@google.com

[sky, 2016-10-20 20:45:08]

Similar comment to last one: +mmenke for c/b/net and +asargent for
c/b/extensions

[mmenke, 2016-10-20 21:00:30]

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

[mmenke, 2016-10-20 21:00:31]

https://codereview.chromium.org/2437753003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2437753003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:225:
base::debug::Alias(&origin);
include base/debug/alias.h.

https://codereview.chromium.org/2437753003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:225:
base::debug::Alias(&origin);
Also, I don't think this is guaranteed to work?  You'll get the contents of
origin, but not the contents of origin's string, since it's a pointer.  The
general way to do this is to put a fixed length character array on the stack,
and copy up to the first n bytes of origin as a string to it.

[asargent_no_longer_on_chrome, 2016-10-20 22:14:25]

https://codereview.chromium.org/2437753003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2437753003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:878: //
WebViewTest.Shim_TestBlobURL.
Maybe I'm missing something, but does this test actually have anything to do
with extensions::ProcessManager? If not, it probably doesn't belong in this
file, and should instead be in another file. Maybe
c/b/e/isolated_app_browsertest.cc ? Or perhaps a new top-level file and test
class, and have that test class derive from ExtensionBrowserTest.

[alexmos, 2016-10-21 00:18:57]

https://codereview.chromium.org/2437753003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2437753003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:225:
base::debug::Alias(&origin);
On 2016/10/20 21:00:30, mmenke wrote:
> include base/debug/alias.h.

Done.

https://codereview.chromium.org/2437753003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:225:
base::debug::Alias(&origin);
On 2016/10/20 21:00:30, mmenke wrote:
> Also, I don't think this is guaranteed to work?  You'll get the contents of
> origin, but not the contents of origin's string, since it's a pointer.  The
> general way to do this is to put a fixed length character array on the stack,
> and copy up to the first n bytes of origin as a string to it.

Replaced with the fixed char array.  Thanks for pointing this out!

[alexmos, 2016-10-21 00:47:15]

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

[commit-bot: I haz the power, 2016-10-21 00:47:56]

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

[alexmos, 2016-10-21 00:49:59]

https://codereview.chromium.org/2437753003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2437753003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:878: //
WebViewTest.Shim_TestBlobURL.
On 2016/10/20 22:14:25, Antony Sargent wrote:
> Maybe I'm missing something, but does this test actually have anything to do
> with extensions::ProcessManager? If not, it probably doesn't belong in this
> file, and should instead be in another file. Maybe
> c/b/e/isolated_app_browsertest.cc ? Or perhaps a new top-level file and test
> class, and have that test class derive from ExtensionBrowserTest.

This test reuses a lot of functionality from the two tests above it for
manipulating blob/filesystem URLs (NestedURLNavigationsToExtensionBlocked,
NestedURLNavigationsToExtensionAllowed), and follows up on the bugs involved
there.  I agree that this may not be an ideal place for these tests, but I'd
prefer that we move them somewhere all at once, and in a followup CL to not
complicate the merge of this to M55/M54.  I also actually meant to add a couple
of ProcessManager checks in this new test similar to what's already in the other
tests; I forgot to do this initially but have added them now.  (These
double-check that we won't get a new extension process when we block an unsafe
blob/filesystem navigation.)

[commit-bot: I haz the power, 2016-10-21 02:27:40]

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

[commit-bot: I haz the power, 2016-10-21 02:27:40]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-10-21 15:44:21]

c/b/net LGTM

https://codereview.chromium.org/2437753003/diff/60001/chrome/browser/net/chro...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2437753003/diff/60001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:229:
sizeof(origin_copy));
Know it doesn't really matter, but would this be better as arraysize?  If this
were the UTF16 equivalent of the same method, that would be the one to use.

[alexmos, 2016-10-21 16:16:14]

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

[commit-bot: I haz the power, 2016-10-21 16:16:35]

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

[alexmos, 2016-10-21 16:16:41]

https://codereview.chromium.org/2437753003/diff/60001/chrome/browser/net/chro...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2437753003/diff/60001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:229:
sizeof(origin_copy));
On 2016/10/21 15:44:21, mmenke wrote:
> Know it doesn't really matter, but would this be better as arraysize?  If this
> were the UTF16 equivalent of the same method, that would be the one to use.

Done.  Thanks!

[asargent_no_longer_on_chrome, 2016-10-21 17:33:18]

lgtm

https://codereview.chromium.org/2437753003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2437753003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:878: //
WebViewTest.Shim_TestBlobURL.
On 2016/10/21 00:49:59, alexmos wrote:
> On 2016/10/20 22:14:25, Antony Sargent wrote:
> > Maybe I'm missing something, but does this test actually have anything to do
> > with extensions::ProcessManager? If not, it probably doesn't belong in this
> > file, and should instead be in another file. Maybe
> > c/b/e/isolated_app_browsertest.cc ? Or perhaps a new top-level file and test
> > class, and have that test class derive from ExtensionBrowserTest.
> 
> This test reuses a lot of functionality from the two tests above it for
> manipulating blob/filesystem URLs (NestedURLNavigationsToExtensionBlocked,
> NestedURLNavigationsToExtensionAllowed), and follows up on the bugs involved
> there.  I agree that this may not be an ideal place for these tests, but I'd
> prefer that we move them somewhere all at once, and in a followup CL to not
> complicate the merge of this to M55/M54.  I also actually meant to add a
couple
> of ProcessManager checks in this new test similar to what's already in the
other
> tests; I forgot to do this initially but have added them now.  (These
> double-check that we won't get a new extension process when we block an unsafe
> blob/filesystem navigation.)

Ok, sounds good.

[commit-bot: I haz the power, 2016-10-21 17:36:01]

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

[commit-bot: I haz the power, 2016-10-21 17:36:02]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-10-21 19:38:29]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-10-21 19:38:29]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2437753003/#ps80001
(title: "arraysize")

[commit-bot: I haz the power, 2016-10-21 19:39:06]

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-21 20:01:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-21 20:03:32]

Description was changed from

==========
Tighten IO thread blob/filesystem URL checks for apps with webview permission.

The IO thread blob/filesystem URL security checks were originally
introduced in r419019 and r422954.  This was merged back to M54 and
M55.

They were later moved to the UI thread (into
ExtensionNavigationThrottle) in r424937 to make them compatible with
PlzNavigate.  However, that only exists in M56.

Per linked bug, we need to tighten the security check currently in M54
and M55 for apps with a "webview" permission where the problematic
blob/filesystem URL requests aren't made from the guest process.

This is the plan for fixing this:
1.  https://codereview.chromium.org/2435593007/: Reintroduce these IO
    thread checks on M56.
2.  <This CL> Tighten the checks for the scenario above using
    ChildProcessSecurityPolicy.
3.  Merge the CL from step 2 back to M55 and M54.
4.  Remove these checks from M56 and tighten the checks in
    ExtensionNavigationThrottle.

The goal of this CL is to determine if a request is being made by a
guest process on the IO thread.  It relies on the fact that
ChildProcessSecurityPolicyImpl already has the origin
"chrome-extension://<appid>" in the origin_set_ for the guest process,
thanks to the GrantOrigin that was added to
WebViewGuest::CreateWebContents in r422976 as part of fixing issue
652784.  Therefore, it's sufficient to check whether the requested
nested URL's origin is in the origin_set_ for the process making the
request.

BUG=656752
==========

to

==========
Tighten IO thread blob/filesystem URL checks for apps with webview permission.

The IO thread blob/filesystem URL security checks were originally
introduced in r419019 and r422954.  This was merged back to M54 and
M55.

They were later moved to the UI thread (into
ExtensionNavigationThrottle) in r424937 to make them compatible with
PlzNavigate.  However, that only exists in M56.

Per linked bug, we need to tighten the security check currently in M54
and M55 for apps with a "webview" permission where the problematic
blob/filesystem URL requests aren't made from the guest process.

This is the plan for fixing this:
1.  https://codereview.chromium.org/2435593007/: Reintroduce these IO
    thread checks on M56.
2.  <This CL> Tighten the checks for the scenario above using
    ChildProcessSecurityPolicy.
3.  Merge the CL from step 2 back to M55 and M54.
4.  Remove these checks from M56 and tighten the checks in
    ExtensionNavigationThrottle.

The goal of this CL is to determine if a request is being made by a
guest process on the IO thread.  It relies on the fact that
ChildProcessSecurityPolicyImpl already has the origin
"chrome-extension://<appid>" in the origin_set_ for the guest process,
thanks to the GrantOrigin that was added to
WebViewGuest::CreateWebContents in r422976 as part of fixing issue
652784.  Therefore, it's sufficient to check whether the requested
nested URL's origin is in the origin_set_ for the process making the
request.

BUG=656752

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

[commit-bot: I haz the power, 2016-10-21 20:03:33]

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