Initial implementation of stashed message ports, content side

Initial implementation of stashed message ports, content side

This is part of a series of patches to implement an
initial version of a mechanism for a MessagePort to
outlive a service worker as described in
https://github.com/mkruisselbrink/navigator-connect/blob/gh-pages/docs/api-idea.md#persisted-messageports

[1/3] https://codereview.chromium.org/1008533005/, blink side implementation
[2/3] this patch, content side implementation
[3/3] https://codereview.chromium.org/1063533002/, layout tests

BUG=426458, 483108
Committed: https://crrev.com/9e86f80ad8421b1a34894ea72fb8af4ebe88a0c2
Cr-Commit-Position: refs/heads/master@{#330766}

[Marijn Kruisselbrink, 2015-04-30 21:53:51]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2015-04-30 22:42:54]

Patchset #1 (id:20001) has been deleted

[Marijn Kruisselbrink, 2015-04-30 23:05:52]

mek@chromium.org changed reviewers:
+ michaeln@chromium.org

[Marijn Kruisselbrink, 2015-05-05 21:47:17]

mek@chromium.org changed reviewers:
+ scheib@chromium.org
- michaeln@chromium.org

[scheib, 2015-05-06 22:19:46]

https://codereview.chromium.org/1110103003/diff/40001/content/browser/message...
File content/browser/message_port_service.cc (right):

https://codereview.chromium.org/1110103003/diff/40001/content/browser/message...
content/browser/message_port_service.cc:82: NOTREACHED();
Handle it or not, but not both. Looks like here we can't trust renderer so it's
handle it.
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...

https://codereview.chromium.org/1110103003/diff/40001/content/browser/service...
File content/browser/service_worker/stashed_port_manager.cc (right):

https://codereview.chromium.org/1110103003/diff/40001/content/browser/service...
content/browser/service_worker/stashed_port_manager.cc:14: struct
StashedPortManager::StashedPort {
This can be wrapped with anon namespace I think, if so please do.

https://codereview.chromium.org/1110103003/diff/40001/content/browser/service...
content/browser/service_worker/stashed_port_manager.cc:130:
base::Bind(&StashedPortManager::TransferMessagePort, this,
On first read at least it's not obvious why it's transfered when it is held, a
explanatory comment would improve readability.

https://codereview.chromium.org/1110103003/diff/40001/content/browser/service...
content/browser/service_worker/stashed_port_manager.cc:201: void
StashedPortManager::TransferredPorts(
OnTransferredPorts?

https://codereview.chromium.org/1110103003/diff/40001/content/browser/service...
File content/browser/service_worker/stashed_port_manager.h (right):

https://codereview.chromium.org/1110103003/diff/40001/content/browser/service...
content/browser/service_worker/stashed_port_manager.h:18: // This class keeps
track of all stashed message port. Stashed message ports are
port -> ports

https://codereview.chromium.org/1110103003/diff/40001/content/browser/service...
content/browser/service_worker/stashed_port_manager.h:71: void
TransferMessagePort(int message_port_id,
Doc what this does too.

[Marijn Kruisselbrink, 2015-05-12 06:56:50]

Patchset #2 (id:60001) has been deleted

[Marijn Kruisselbrink, 2015-05-12 06:57:33]

https://codereview.chromium.org/1110103003/diff/40001/content/browser/message...
File content/browser/message_port_service.cc (right):

https://codereview.chromium.org/1110103003/diff/40001/content/browser/message...
content/browser/message_port_service.cc:82: NOTREACHED();
On 2015/05/06 22:19:45, scheib wrote:
> Handle it or not, but not both. Looks like here we can't trust renderer so
it's
> handle it.
>
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...

I would generally agree with you, but this exact same pattern is used in pretty
much every other MessagePortService method that takes a message_port_id, so I
think consistency here is more important.

https://codereview.chromium.org/1110103003/diff/40001/content/browser/service...
File content/browser/service_worker/stashed_port_manager.cc (right):

https://codereview.chromium.org/1110103003/diff/40001/content/browser/service...
content/browser/service_worker/stashed_port_manager.cc:14: struct
StashedPortManager::StashedPort {
On 2015/05/06 22:19:46, scheib wrote:
> This can be wrapped with anon namespace I think, if so please do.

I don't think it can (and it makes more sense as a private member of
StashedPortManager anyway)

https://codereview.chromium.org/1110103003/diff/40001/content/browser/service...
content/browser/service_worker/stashed_port_manager.cc:130:
base::Bind(&StashedPortManager::TransferMessagePort, this,
On 2015/05/06 22:19:46, scheib wrote:
> On first read at least it's not obvious why it's transfered when it is held, a
> explanatory comment would improve readability.

I tried to explain this a bit better

https://codereview.chromium.org/1110103003/diff/40001/content/browser/service...
content/browser/service_worker/stashed_port_manager.cc:201: void
StashedPortManager::TransferredPorts(
On 2015/05/06 22:19:45, scheib wrote:
> OnTransferredPorts?

Done.

https://codereview.chromium.org/1110103003/diff/40001/content/browser/service...
File content/browser/service_worker/stashed_port_manager.h (right):

https://codereview.chromium.org/1110103003/diff/40001/content/browser/service...
content/browser/service_worker/stashed_port_manager.h:18: // This class keeps
track of all stashed message port. Stashed message ports are
On 2015/05/06 22:19:46, scheib wrote:
> port -> ports

Done.

https://codereview.chromium.org/1110103003/diff/40001/content/browser/service...
content/browser/service_worker/stashed_port_manager.h:71: void
TransferMessagePort(int message_port_id,
On 2015/05/06 22:19:46, scheib wrote:
> Doc what this does too.

Done.

[scheib, 2015-05-12 18:17:05]

LGTM

[Marijn Kruisselbrink, 2015-05-12 23:48:13]

mek@chromium.org changed reviewers:
+ falken@chromium.org

[Marijn Kruisselbrink, 2015-05-12 23:48:13]

+falken for serviceworker OWNERS

[falken, 2015-05-15 06:08:01]

falken@chromium.org changed reviewers:
+ nhiroki@chromium.org

[falken, 2015-05-15 06:08:02]

lgtm, though as nhiroki reviewed the blink-side patch he might want to take a
look at chromium-side

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:378: const
scoped_refptr<ServiceWorkerVersion> service_worker,
Can this be const&? And stashed_port_manager too?

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
File content/browser/service_worker/stashed_port_manager.h (right):

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
content/browser/service_worker/stashed_port_manager.h:95:
std::set<ServiceWorkerVersion*> observing_service_workers_;
super nit: observing_service_workers_ sounds to me like the SW are observing
this. How about observed_service_workers?

https://codereview.chromium.org/1110103003/diff/80001/content/public/browser/...
File content/public/browser/message_port_delegate.h (right):

https://codereview.chromium.org/1110103003/diff/80001/content/public/browser/...
content/public/browser/message_port_delegate.h:38: virtual void
MessageWasHeld(int route_id){};
should have a space before {}?

[kinuko, 2015-05-15 06:59:17]

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

[kinuko, 2015-05-15 06:59:18]

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
File content/browser/service_worker/stashed_port_manager.cc (right):

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
content/browser/service_worker/stashed_port_manager.cc:161:
observing_service_workers_.erase(service_worker);
Is it guaranteed that OnRunningStateChanged is always fired before
ServiceWorkerVersion goes away?

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
File content/browser/service_worker/stashed_port_manager.h (right):

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
content/browser/service_worker/stashed_port_manager.h:32: // TODO(mek): Make
sure transfering a stashed port unregisters it from here.
nit: We seem to have really lots of TODOs. Can we have at least one issue
(483108?) associated to them?

[nhiroki, 2015-05-18 04:45:57]

LGTM provided kinuko@'s comment on "OnRunningStateChanged" is resolved.

https://codereview.chromium.org/1110103003/diff/80001/content/browser/message...
File content/browser/message_port_service.cc (right):

https://codereview.chromium.org/1110103003/diff/80001/content/browser/message...
content/browser/message_port_service.cc:84: }
Can you replace this if-block with
DCHECK(message_ports_.count(message_port_id))? The coding standard disallows
such NOTREACHED() usage.

http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

https://codereview.chromium.org/1110103003/diff/80001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_context_client.h (right):

https://codereview.chromium.org/1110103003/diff/80001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_context_client.h:130: const
blink::WebString& name) override;
nit: You don't have to add "virtual" if you use "override"

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance

[Marijn Kruisselbrink, 2015-05-18 22:19:36]

https://codereview.chromium.org/1110103003/diff/80001/content/browser/message...
File content/browser/message_port_service.cc (right):

https://codereview.chromium.org/1110103003/diff/80001/content/browser/message...
content/browser/message_port_service.cc:84: }
On 2015/05/18 at 04:45:57, nhiroki wrote:
> Can you replace this if-block with
DCHECK(message_ports_.count(message_port_id))? The coding standard disallows
such NOTREACHED() usage.
> 
>
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

I'm a bit on the edge on this one. On one hand of course I shouldn't be adding
new usages of a no longer allowed pattern, but on the other hand it would be odd
for one method out of all the ones in this class to be different for these
checks. I could of course "fix" all the other occurrences of this pattern in
this file as well, but I'd be hesitant to do that without some very careful
analysis that the resulting code wouldn't introduce security bugs by allowing
renderers to somehow mess up memory in the browser process.

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:378: const
scoped_refptr<ServiceWorkerVersion> service_worker,
On 2015/05/15 at 06:08:02, falken wrote:
> Can this be const&? And stashed_port_manager too?

Of course it can, done.

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
File content/browser/service_worker/stashed_port_manager.cc (right):

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
content/browser/service_worker/stashed_port_manager.cc:161:
observing_service_workers_.erase(service_worker);
On 2015/05/15 at 06:59:18, kinuko wrote:
> Is it guaranteed that OnRunningStateChanged is always fired before
ServiceWorkerVersion goes away?

Actually I'm not sure. I misread a comment in ~ServiceWorkerVersion
"EmbeddedWorker's dtor sends StopWorker if it's still running." to indicate that
this would always be the case, but looking at this more carefully the comment
doesn't mean what I thought it meant.
I'm not sure under what circumstances a ServiceWorkerVersion instance for a
still running service worker would get deleted though, but if that happens it
seems OnRunningStateChanged won't get fired (and there doesn't seem to be any
other way to be notified of something like that happening either).

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
File content/browser/service_worker/stashed_port_manager.h (right):

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
content/browser/service_worker/stashed_port_manager.h:32: // TODO(mek): Make
sure transfering a stashed port unregisters it from here.
On 2015/05/15 at 06:59:18, kinuko wrote:
> nit: We seem to have really lots of TODOs. Can we have at least one issue
(483108?) associated to them?

I should probably file bugs for most of these TODOs, yes. I'm not sure if there
is much value in somehow referring to a generic tracking bug here though...

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
content/browser/service_worker/stashed_port_manager.h:95:
std::set<ServiceWorkerVersion*> observing_service_workers_;
On 2015/05/15 at 06:08:02, falken wrote:
> super nit: observing_service_workers_ sounds to me like the SW are observing
this. How about observed_service_workers?

thanks, much clearer indeed

https://codereview.chromium.org/1110103003/diff/80001/content/public/browser/...
File content/public/browser/message_port_delegate.h (right):

https://codereview.chromium.org/1110103003/diff/80001/content/public/browser/...
content/public/browser/message_port_delegate.h:38: virtual void
MessageWasHeld(int route_id){};
On 2015/05/15 at 06:08:02, falken wrote:
> should have a space before {}?

Ah yes, seems to be a bug in git cl format/clang-format (it removes the space),
but the style guide is pretty clear that the space should be there.

https://codereview.chromium.org/1110103003/diff/80001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_context_client.h (right):

https://codereview.chromium.org/1110103003/diff/80001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_context_client.h:130: const
blink::WebString& name) override;
On 2015/05/18 at 04:45:57, nhiroki wrote:
> nit: You don't have to add "virtual" if you use "override"
> 
> https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance

Yes, of course. Although I should probably remove the override and stick to just
virtual for now, until after the blink repo merge (when presumably virtual will
be replaced by override in all these blink/content API layers).

[kinuko, 2015-05-19 09:01:46]

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
File content/browser/service_worker/stashed_port_manager.cc (right):

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
content/browser/service_worker/stashed_port_manager.cc:34: }
nit: please have one empty line between methods

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
content/browser/service_worker/stashed_port_manager.cc:161:
observing_service_workers_.erase(service_worker);
On 2015/05/18 22:19:36, Marijn Kruisselbrink wrote:
> On 2015/05/15 at 06:59:18, kinuko wrote:
> > Is it guaranteed that OnRunningStateChanged is always fired before
> ServiceWorkerVersion goes away?
> 
> Actually I'm not sure. I misread a comment in ~ServiceWorkerVersion
> "EmbeddedWorker's dtor sends StopWorker if it's still running." to indicate
that
> this would always be the case, but looking at this more carefully the comment
> doesn't mean what I thought it meant.
> I'm not sure under what circumstances a ServiceWorkerVersion instance for a
> still running service worker would get deleted though, but if that happens it
> seems OnRunningStateChanged won't get fired (and there doesn't seem to be any
> other way to be notified of something like that happening either).

My guess is it's not guaranteed, but falken@ would probably know better.

I think we can change the current code to make it guaranteed though, it looks
easy to do so.

https://codereview.chromium.org/1110103003/diff/80001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_context_client.h (right):

https://codereview.chromium.org/1110103003/diff/80001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_context_client.h:130: const
blink::WebString& name) override;
On 2015/05/18 22:19:36, Marijn Kruisselbrink wrote:
> On 2015/05/18 at 04:45:57, nhiroki wrote:
> > nit: You don't have to add "virtual" if you use "override"
> > 
> > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance
> 
> Yes, of course. Although I should probably remove the override and stick to
just
> virtual for now, until after the blink repo merge (when presumably virtual
will
> be replaced by override in all these blink/content API layers).

Let's not add override until after the blink repo merge, having override at the
repo boundary tends to make things a bit difficult.

https://codereview.chromium.org/1110103003/diff/100001/content/public/browser...
File content/public/browser/message_port_delegate.h (right):

https://codereview.chromium.org/1110103003/diff/100001/content/public/browser...
content/public/browser/message_port_delegate.h:38: virtual void
MessageWasHeld(int route_id) {};
nit: no need of trailing ';'

[Marijn Kruisselbrink, 2015-05-19 17:40:19]

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
File content/browser/service_worker/stashed_port_manager.cc (right):

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
content/browser/service_worker/stashed_port_manager.cc:34: }
On 2015/05/19 09:01:45, kinuko wrote:
> nit: please have one empty line between methods

Done.

https://codereview.chromium.org/1110103003/diff/80001/content/browser/service...
content/browser/service_worker/stashed_port_manager.cc:161:
observing_service_workers_.erase(service_worker);
On 2015/05/19 09:01:45, kinuko wrote:
> On 2015/05/18 22:19:36, Marijn Kruisselbrink wrote:
> > On 2015/05/15 at 06:59:18, kinuko wrote:
> > > Is it guaranteed that OnRunningStateChanged is always fired before
> > ServiceWorkerVersion goes away?
> > 
> > Actually I'm not sure. I misread a comment in ~ServiceWorkerVersion
> > "EmbeddedWorker's dtor sends StopWorker if it's still running." to indicate
> that
> > this would always be the case, but looking at this more carefully the
comment
> > doesn't mean what I thought it meant.
> > I'm not sure under what circumstances a ServiceWorkerVersion instance for a
> > still running service worker would get deleted though, but if that happens
it
> > seems OnRunningStateChanged won't get fired (and there doesn't seem to be
any
> > other way to be notified of something like that happening either).
> 
> My guess is it's not guaranteed, but falken@ would probably know better.
That's also my conclusion, yes.

> I think we can change the current code to make it guaranteed though, it looks
> easy to do so.
Do you want me to file a bug for that?

https://codereview.chromium.org/1110103003/diff/100001/content/public/browser...
File content/public/browser/message_port_delegate.h (right):

https://codereview.chromium.org/1110103003/diff/100001/content/public/browser...
content/public/browser/message_port_delegate.h:38: virtual void
MessageWasHeld(int route_id) {};
On 2015/05/19 09:01:45, kinuko wrote:
> nit: no need of trailing ';'

Done.

[Marijn Kruisselbrink, 2015-05-19 21:00:16]

mek@chromium.org changed reviewers:
+ avi@chromium.org, tsepez@chromium.org

[Marijn Kruisselbrink, 2015-05-19 21:00:17]

+avi for content/ OWNERS (in particular the non service_worker bits)
+tsepez for IPC OWNERS

[Tom Sepez, 2015-05-19 21:46:52]

Messages themselves are fine, but my concern is whether a compromised service
worker (akin to a compromised renderer) could obtain a message port that doesn't
belong to it by lying about routing ids or other ids passed in the messages. 
Thanks!

[Marijn Kruisselbrink, 2015-05-19 22:01:41]

On 2015/05/19 at 21:46:52, tsepez wrote:
> Messages themselves are fine, but my concern is whether a compromised service
worker (akin to a compromised renderer) could obtain a message port that doesn't
belong to it by lying about routing ids or other ids passed in the messages. 
Thanks!

Not a great answer, but I'm pretty sure any renderer can already obtain any
message port by transferring it to itself. Nothing stops a renderer from setting
up a channel to itself, and then forging a MessagePortHostMsg_PostMessage IPC
that sends a message that transfers arbitrary message port ids to itself.
The new ServiceWorkerHostMsg_StashMessagePort does make it a bit easier of
course, but it doesn't really change what is possible. WorkerMsg_Connect seems
to be another IPC that makes it fairly easy to claim any message port.
MessagePortService does actually more or less know what renderer a port is
currently associated with, so it certainly should be possible to add more checks
in various places to make sure a renderer can't claim unrelated ports.

[Tom Sepez, 2015-05-19 22:12:05]

On 2015/05/19 22:01:41, Marijn Kruisselbrink wrote:
> On 2015/05/19 at 21:46:52, tsepez wrote:
> > Messages themselves are fine, but my concern is whether a compromised
service
> worker (akin to a compromised renderer) could obtain a message port that
doesn't
> belong to it by lying about routing ids or other ids passed in the messages. 
> Thanks!
> 
> Not a great answer, but I'm pretty sure any renderer can already obtain any
> message port by transferring it to itself. Nothing stops a renderer from
setting
> up a channel to itself, and then forging a MessagePortHostMsg_PostMessage IPC
> that sends a message that transfers arbitrary message port ids to itself.
> The new ServiceWorkerHostMsg_StashMessagePort does make it a bit easier of
> course, but it doesn't really change what is possible. WorkerMsg_Connect seems
> to be another IPC that makes it fairly easy to claim any message port.
> MessagePortService does actually more or less know what renderer a port is
> currently associated with, so it certainly should be possible to add more
checks
> in various places to make sure a renderer can't claim unrelated ports.

OK, LGTM for now but please file a follow-up bug to tighten this up.

[Avi (use Gerrit), 2015-05-20 03:02:10]

LGTM with DCHECK nits.

Lots of your DCHECKs, if they failed, would log no relevant info other than
"nope, you crashed". A better choice of DCHECKs would give more useful info.

https://codereview.chromium.org/1110103003/diff/120001/content/browser/servic...
File content/browser/service_worker/stashed_port_manager.cc (right):

https://codereview.chromium.org/1110103003/diff/120001/content/browser/servic...
content/browser/service_worker/stashed_port_manager.cc:118:
DCHECK(port.service_worker && port.route_id != MSG_ROUTING_NONE);
break into a DCHECK and DCHECK_NE

https://codereview.chromium.org/1110103003/diff/120001/content/browser/servic...
content/browser/service_worker/stashed_port_manager.cc:230: DCHECK(ports.size()
== route_ids.size());
DCHECK_EQ

https://codereview.chromium.org/1110103003/diff/120001/content/renderer/servi...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/1110103003/diff/120001/content/renderer/servi...
content/renderer/service_worker/service_worker_script_context.cc:61:
DCHECK(channel->message_port_id() >= 0);
DCHECK_GE

[kinuko, 2015-05-20 14:34:32]

On 2015/05/19 17:40:19, Marijn Kruisselbrink wrote:
> > > > Is it guaranteed that OnRunningStateChanged is always fired before
> > > ServiceWorkerVersion goes away?
> > > 
> > > Actually I'm not sure. I misread a comment in ~ServiceWorkerVersion
> > > "EmbeddedWorker's dtor sends StopWorker if it's still running." to
indicate
> > that
> > > this would always be the case, but looking at this more carefully the
> comment
> > > doesn't mean what I thought it meant.
> > > I'm not sure under what circumstances a ServiceWorkerVersion instance for
a
> > > still running service worker would get deleted though, but if that happens
> it
> > > seems OnRunningStateChanged won't get fired (and there doesn't seem to be
> any
> > > other way to be notified of something like that happening either).
> > 
> > My guess is it's not guaranteed, but falken@ would probably know better.
> That's also my conclusion, yes.
> 
> > I think we can change the current code to make it guaranteed though, it
looks
> > easy to do so.
> Do you want me to file a bug for that?

Filed: crbug.com/490125

[Marijn Kruisselbrink, 2015-05-20 17:09:15]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2015-05-20 17:09:15]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
scheib@chromium.org, nhiroki@chromium.org, avi@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1110103003/#ps140001
(title: "nicer dchecks")

[commit-bot: I haz the power, 2015-05-20 17:09:26]

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

[commit-bot: I haz the power, 2015-05-20 17:51:52]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-05-20 17:52:29]

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