[mojo] Add CreateSharedBuffer method to Broker.

[mojo] Add CreateSharedBuffer method to Broker.

BUG=556258
Committed: https://crrev.com/565052abf18c8c6cad9a960831fc01f9d6b5b98a
Cr-Commit-Position: refs/heads/master@{#369631}

[Eliot Courtney, 2016-01-05 03:17:13]

edcourtney@chromium.org changed reviewers:
+ amistry@chromium.org, jam@chromium.org, sammc@chromium.org

[Eliot Courtney, 2016-01-05 04:45:47]

I've made a few comments on my own code with some questions I had.

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
File mojo/edk/system/child_broker.cc (right):

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:45: ssize_t result =
PlatformChannelRecvmsg(parent_sync_channel_.get(), buf, 1,
Does this need to be queried in a loop since it won't block? It seemed to me
that the Windows version of this code (ReadFile...) doesn't block in this case,
so I think it might have the same problem if this is a problem.

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:389: ssize_t bytes_written =
PlatformChannelWrite(
This loop is on top of a non-blocking function (PlatformChannelWrite), which
could eat CPU; Is it worth somehow making this block?

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:404:
PlatformChannelRecvmsg(parent_sync_channel_.get(), buf, 1, handles);
Same comment as above.

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
File mojo/edk/system/child_broker_host.cc (right):

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker_host.cc:78: ssize_t result =
PlatformChannelSendmsgWithHandles(sync_channel_.get(), &iov,
Should this be done in a loop to make sure it sends?

[jam, 2016-01-06 02:58:16]

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/broker_...
File mojo/edk/system/broker_messages.h (right):

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/broker_...
mojo/edk/system/broker_messages.h:23: enum WindowsSandboxMessages {
nit: lets share the enum class type and just have ifdef around the IDs. that
would simplify things a little (i.e. BrokerMessage can use the same type)

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
File mojo/edk/system/child_broker.cc (right):

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:45: ssize_t result =
PlatformChannelRecvmsg(parent_sync_channel_.get(), buf, 1,
On 2016/01/05 04:45:47, Eliot Courtney wrote:
> Does this need to be queried in a loop since it won't block? It seemed to me
> that the Windows version of this code (ReadFile...) doesn't block in this
case,
> so I think it might have the same problem if this is a problem.

ReadFile below specifies the number of bytes to read ("sizeof(parent_handle)")
and no overlapped struct (the NULL parameter), so it's synchronous.

I'm not an expert on posix by any means, but can't you simulate the above by
temporarily calling
fcntl(parent_sync_channel_.get(), F_SETFL, 0)
on the pipe?

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:119: CreateSharedBufferNoLock(num_bytes);
no need to have two methods for this. you can inline that second method here.
CreatePlatformChannelPair is an exception because it is called initially with
the lock already acquired.

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:329: 
nit: no need to change

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:389: ssize_t bytes_written =
PlatformChannelWrite(
On 2016/01/05 04:45:47, Eliot Courtney wrote:
> This loop is on top of a non-blocking function (PlatformChannelWrite), which
> could eat CPU; Is it worth somehow making this block? 

i don't see how it would ever block in practice, because we only write tiny
messages one at a time?

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:401: while (1) {
same as first comment in this file, why not make reads on this pipe always be
blocking?

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
File mojo/edk/system/child_broker_host.h (left):

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker_host.h:69: 
nit: undo

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
File mojo/edk/system/child_broker_host.h (right):

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker_host.h:32: , public base::MessageLoopForIO::Watcher
{
nit: you can now move the comma to line 28

[Anand Mistry (off Chromium), 2016-01-06 06:43:56]

Before I review this (since my comments will be affected by the answer), do we
need the same functionality on Windows? Are sandboxed processes on windows able
to create shared memory?

[Eliot Courtney, 2016-01-06 23:56:47]

On 2016/01/06 06:43:56, Anand Mistry wrote:
> Before I review this (since my comments will be affected by the answer), do we
> need the same functionality on Windows? Are sandboxed processes on windows
able
> to create shared memory?

I think they are able to. See
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/chil...
.

[jam, 2016-01-07 00:32:38]

On 2016/01/06 06:43:56, Anand Mistry wrote:
> Before I review this (since my comments will be affected by the answer), do we
> need the same functionality on Windows? Are sandboxed processes on windows
able
> to create shared memory?

Yes they are able to.

[Eliot Courtney, 2016-01-07 02:26:09]

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/broker_...
File mojo/edk/system/broker_messages.h (right):

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/broker_...
mojo/edk/system/broker_messages.h:23: enum WindowsSandboxMessages {
On 2016/01/06 02:58:16, jam wrote:
> nit: lets share the enum class type and just have ifdef around the IDs. that
> would simplify things a little (i.e. BrokerMessage can use the same type)

Done.

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
File mojo/edk/system/child_broker.cc (right):

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:45: ssize_t result =
PlatformChannelRecvmsg(parent_sync_channel_.get(), buf, 1,
On 2016/01/06 02:58:16, jam wrote:
> On 2016/01/05 04:45:47, Eliot Courtney wrote:
> > Does this need to be queried in a loop since it won't block? It seemed to me
> > that the Windows version of this code (ReadFile...) doesn't block in this
> case,
> > so I think it might have the same problem if this is a problem.
> 
> ReadFile below specifies the number of bytes to read ("sizeof(parent_handle)")
> and no overlapped struct (the NULL parameter), so it's synchronous.
> 
> I'm not an expert on posix by any means, but can't you simulate the above by
> temporarily calling
> fcntl(parent_sync_channel_.get(), F_SETFL, 0)
> on the pipe?

Thanks! I've done this, although made it not temporary, since it seems
reasonable parent_sync_channel_ should always be synchronous.

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:119: CreateSharedBufferNoLock(num_bytes);
On 2016/01/06 02:58:16, jam wrote:
> no need to have two methods for this. you can inline that second method here.
> CreatePlatformChannelPair is an exception because it is called initially with
> the lock already acquired.

Done.

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:329: 
On 2016/01/06 02:58:16, jam wrote:
> nit: no need to change

Done.

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:389: ssize_t bytes_written =
PlatformChannelWrite(
On 2016/01/06 02:58:16, jam wrote:
> On 2016/01/05 04:45:47, Eliot Courtney wrote:
> > This loop is on top of a non-blocking function (PlatformChannelWrite), which
> > could eat CPU; Is it worth somehow making this block? 
> 
> i don't see how it would ever block in practice, because we only write tiny
> messages one at a time?

I removed the O_NONBLOCK using fcntl like you suggested, so I think my concern
no longer applies.

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:401: while (1) {
On 2016/01/06 02:58:16, jam wrote:
> same as first comment in this file, why not make reads on this pipe always be
> blocking?

Done.

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:404:
PlatformChannelRecvmsg(parent_sync_channel_.get(), buf, 1, handles);
On 2016/01/05 04:45:47, Eliot Courtney wrote:
> Same comment as above.

Done.

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
File mojo/edk/system/child_broker_host.h (left):

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker_host.h:69: 
On 2016/01/06 02:58:16, jam wrote:
> nit: undo

Done.

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
File mojo/edk/system/child_broker_host.h (right):

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker_host.h:32: , public base::MessageLoopForIO::Watcher
{
On 2016/01/06 02:58:16, jam wrote:
> nit: you can now move the comma to line 28

Done.

[Anand Mistry (off Chromium), 2016-01-07 04:36:33]

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/broker...
File mojo/edk/system/broker_messages.h (right):

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/broker...
mojo/edk/system/broker_messages.h:22: enum SandboxMessages {
Not your fault, but these should have a uint32_t size declaration. Without it,
an enum size can vary based on platform, and use in a serialised struct could
cause crashes if you transfer between a 32 and 64 bit process. In Chrome,
there's at least one case where this is possible (NaCl, which is always ILP32).

Maybe you can make this change to all the enums here in a separate CL.

Also, I don't see any packed declarations. These should be added to any
serialised structures.

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/broker...
mojo/edk/system/broker_messages.h:49: uint32_t num_bytes;  // Size of the shared
buffer to create.
Maybe call it shared_buffer_size instead. num_bytes is pretty non-descriptive.

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/child_...
File mojo/edk/system/child_broker.cc (right):

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/child_...
mojo/edk/system/child_broker.cc:46: fcntl(parent_sync_channel_.get().handle,
F_SETFL,
Since you rely on the channels becoming blocking, I think these calls to fcntl()
should check the return value using PCHECK.

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/child_...
mojo/edk/system/child_broker.cc:123: sync_channel_lock_.Lock();
Why aren't you using AutoLock?

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/child_...
File mojo/edk/system/child_broker_host.cc (right):

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/child_...
mojo/edk/system/child_broker_host.cc:368: BrokerMessage* message =
reinterpret_cast<BrokerMessage*>(&read_data_[0]);
read_data_.data()

[Eliot Courtney, 2016-01-07 05:01:30]

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/broker...
File mojo/edk/system/broker_messages.h (right):

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/broker...
mojo/edk/system/broker_messages.h:22: enum SandboxMessages {
On 2016/01/07 04:36:33, Anand Mistry wrote:
> Not your fault, but these should have a uint32_t size declaration. Without it,
> an enum size can vary based on platform, and use in a serialised struct could
> cause crashes if you transfer between a 32 and 64 bit process. In Chrome,
> there's at least one case where this is possible (NaCl, which is always
ILP32).
> 
> Maybe you can make this change to all the enums here in a separate CL.
> 
> Also, I don't see any packed declarations. These should be added to any
> serialised structures.

Acknowledged.

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/broker...
mojo/edk/system/broker_messages.h:49: uint32_t num_bytes;  // Size of the shared
buffer to create.
On 2016/01/07 04:36:33, Anand Mistry wrote:
> Maybe call it shared_buffer_size instead. num_bytes is pretty non-descriptive.

Done.

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/child_...
File mojo/edk/system/child_broker.cc (right):

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/child_...
mojo/edk/system/child_broker.cc:46: fcntl(parent_sync_channel_.get().handle,
F_SETFL,
On 2016/01/07 04:36:33, Anand Mistry wrote:
> Since you rely on the channels becoming blocking, I think these calls to
fcntl()
> should check the return value using PCHECK.

Done.

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/child_...
mojo/edk/system/child_broker.cc:123: sync_channel_lock_.Lock();
On 2016/01/07 04:36:33, Anand Mistry wrote:
> Why aren't you using AutoLock?

This lock is actually a LockImpl, see comment:
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/edk/system/ch...

As far as I can tell you can't use AutoLock with it.

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/child_...
File mojo/edk/system/child_broker_host.cc (right):

https://codereview.chromium.org/1555273002/diff/100001/mojo/edk/system/child_...
mojo/edk/system/child_broker_host.cc:368: BrokerMessage* message =
reinterpret_cast<BrokerMessage*>(&read_data_[0]);
On 2016/01/07 04:36:33, Anand Mistry wrote:
> read_data_.data()

Done.

[jam, 2016-01-08 22:37:41]

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
File mojo/edk/system/child_broker.cc (right):

https://codereview.chromium.org/1555273002/diff/60001/mojo/edk/system/child_b...
mojo/edk/system/child_broker.cc:45: ssize_t result =
PlatformChannelRecvmsg(parent_sync_channel_.get(), buf, 1,
On 2016/01/07 02:26:09, Eliot Courtney wrote:
> On 2016/01/06 02:58:16, jam wrote:
> > On 2016/01/05 04:45:47, Eliot Courtney wrote:
> > > Does this need to be queried in a loop since it won't block? It seemed to
me
> > > that the Windows version of this code (ReadFile...) doesn't block in this
> > case,
> > > so I think it might have the same problem if this is a problem.
> > 
> > ReadFile below specifies the number of bytes to read
("sizeof(parent_handle)")
> > and no overlapped struct (the NULL parameter), so it's synchronous.
> > 
> > I'm not an expert on posix by any means, but can't you simulate the above by
> > temporarily calling
> > fcntl(parent_sync_channel_.get(), F_SETFL, 0)
> > on the pipe?
> 
> Thanks! I've done this, although made it not temporary, since it seems
> reasonable parent_sync_channel_ should always be synchronous.

great, i had some more statements in this comment about why it's temporary but
then i removed them (i agree it should always be sync). i just forgot this one
place :)

https://codereview.chromium.org/1555273002/diff/120001/mojo/edk/system/child_...
File mojo/edk/system/child_broker.cc (right):

https://codereview.chromium.org/1555273002/diff/120001/mojo/edk/system/child_...
mojo/edk/system/child_broker.cc:380: fcntl(parent_sync_channel_.get().handle,
F_GETFL, 0) & O_NONBLOCK);
code inside DCHECK will get compiled out in release builds, so instead this
should int rv = fcntl(...)
DCHECK_EQ(0, rv)

https://codereview.chromium.org/1555273002/diff/120001/mojo/edk/system/child_...
File mojo/edk/system/child_broker_host.cc (right):

https://codereview.chromium.org/1555273002/diff/120001/mojo/edk/system/child_...
mojo/edk/system/child_broker_host.cc:411: &iov, 1, &handle, 1);
seems like instead of  a method to do this, this call can just move up to line
389?

the windows code is different because one message can have arbitrary number of
tokens/handles. here we know that the one write will complete right away

[Eliot Courtney, 2016-01-10 01:31:59]

I noticed that in PlatformChannelRecvmsg it passes MSG_DONTWAIT to recvmsg,
which means that even if we set the socket to block using fcntl, it will still
be non-blocking. 

I've added a boolean argument to PlatformChannelRecvmsg to decide whether to do
a non-blocking read or not, but it's a bit subtle to have to both set your
socket to be blocking, and pass true to this function. I think the best solution
would be for PlatformChannelRecvmsg to not pass MSG_DONTWAIT at all. 

Do you know if it's safe to make that modification? I'm not sure if there are
things that expect non-blocking behaviour that will suddenly break in unexpected
ways with that change.

https://codereview.chromium.org/1555273002/diff/120001/mojo/edk/system/child_...
File mojo/edk/system/child_broker.cc (right):

https://codereview.chromium.org/1555273002/diff/120001/mojo/edk/system/child_...
mojo/edk/system/child_broker.cc:380: fcntl(parent_sync_channel_.get().handle,
F_GETFL, 0) & O_NONBLOCK);
On 2016/01/08 22:37:40, jam wrote:
> code inside DCHECK will get compiled out in release builds, so instead this
> should int rv = fcntl(...)
> DCHECK_EQ(0, rv)

Sorry, this line shouldn't be here -- I was just verifying the fd was still
blocking when this function gets called.

https://codereview.chromium.org/1555273002/diff/120001/mojo/edk/system/child_...
File mojo/edk/system/child_broker_host.cc (right):

https://codereview.chromium.org/1555273002/diff/120001/mojo/edk/system/child_...
mojo/edk/system/child_broker_host.cc:411: &iov, 1, &handle, 1);
On 2016/01/08 22:37:40, jam wrote:
> seems like instead of  a method to do this, this call can just move up to line
> 389?
> 
> the windows code is different because one message can have arbitrary number of
> tokens/handles. here we know that the one write will complete right away

Done.

[jam, 2016-01-11 04:56:50]

lgtm with minor comments

https://codereview.chromium.org/1555273002/diff/120001/mojo/edk/system/child_...
File mojo/edk/system/child_broker_host.cc (right):

https://codereview.chromium.org/1555273002/diff/120001/mojo/edk/system/child_...
mojo/edk/system/child_broker_host.cc:411: &iov, 1, &handle, 1);
On 2016/01/10 01:31:59, Eliot Courtney wrote:
> On 2016/01/08 22:37:40, jam wrote:
> > seems like instead of  a method to do this, this call can just move up to
line
> > 389?
> > 
> > the windows code is different because one message can have arbitrary number
of
> > tokens/handles. here we know that the one write will complete right away
> 
> Done.

I had meant line 389 above. you put it in line 402.

also there's no need to have a while loop at the top of the method. the child
process sends one message at a time. you can simplify this code by early exiting
in the "NOTREACHED() << "Unknown command. Stopping reading.";" and therefore
this code can be at the end of the method and not in any while loop or if
statement

[Anand Mistry (off Chromium), 2016-01-11 06:14:46]

lgtm

[Eliot Courtney, 2016-01-12 00:30:19]

https://codereview.chromium.org/1555273002/diff/120001/mojo/edk/system/child_...
File mojo/edk/system/child_broker_host.cc (right):

https://codereview.chromium.org/1555273002/diff/120001/mojo/edk/system/child_...
mojo/edk/system/child_broker_host.cc:411: &iov, 1, &handle, 1);
On 2016/01/11 04:56:49, jam wrote:
> On 2016/01/10 01:31:59, Eliot Courtney wrote:
> > On 2016/01/08 22:37:40, jam wrote:
> > > seems like instead of  a method to do this, this call can just move up to
> line
> > > 389?
> > > 
> > > the windows code is different because one message can have arbitrary
number
> of
> > > tokens/handles. here we know that the one write will complete right away
> > 
> > Done.
> 
> I had meant line 389 above. you put it in line 402.
> 
> also there's no need to have a while loop at the top of the method. the child
> process sends one message at a time. you can simplify this code by early
exiting
> in the "NOTREACHED() << "Unknown command. Stopping reading.";" and therefore
> this code can be at the end of the method and not in any while loop or if
> statement

Sorry, I misunderstood. I can remove the while loop, but I need to assume the
maximum size of the message, which is fine for now, since the messages are not
variable length on POSIX. By making the assumptions that 1. messages are not
variable length and 2. client only sends one message at a time, I simplified the
code. PTAL. Thanks!

[jam, 2016-01-12 07:42:39]

On 2016/01/12 00:30:19, Eliot Courtney wrote:
>
https://codereview.chromium.org/1555273002/diff/120001/mojo/edk/system/child_...
> File mojo/edk/system/child_broker_host.cc (right):
> 
>
https://codereview.chromium.org/1555273002/diff/120001/mojo/edk/system/child_...
> mojo/edk/system/child_broker_host.cc:411: &iov, 1, &handle, 1);
> On 2016/01/11 04:56:49, jam wrote:
> > On 2016/01/10 01:31:59, Eliot Courtney wrote:
> > > On 2016/01/08 22:37:40, jam wrote:
> > > > seems like instead of  a method to do this, this call can just move up
to
> > line
> > > > 389?
> > > > 
> > > > the windows code is different because one message can have arbitrary
> number
> > of
> > > > tokens/handles. here we know that the one write will complete right away
> > > 
> > > Done.
> > 
> > I had meant line 389 above. you put it in line 402.
> > 
> > also there's no need to have a while loop at the top of the method. the
child
> > process sends one message at a time. you can simplify this code by early
> exiting
> > in the "NOTREACHED() << "Unknown command. Stopping reading.";" and therefore
> > this code can be at the end of the method and not in any while loop or if
> > statement
> 
> Sorry, I misunderstood. I can remove the while loop, but I need to assume the
> maximum size of the message, which is fine for now, since the messages are not
> variable length on POSIX. By making the assumptions that 1. messages are not
> variable length and 2. client only sends one message at a time, I simplified
the
> code. PTAL. Thanks!

lgtm

[Eliot Courtney, 2016-01-14 23:31:11]

The CQ bit was checked by edcourtney@chromium.org

[Eliot Courtney, 2016-01-14 23:31:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
amistry@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1555273002/#ps220001
(title: "sync")

[commit-bot: I haz the power, 2016-01-14 23:33:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1555273002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1555273002/220001

[commit-bot: I haz the power, 2016-01-15 00:39:13]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-01-15 00:41:48]

Description was changed from

==========
[mojo] Add CreateSharedBuffer method to Broker.

BUG=556258
==========

to

==========
[mojo] Add CreateSharedBuffer method to Broker.

BUG=556258

Committed: https://crrev.com/565052abf18c8c6cad9a960831fc01f9d6b5b98a
Cr-Commit-Position: refs/heads/master@{#369631}
==========

[commit-bot: I haz the power, 2016-01-15 00:41:49]

Patchset 12 (id:??) landed as
https://crrev.com/565052abf18c8c6cad9a960831fc01f9d6b5b98a
Cr-Commit-Position: refs/heads/master@{#369631}

[Eliot Courtney, 2016-01-18 07:25:10]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/1598223004/ by edcourtney@chromium.org.

The reason for reverting is: This breaks some tests on the CFI bot. Reverting
until I work out the fix.

https://code.google.com/p/chromium/issues/detail?id=578716.