Componentize renderer side of DNS prefetching.

Componentize renderer side of DNS prefetching.

BUG=419909
Committed: https://crrev.com/edf5a93c4b9b3a5a2b318f9c1973e9d8ff2c768a
Cr-Commit-Position: refs/heads/master@{#304529}

[gunsch, 2014-10-10 20:59:22]

gunsch@chromium.org changed reviewers:
+ blundell@chromium.org, cbentzel@chromium.org, jam@chromium.org,
palmer@chromium.org

[gunsch, 2014-10-10 20:59:24]

cbentzel: for the code/structure itself.
blundell: for new component / general componentization structure
jam: for chrome/ and ipc/ changes (just moving files and one IPC message)
palmer: for IPC security review (not a new IPC, but relocating an existing one)

PTAL, thanks!

https://codereview.chromium.org/644123002/diff/1/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/644123002/diff/1/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_message_filter.cc:67:
IPC_MESSAGE_HANDLER(DnsPredictorMsg_RequestPrefetch, OnDnsPrefetch)
I expect this change here is not quite correct, but creating a new
RenderMessageFilter in chrome/browser/renderer_host seemed excessive.
Suggestions appreciated.

https://codereview.chromium.org/644123002/diff/1/components/predictor/OWNERS
File components/predictor/OWNERS (right):

https://codereview.chromium.org/644123002/diff/1/components/predictor/OWNERS#...
components/predictor/OWNERS:2: jar@chromium.org
Please suggest the most appropriate folks for OWNERS here. I picked jar@ as
original author and cbentzel@ on the networking side, but I admit I don't know
the networking team well enough to pick this well.

[palmer, 2014-10-13 23:56:22]

https://codereview.chromium.org/644123002/diff/1/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/644123002/diff/1/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_message_filter.cc:67:
IPC_MESSAGE_HANDLER(DnsPredictorMsg_RequestPrefetch, OnDnsPrefetch)
I'm not sure I understand the issue?

https://codereview.chromium.org/644123002/diff/1/components/predictor/common/...
File components/predictor/common/predictor_messages.h (right):

https://codereview.chromium.org/644123002/diff/1/components/predictor/common/...
components/predictor/common/predictor_messages.h:21: std::vector<std::string> /*
hostnames */)
Does the receiver of this message set any hard limit on the size of the vector
and the sizes of the strings?

[blundell, 2014-10-14 06:57:47]

Will (the browser side of) this component eventually be used on iOS?

[gunsch, 2014-10-14 15:54:39]

Blundell: I don't know, does iOS currently do any prefetching at all? The
chrome/browser implementation of the browser side is much harder to refactor
out, since it's connected with proxy settings, profiles, etc. Per the linked
issue, I'm hoping to check in a simple browser-side implementation next that
Chromecast would like to use, but I'm not connected with whatever the iOS build
is doing.

https://codereview.chromium.org/644123002/diff/1/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/644123002/diff/1/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_message_filter.cc:67:
IPC_MESSAGE_HANDLER(DnsPredictorMsg_RequestPrefetch, OnDnsPrefetch)
On 2014/10/13 23:56:21, Chromium Palmer wrote:
> I'm not sure I understand the issue?

Maybe there's no issue, but it looks like other MessageFilter instances stick
with handling IPC messages from one "IPC namespace" (e.g. ChromeViewHostMsg).

https://codereview.chromium.org/644123002/diff/1/components/predictor/common/...
File components/predictor/common/predictor_messages.h (right):

https://codereview.chromium.org/644123002/diff/1/components/predictor/common/...
components/predictor/common/predictor_messages.h:21: std::vector<std::string> /*
hostnames */)
On 2014/10/13 23:56:21, Chromium Palmer wrote:
> Does the receiver of this message set any hard limit on the size of the vector
> and the sizes of the strings?

As far as I can tell, in Chrome's current implementation, the limit is only
enforced on the renderer (sender) side. (which makes me notice the IPC comment
was backwards, the IPC message is sent _to_ the browser process).

Some light codesearching seems to imply that's the general case, can you point
me to some similar receiver-side limits on IPC vectors?

[blundell, 2014-10-15 09:15:34]

component structure LGTM. The name seems too generic to me though. Should this
component be called dns_prefetcher?

https://codereview.chromium.org/644123002/diff/490001/components/predictor/re...
File components/predictor/renderer/renderer_net_predictor.cc (right):

https://codereview.chromium.org/644123002/diff/490001/components/predictor/re...
components/predictor/renderer/renderer_net_predictor.cc:133: predictor::NameList
names;
everything in this component should be in the predictor namespace.

[jam, 2014-10-15 16:14:52]

On 2014/10/10 20:59:24, gunsch wrote:
> cbentzel: for the code/structure itself.
> blundell: for new component / general componentization structure
> jam: for chrome/ and ipc/ changes (just moving files and one IPC message)

I'm too overloaded to do this, sorry. please look for other owners.

> palmer: for IPC security review (not a new IPC, but relocating an existing
one)
> 
> PTAL, thanks!
> 
>
https://codereview.chromium.org/644123002/diff/1/chrome/browser/renderer_host...
> File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):
> 
>
https://codereview.chromium.org/644123002/diff/1/chrome/browser/renderer_host...
> chrome/browser/renderer_host/chrome_render_message_filter.cc:67:
> IPC_MESSAGE_HANDLER(DnsPredictorMsg_RequestPrefetch, OnDnsPrefetch)
> I expect this change here is not quite correct, but creating a new
> RenderMessageFilter in chrome/browser/renderer_host seemed excessive.
> Suggestions appreciated.
> 
> https://codereview.chromium.org/644123002/diff/1/components/predictor/OWNERS
> File components/predictor/OWNERS (right):
> 
>
https://codereview.chromium.org/644123002/diff/1/components/predictor/OWNERS#...
> components/predictor/OWNERS:2: mailto:jar@chromium.org
> Please suggest the most appropriate folks for OWNERS here. I picked jar@ as
> original author and cbentzel@ on the networking side, but I admit I don't know
> the networking team well enough to pick this well.

[palmer, 2014-10-15 19:16:13]

> As far as I can tell, in Chrome's current implementation, the limit is only
> enforced on the renderer (sender) side. (which makes me notice the IPC comment
> was backwards, the IPC message is sent _to_ the browser process).

By definition, it's meaningless to enforce security or correctness guarantees on
the untrusted, potentially-untrustworthy side of an IPC (in our case, that's the
renderer).

http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc

> Some light codesearching seems to imply that's the general case, can you point
> me to some similar receiver-side limits on IPC vectors?

It's the norm, in IPC CLs that I review, to enforce whatever guarantee is
necessary on the browser side.

[cbentzel, 2014-10-20 16:21:31]

On 2014/10/15 19:16:13, Chromium Palmer wrote:
> > As far as I can tell, in Chrome's current implementation, the limit is only
> > enforced on the renderer (sender) side. (which makes me notice the IPC
comment
> > was backwards, the IPC message is sent _to_ the browser process).
> 
> By definition, it's meaningless to enforce security or correctness guarantees
on
> the untrusted, potentially-untrustworthy side of an IPC (in our case, that's
the
> renderer).
> 
> http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
> 
> > Some light codesearching seems to imply that's the general case, can you
point
> > me to some similar receiver-side limits on IPC vectors?
> 
> It's the norm, in IPC CLs that I review, to enforce whatever guarantee is
> necessary on the browser side.

Sorry, I was on vacation last week but didn't send out announcement.

Will take a look and possibly redirect by end of day today.

[gunsch, 2014-10-28 19:51:41]

gunsch@chromium.org changed reviewers:
+ thakis@chromium.org
- jam@chromium.org

[gunsch, 2014-10-28 19:55:31]

-jam
thakis@: can you take a look at chrome/ changes? This is largely structural
refactoring.

palmer@: established some common limits in predictor_common.h and enforcing them
browser-side, PTAL

blundell@: I had originally named this "prefetch" and then noticed all of the
files were already called "predictor" of some sort or another. Open to thoughts
from everyone else on this thread about naming: "predictor" vs "prefetch" vs
"dns_prefetch", and if the classes themselves should be renamed as well.

cbentzel@: could you take a look or reassign? Thanks.

Revisiting this CL again, got sidetracked pushing ATV out the door.

https://codereview.chromium.org/644123002/diff/490001/components/predictor/re...
File components/predictor/renderer/renderer_net_predictor.cc (right):

https://codereview.chromium.org/644123002/diff/490001/components/predictor/re...
components/predictor/renderer/renderer_net_predictor.cc:133: predictor::NameList
names;
On 2014/10/15 09:15:33, blundell wrote:
> everything in this component should be in the predictor namespace.

Done.

[Nico, 2014-10-28 20:07:20]

chrome lgtm (except for the logging). i also left a bikeshed comment on the
component's name.

make sure to get lgtms from the two people you are making owners of the new
component.

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:742: << hostnames.size();
Don't do useless logging. If this is an invariant, CHECK() for it. If it isn't,
just return.

As is, this will produce log spam for users if it fails without us ever doing
anything about it since "just fixing some logspam" won't be considered high
priority.

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:754: << it->length();
likewise

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
File chrome/browser/net/predictor.h (right):

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
chrome/browser/net/predictor.h:36: #include
"components/predictor/common/predictor_common.h"
is "predictor" the best name if this is just in components/? will dns be the
only thing a component will predict?

[gunsch, 2014-10-28 20:46:01]

gunsch@chromium.org changed reviewers:
+ jar@chromium.org, nasko@chromium.org

[gunsch, 2014-10-28 20:47:32]

+jar: original author of the prefetch code, tentatively added as OWNERS of the
new component. Let me know if that's okay or who would be a better person to
take ownership of it.

+nasko: please review for content/ OWNERS. there's a "new dependency" on
content/public/renderer due to separating out the renderer-specific dns_prefetch
code, but no functionality change.

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:742: << hostnames.size();
If it fails, that's a security issue, since this should never happen in practice
(and is enforced on the renderer side). But per palmer's link
(http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc):

"When validating untrustworthy input, don't simply use CHECK. We do not want the
input validation mechanism to become an easy way for a malicious renderer to
kill the browser process. It's usually better to ignore the bad input.".

So I don't think this is actually "logspam", nor do I think a CHECK is
appropriate. I can upgrade to a DFATAL to enforce the "invariant" aspect,
however.

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
File chrome/browser/net/predictor.h (right):

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
chrome/browser/net/predictor.h:36: #include
"components/predictor/common/predictor_common.h"
Sounds like a +1 to blundell's idea, so renaming the component "dns_prefetch"

[Nico, 2014-10-28 20:56:42]

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:742: << hostnames.size();
On 2014/10/28 20:47:32, gunsch wrote:
> If it fails, that's a security issue, since this should never happen in
practice
> (and is enforced on the renderer side). But per palmer's link
>
(http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc):
> 
> "When validating untrustworthy input, don't simply use CHECK. We do not want
the
> input validation mechanism to become an easy way for a malicious renderer to
> kill the browser process. It's usually better to ignore the bad input.".
> 
> So I don't think this is actually "logspam", nor do I think a CHECK is
> appropriate. I can upgrade to a DFATAL to enforce the "invariant" aspect,
> however.

It's logspam in the sense that printing this doesn't help either the user, nor
us. If this can happen with a compromised renderer, the validation should be
done in the ipc layer which has code for killing renderers that send invalid
values.

[gunsch, 2014-10-28 21:11:30]

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:742: << hostnames.size();
That sounds practical, can you point me to an example or docs on the subject? I
haven't heard of this before, and can't find information on this in the public
docs nor examples in similar IPC messages.

[Nico, 2014-10-28 21:31:18]

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:742: << hostnames.size();
On 2014/10/28 21:11:29, gunsch wrote:
> That sounds practical, can you point me to an example or docs on the subject?
I
> haven't heard of this before, and can't find information on this in the public
> docs nor examples in similar IPC messages.

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

This is called from
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_pr...
, and this is what calls set_dispatch_error() on an ipc::Message:
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_message_ma...

This in turn is called if the Read() call in Dispatch() from the respective
message class fails (e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_message_ma...
)

You configure this via ipc traits, for example to set the max value on an enum:
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

Clicking "ParamTraits" on
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_param_trai...
and looking at the "references" list that cs opens at the bottom should show you
a bunch of examples.

[jar (doing other things), 2014-10-28 22:49:32]

https://codereview.chromium.org/644123002/diff/570001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/644123002/diff/570001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:741: LOG(DFATAL) << "Refusing to prefetch too
many hostnames: "
nit: Please use DLOG.  No one will look at this... so there is no reason to
swell the binary.

It is also plausible that you should "fix" the data, by truncating any overlong
list, rather than aborting all progress.

https://codereview.chromium.org/644123002/diff/570001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:753: LOG(DFATAL) << "Refusing to prefetch
hostname with length: "
nit: DLOG

[Nico, 2014-10-29 22:16:35]

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:742: << hostnames.size();
Even better, we even have a web page for this:
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc

[gunsch, 2014-10-29 23:58:29]

PTAL: this now includes custom validation when deserializing at the IPC layer.

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:742: << hostnames.size();
Yeah, I linked that earlier in this reply thread, it doesn't have any
information about doing custom IPC validation that aren't already covered by the
existing macros. I've taken a stab at making my own ParamTraits here, let me
know what you think.

https://codereview.chromium.org/644123002/diff/530001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:754: << it->length();
On 2014/10/28 20:07:19, Nico wrote:
> likewise

Done.

https://codereview.chromium.org/644123002/diff/570001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/644123002/diff/570001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:741: LOG(DFATAL) << "Refusing to prefetch too
many hostnames: "
Covered by removing this and doing validation with a custom IpcParamTraits.

https://codereview.chromium.org/644123002/diff/570001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:753: LOG(DFATAL) << "Refusing to prefetch
hostname with length: "
ditto

[Nico, 2014-10-30 00:03:05]

lgtm, thanks. Might want to have a security person (palmer, tsepez, …) have a
look at the new ipc trait too, they're experts there.

https://codereview.chromium.org/644123002/diff/590001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/644123002/diff/590001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:744: }
(i'd remove the checks in here that are covered by ipc deserialization checking
now)

https://codereview.chromium.org/644123002/diff/590001/components/dns_prefetch...
File components/dns_prefetch/common/prefetch_messages.cc (right):

https://codereview.chromium.org/644123002/diff/590001/components/dns_prefetch...
components/dns_prefetch/common/prefetch_messages.cc:27: for
(dns_prefetch::NameList::const_iterator it = hostnames.begin();
if you want, you can write

  for (const auot& hostname : hostnames) {
    if (hostname.length() > ...
  }

[gunsch, 2014-10-30 00:32:50]

thakis@: palmer@ is on this review, will definitely wait for his approval.
Thanks for looking.

https://codereview.chromium.org/644123002/diff/590001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/644123002/diff/590001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:744: }
On 2014/10/30 00:03:05, Nico wrote:
> (i'd remove the checks in here that are covered by ipc deserialization
checking
> now)

good catch, forgot to upload these changes

https://codereview.chromium.org/644123002/diff/590001/components/dns_prefetch...
File components/dns_prefetch/common/prefetch_messages.cc (right):

https://codereview.chromium.org/644123002/diff/590001/components/dns_prefetch...
components/dns_prefetch/common/prefetch_messages.cc:27: for
(dns_prefetch::NameList::const_iterator it = hostnames.begin();
On 2014/10/30 00:03:05, Nico wrote:
> if you want, you can write
> 
>   for (const auot& hostname : hostnames) {
>     if (hostname.length() > ...
>   }

Done.

[gunsch, 2014-10-30 17:07:49]

gunsch@chromium.org changed required reviewers:
+ blundell@chromium.org, cbentzel@chromium.org, jar@chromium.org,
nasko@chromium.org, palmer@chromium.org, thakis@chromium.org

[jar (doing other things), 2014-10-30 17:50:30]

Patch set 9 LGTM

[gunsch, 2014-10-31 17:02:20]

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

[gunsch, 2014-10-31 17:02:21]

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

[gunsch, 2014-10-31 17:02:59]

ping?

-nasko (OOO)
+creis: please review for content/ OWNERS. there's a "new dependency" on
content/public/renderer due to separating out the renderer-specific dns_prefetch
code, but no functionality change.

[Charlie Reis, 2014-10-31 17:37:20]

On 2014/10/31 17:02:59, gunsch wrote:
> ping?
> 
> -nasko (OOO)
> +creis: please review for content/ OWNERS. there's a "new dependency" on
> content/public/renderer due to separating out the renderer-specific
dns_prefetch
> code, but no functionality change.

I don't see any issue with depending on content/public/renderer from
components/dns_prefetch/renderer.  LGTM.

[palmer, 2014-10-31 18:46:23]

LGTM. Something like net::Hostname can and should come in a future CL.

https://codereview.chromium.org/644123002/diff/630001/components/dns_prefetch...
File components/dns_prefetch/common/prefetch_messages.cc (right):

https://codereview.chromium.org/644123002/diff/630001/components/dns_prefetch...
components/dns_prefetch/common/prefetch_messages.cc:28: if (hostname.length() >
dns_prefetch::kMaxDnsHostnameLength)
There's more to a hostname's validity than its length. Max length, max length
per label, max # of labels, valid characters.

Ultimately, this should be enforced by the type system: a class Hostname whose
constructor enforces the RFC.

[cbentzel, 2014-11-04 17:45:19]

https://codereview.chromium.org/644123002/diff/630001/components/OWNERS
File components/OWNERS (right):

https://codereview.chromium.org/644123002/diff/630001/components/OWNERS#newco...
components/OWNERS:47: per-file dns_prefetch*=cbentzel@chromium.org
I'm probably not a good reviewer for this. Would prefer ttuttle as he also does
DNS work in Chrome.

[gunsch, 2014-11-04 17:50:03]

gunsch@chromium.org changed reviewers:
+ ttuttle@chromium.org
- cbentzel@chromium.org

[gunsch, 2014-11-04 17:50:03]

gunsch@chromium.org changed required reviewers:
+ ttuttle@chromium.org
- cbentzel@chromium.org

[gunsch, 2014-11-04 17:50:34]

On 2014/11/04 17:45:19, cbentzel wrote:
> https://codereview.chromium.org/644123002/diff/630001/components/OWNERS
> File components/OWNERS (right):
> 
>
https://codereview.chromium.org/644123002/diff/630001/components/OWNERS#newco...
> components/OWNERS:47: per-file mailto:dns_prefetch*=cbentzel@chromium.org
> I'm probably not a good reviewer for this. Would prefer ttuttle as he also
does
> DNS work in Chrome.

Thanks. ttuttle@: can you take a look?

[wzhong, 2014-11-06 00:08:06]

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

[wzhong, 2014-11-06 00:08:08]

https://codereview.chromium.org/644123002/diff/650001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/644123002/diff/650001/chrome/browser/DEPS#new...
chrome/browser/DEPS:57: "+components/dns_prefetch",
Why is it not in alphabetical?

[Deprecated (see juliatuttle), 2014-11-12 18:24:13]

lgtm

[gunsch, 2014-11-17 20:01:49]

gunsch@chromium.org changed reviewers:
+ tsepez@chromium.org

[gunsch, 2014-11-17 20:01:49]

gunsch@chromium.org changed required reviewers:
+ tsepez@chromium.org

[gunsch, 2014-11-17 20:03:00]

Ah, looks like I still need one more approval.

tsepez@: components/dns_prefetch/common/DEPS adds a dependency on "ipc/"
directory, since the newly-split-out component includes IPC messages. Can you
take a look?

[Tom Sepez, 2014-11-17 20:05:52]

LGTM.

[gunsch, 2014-11-17 23:46:30]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-11-17 23:48:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/644123002/710001

[commit-bot: I haz the power, 2014-11-18 00:52:17]

Committed patchset #13 (id:710001)

[commit-bot: I haz the power, 2014-11-18 00:53:06]

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