PPAPI/NaCl: Speculative implementation for ipc_channel_nacl.cc

PPAPI/NaCl: Speculative implementation for ipc_channel_nacl.cc

BUG=116317
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139635

[dmichael (off chromium), 2012-05-01 21:42:10]

I think this is ready for a first-look...  it compiles, and is hopefully
plausible looking :-). It's speculative and won't do anything until
(a) Bill's CL lands: https://chromiumcodereview.appspot.com/10214007/
and
(b) We implement a NaClCustomDesc embedding, whose behavior is closely tied to
ipc_channel_nacl.cc.

mseaborn suggested that perhaps we shouldn't check this in until we have a
Chrome embedding of NaClCustomDesc. I could go either way... if we check it in
without testing, it will almost certainly have to change later to fix whatever
bugs it has, but that might be okay.

[Mark Seaborn, 2012-05-01 22:07:09]

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.cc
File ipc/ipc_channel_nacl.cc (right):

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.cc#new...
ipc/ipc_channel_nacl.cc:23: #include "sys/nacl_imc_api.h"
These "sys/nacl_*" headers are "system" headers for this file so really belong
in the earlier block and should use "#include <>".

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.cc#new...
ipc/ipc_channel_nacl.cc:44: while (bytes_read < 0 && errno == EAGAIN)
You don't need to handle EAGAIN.  I assume your Chrome-side NaCl desc
implementation won't produce it.

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.cc#new...
ipc/ipc_channel_nacl.cc:48: if (errno == ECONNRESET || errno == EPIPE) {
Ditto.  It's up to Chrome what errors this NaCl desc produces so it would be a
lot clearer to check for errors that Chrome *does* produce rather than just
guessing.

This is why I think you should implement the NaCl desc first. :-)

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.cc#new...
ipc/ipc_channel_nacl.cc:307: // TODO(dmichael): Is this right in NaCl?
Nope.  NaCl doesn't expose process IDs.

Well, "shouldn't" rather than "doesn't".  I actually want to disable our
getpid() syscall before the next branch because we shouldn't be leaking this
information.

[brettw, 2012-05-01 22:29:47]

Didn't look at the .cc file yet.

http://codereview.chromium.org/10174048/diff/3005/build/all.gyp
File build/all.gyp (right):

http://codereview.chromium.org/10174048/diff/3005/build/all.gyp#newcode26
build/all.gyp:26: '../ppapi/ppapi_proxy_untrusted.gyp:*',
Don't check this in.

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.cc
File ipc/ipc_channel_nacl.cc (right):

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.cc#new...
ipc/ipc_channel_nacl.cc:27: namespace {
Style nit: blank lines around this and the closing of the namespace as well.

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.cc#new...
ipc/ipc_channel_nacl.cc:35: CHECK(Channel::kReadBufferSize);
I'm not sure why you're asserting on this. I'd just remove this line.

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.cc#new...
ipc/ipc_channel_nacl.cc:44: while (bytes_read < 0 && errno == EAGAIN)
Right, we can probably not bother with EAGAIN for now.

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.h
File ipc/ipc_channel_nacl.h (right):

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.h#newc...
ipc/ipc_channel_nacl.h:82: // IPC::ChannelReader expects to be able to call
ReadData on us to
Blank line before this.

[brettw, 2012-05-02 18:18:56]

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.cc
File ipc/ipc_channel_nacl.cc (right):

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.cc#new...
ipc/ipc_channel_nacl.cc:87: private:
Blank line before this.

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.cc#new...
ipc/ipc_channel_nacl.cc:94: DISALLOW_COPY_AND_ASSIGN(ReaderThreadRunner);
Blank line before this.

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.cc#new...
ipc/ipc_channel_nacl.cc:204:
Channel::ChannelImpl::ReaderThreadRunner::ReaderThreadRunner(
This is confusing. Can you group all the ReaderThreadRunner functions below the
class decl above so that the two classes don't have interleaved functions?

http://codereview.chromium.org/10174048/diff/3005/ipc/ipc_channel_nacl.cc#new...
ipc/ipc_channel_nacl.cc:219: quit_lock_.Acquire();
I don't see how this quitting works. Normally this thread will be blocked on a
recv and you won't even get here. If somebody deletes the channel, it will call
Close which will block on this thread, which will be blocked on reading. Am I
missing something?

It seems like we should instead close the socket in Close() and then that should
abort all pending reads. Then we don't need this lock or flag at all, since the
thread will exit when the socket is closed.

[dmichael (off chromium), 2012-05-03 17:06:31]

Comments addressed. I can leave this hanging until the NaClDescCustom side is
done, if we prefer.

https://chromiumcodereview.appspot.com/10174048/diff/3005/ipc/ipc_channel_nac...
File ipc/ipc_channel_nacl.cc (right):

https://chromiumcodereview.appspot.com/10174048/diff/3005/ipc/ipc_channel_nac...
ipc/ipc_channel_nacl.cc:35: CHECK(Channel::kReadBufferSize);
On 2012/05/01 22:29:48, brettw wrote:
> I'm not sure why you're asserting on this. I'd just remove this line.
That was there since I dereference buffer->at(0). I know it's a bit silly, since
that should never be non-positive, but I tend to be paranoid about dereferencing
invalid memory. I took it out... It could be a compile assert, which would be
slightly less silly.

https://chromiumcodereview.appspot.com/10174048/diff/3005/ipc/ipc_channel_nac...
ipc/ipc_channel_nacl.cc:44: while (bytes_read < 0 && errno == EAGAIN)
On 2012/05/01 22:29:48, brettw wrote:
> Right, we can probably not bother with EAGAIN for now.
My thinking was that I don't know for sure yet if I'll want to sometimes respond
with EAGAIN, so I wasn't going to risk forgetting to add it later. But...  it's
so likely to be unnecessary that I might as well yank it.

https://chromiumcodereview.appspot.com/10174048/diff/3005/ipc/ipc_channel_nac...
ipc/ipc_channel_nacl.cc:48: if (errno == ECONNRESET || errno == EPIPE) {
On 2012/05/01 22:07:09, Mark Seaborn wrote:
> Ditto.  It's up to Chrome what errors this NaCl desc produces so it would be a
> lot clearer to check for errors that Chrome *does* produce rather than just
> guessing.
> 
> This is why I think you should implement the NaCl desc first. :-)
Yeah. I think I'll leave this here for now while I work on the other end, and I
can revisit it when I know. Consider it a placeholder for handling the chrome
errors. I added a TODO.

https://chromiumcodereview.appspot.com/10174048/diff/3005/ipc/ipc_channel_nac...
ipc/ipc_channel_nacl.cc:204:
Channel::ChannelImpl::ReaderThreadRunner::ReaderThreadRunner(
On 2012/05/02 18:18:56, brettw wrote:
> This is confusing. Can you group all the ReaderThreadRunner functions below
the
> class decl above so that the two classes don't have interleaved functions?
Yeah, I had it this way because originally I had ReaderThreadRunner defined in
the header file, and this was the style-guide imposed ordering. Done.

https://chromiumcodereview.appspot.com/10174048/diff/3005/ipc/ipc_channel_nac...
ipc/ipc_channel_nacl.cc:219: quit_lock_.Acquire();
On 2012/05/02 18:18:56, brettw wrote:
> I don't see how this quitting works. Normally this thread will be blocked on a
> recv and you won't even get here. If somebody deletes the channel, it will
call
> Close which will block on this thread, which will be blocked on reading. Am I
> missing something?
> 
> It seems like we should instead close the socket in Close() and then that
should
> abort all pending reads. Then we don't need this lock or flag at all, since
the
> thread will exit when the socket is closed.
You're right, that makes more sense. Done.

https://chromiumcodereview.appspot.com/10174048/diff/3005/ipc/ipc_channel_nac...
ipc/ipc_channel_nacl.cc:307: // TODO(dmichael): Is this right in NaCl?
On 2012/05/01 22:07:09, Mark Seaborn wrote:
> Nope.  NaCl doesn't expose process IDs.
> 
> Well, "shouldn't" rather than "doesn't".  I actually want to disable our
> getpid() syscall before the next branch because we shouldn't be leaking this
> information.
Do you have a bug to track that? I would think that at this point we might have
to leave the call in place but maybe return some random fake pid or something
for ABI compatibility.

Anyway, I shouldn't need a real pid here, so I'll just use 0 for now. In fact,
I've realized that it probably doesn't make sense to have the HelloMessage
handshake at all, since there will be an underlying channel in the trusted side
taking care of it. So...  I'll probably delete it in a future patch if/when I'm
more certain.

[brettw, 2012-05-03 21:06:25]

LGTM, good luck debugging this!

[Mark Seaborn, 2012-05-03 21:34:29]

On 3 May 2012 10:06, <dmichael@chromium.org> wrote:

> Comments addressed. I can leave this hanging until the NaClDescCustom side
> is
> done, if we prefer.
>

Yes, that would be better.


https://chromiumcodereview.appspot.com/10174048/diff/3005/ipc/ipc_channel_nac...
> ipc/ipc_channel_nacl.cc:219: quit_lock_.Acquire();
>
> On 2012/05/02 18:18:56, brettw wrote:
>
>> I don't see how this quitting works. Normally this thread will be blocked
>> on a
>>
> recv and you won't even get here. If somebody deletes the channel, it will
>> call
>>
> Close which will block on this thread, which will be blocked on reading.
>> Am I
>>
> missing something?
>>
>
>  It seems like we should instead close the socket in Close() and then that
>> should
>>
> abort all pending reads. Then we don't need this lock or flag at all,
>> since the
>>
> thread will exit when the socket is closed.
>>
>
Are you assuming that if thread A is doing

recvmsg(N, buffer...)

and thread B does close(N), then the recvmsg() call in thread A will be
aborted and return?  That assumption is not correct.  File descriptors are
refcounted, and an in-progress operation like recvmsg() simply holds a
reference to the descriptor, so close() will not abort the recvmsg().

With Unix sockets, though, shutdown() would do what you're thinking of.
You could provide a similar facility through your Chromium-side
NaClCustomDesc implementation.


>
<https://chromiumcodereview.appspot.com/10174048/diff/3005/ipc/ipc_channel_nac...>
>
> ipc/ipc_channel_nacl.cc:307: // TODO(dmichael): Is this right in NaCl?
> On 2012/05/01 22:07:09, Mark Seaborn wrote:
>
>> Nope.  NaCl doesn't expose process IDs.
>>
>
>  Well, "shouldn't" rather than "doesn't".  I actually want to disable our
>>
> getpid() syscall before the next branch because we shouldn't be leaking
>> this
>>
> information.
>>
> Do you have a bug to track that?


Yes: http://code.google.com/p/nativeclient/issues/detail?id=537


> I would think that at this point we
> might have to leave the call in place but maybe return some random fake
> pid or something for ABI compatibility.
>

There's no stable ABI to be compatible with.  getpid() was only ever
exposed as a NaCl syscall, and direct NaCl syscalls are not part of our
stable ABI.


> Anyway, I shouldn't need a real pid here, so I'll just use 0 for now.


Be careful, because 0 and -1 have special meanings if passed to kill() and
waitpid().  If there has to be a GetProcId method, it would be safer if it
were never called and if it had a NOTREACHED.


> In fact, I've realized that it probably doesn't make sense to have the
> HelloMessage handshake at all, since there will be an underlying channel
> in the trusted side taking care of it. So...  I'll probably delete it in
> a future patch if/when I'm more certain.
>

Right, a hello message shouldn't be necessary.

Mark

[Mark Seaborn, 2012-05-03 21:42:22]

https://chromiumcodereview.appspot.com/10174048/diff/10005/ipc/ipc_channel_na...
File ipc/ipc_channel_nacl.cc (right):

https://chromiumcodereview.appspot.com/10174048/diff/10005/ipc/ipc_channel_na...
ipc/ipc_channel_nacl.cc:178: close(pipe_);
Doing close() before joining the thread is not the right thing to do.  Firstly,
it doesn't do what you want (see previous comment).  Also, there is a race
condition: if you do close(fd) before recvmsg(fd, ...) happens in the other
thread, the recvmsg() will be on an undefined file descriptor, and possibly on
an FD created by another thread.  So close() should be done after the join.

Also, you should check for errors from close().  Otherwise, if the process makes
mistakes about FD lifetime, you won't find out about it.

[dmichael (off chromium), 2012-05-03 22:29:43]

On 2012/05/03 21:34:29, Mark Seaborn wrote:
> On 3 May 2012 10:06, <mailto:dmichael@chromium.org> wrote:
> 
> > Comments addressed. I can leave this hanging until the NaClDescCustom side
> > is
> > done, if we prefer.
> >
> 
> Yes, that would be better.
> 
> 
>
https://chromiumcodereview.appspot.com/10174048/diff/3005/ipc/ipc_channel_nac...
> > ipc/ipc_channel_nacl.cc:219: quit_lock_.Acquire();
> >
> > On 2012/05/02 18:18:56, brettw wrote:
> >
> >> I don't see how this quitting works. Normally this thread will be blocked
> >> on a
> >>
> > recv and you won't even get here. If somebody deletes the channel, it will
> >> call
> >>
> > Close which will block on this thread, which will be blocked on reading.
> >> Am I
> >>
> > missing something?
> >>
> >
> >  It seems like we should instead close the socket in Close() and then that
> >> should
> >>
> > abort all pending reads. Then we don't need this lock or flag at all,
> >> since the
> >>
> > thread will exit when the socket is closed.
> >>
> >
> Are you assuming that if thread A is doing
> 
> recvmsg(N, buffer...)
> 
> and thread B does close(N), then the recvmsg() call in thread A will be
> aborted and return?  That assumption is not correct.  File descriptors are
> refcounted, and an in-progress operation like recvmsg() simply holds a
> reference to the descriptor, so close() will not abort the recvmsg().
So you're saying close() does not result in calling NaClDescCustomFuncs.Destroy?
I was assuming I would be able to handle close() as I pleased...

> 
> With Unix sockets, though, shutdown() would do what you're thinking of.
> You could provide a similar facility through your Chromium-side
> NaClCustomDesc implementation.
> 
> 
> >
>
<https://chromiumcodereview.appspot.com/10174048/diff/3005/ipc/ipc_channel_nac...>
> >
> > ipc/ipc_channel_nacl.cc:307: // TODO(dmichael): Is this right in NaCl?
> > On 2012/05/01 22:07:09, Mark Seaborn wrote:
> >
> >> Nope.  NaCl doesn't expose process IDs.
> >>
> >
> >  Well, "shouldn't" rather than "doesn't".  I actually want to disable our
> >>
> > getpid() syscall before the next branch because we shouldn't be leaking
> >> this
> >>
> > information.
> >>
> > Do you have a bug to track that?
> 
> 
> Yes: http://code.google.com/p/nativeclient/issues/detail?id=537
> 
> 
> > I would think that at this point we
> > might have to leave the call in place but maybe return some random fake
> > pid or something for ABI compatibility.
> >
> 
> There's no stable ABI to be compatible with.  getpid() was only ever
> exposed as a NaCl syscall, and direct NaCl syscalls are not part of our
> stable ABI.
> 
So...  we're not promising to keep system call support stable? I would have
considered it part of our ABI even if it's not part of the IRT. Isn't this
plausibly going to affect existing apps in the web store, or am I missing
something?
> 
> > Anyway, I shouldn't need a real pid here, so I'll just use 0 for now.
> 
> 
> Be careful, because 0 and -1 have special meanings if passed to kill() and
> waitpid().
If we're actually doing those, I'm fine with them failing.
>  If there has to be a GetProcId method, it would be safer if it
> were never called and if it had a NOTREACHED.
Yes, that's probably the right thing to do in base/process_util_posix.cc for
NaCl (or make our own process_util_nacl.cc).
> 
> 
> > In fact, I've realized that it probably doesn't make sense to have the
> > HelloMessage handshake at all, since there will be an underlying channel
> > in the trusted side taking care of it. So...  I'll probably delete it in
> > a future patch if/when I'm more certain.
> >
> 
> Right, a hello message shouldn't be necessary.
> 
> Mark

[dmichael (off chromium), 2012-05-03 22:43:05]

> > There's no stable ABI to be compatible with.  getpid() was only ever
> > exposed as a NaCl syscall, and direct NaCl syscalls are not part of our
> > stable ABI.
> > 
> So...  we're not promising to keep system call support stable? I would have
> considered it part of our ABI even if it's not part of the IRT. Isn't this
> plausibly going to affect existing apps in the web store, or am I missing
> something?
> > 
Disregard...  I see that getpid() will return -1 now, which makes sense. I
thought you were doing something more drastic like removing it from the
trampoline or something.

[Mark Seaborn, 2012-05-03 22:55:00]

On 3 May 2012 15:29, <dmichael@chromium.org> wrote:

> On 2012/05/03 21:34:29, Mark Seaborn wrote:
>
> Are you assuming that if thread A is doing
>>
>
>  recvmsg(N, buffer...)
>>
>
>  and thread B does close(N), then the recvmsg() call in thread A will be
>> aborted and return?  That assumption is not correct.  File descriptors are
>> refcounted, and an in-progress operation like recvmsg() simply holds a
>> reference to the descriptor, so close() will not abort the recvmsg().
>>
> So you're saying close() does not result in calling
> NaClDescCustomFuncs.Destroy?
> I was assuming I would be able to handle close() as I pleased...
>

close() removes an entry from the FD table which decrements the descriptor
object's refcount.  When the refcount reaches zero,
NaClDescCustomFuncs.Destroy is called.

A syscall like imc_recvmsg() also holds a reference to the descriptor
object while the SendMsg() operation is in progress, and it decrements the
refcount when SendMsg() completes.  We *could* change it so that SendMsg()
takes an owning reference to the descriptor instead of a borrowed
reference, which would allow a descriptor to implement "close() cancels
recvmsg()".  However, "close() cancels recvmsg()" doesn't really seem like
the right way to do things.  You'd have a hard time implementing it on top
of the host OS's sockets.


 > I would think that at this point we
>> > might have to leave the call in place but maybe return some random fake
>> > pid or something for ABI compatibility.
>> >
>>
>
>  There's no stable ABI to be compatible with.  getpid() was only ever
>> exposed as a NaCl syscall, and direct NaCl syscalls are not part of our
>> stable ABI.
>>
>
> So...  we're not promising to keep system call support stable?


That's right.


> I would have
> considered it part of our ABI even if it's not part of the IRT. Isn't this
> plausibly going to affect existing apps in the web store, or am I missing
> something?
>

It shouldn't affect existing apps because they shouldn't be doing direct
syscalls.  Unfortunately, getpid() in the SDK does the wrong thing by doing
a direct syscall:
http://code.google.com/p/nativeclient/issues/detail?id=2488  We need to fix
the SDK.


>
>  > Anyway, I shouldn't need a real pid here, so I'll just use 0 for now.
>>
>
>
>  Be careful, because 0 and -1 have special meanings if passed to kill() and
>> waitpid().
>>
> If we're actually doing those, I'm fine with them failing.
>

If you pass 0 to kill() it could kill the whole browser (the process group)
or if you pass -1 it could kill all the user's processes:

       "If pid equals 0, then sig is sent to every process in the process
group of the
       calling process.

       If pid equals -1, then sig is sent to every process for which the
calling
       process has permission to send signals, except for process 1 (init),
but see
       below."
       -- http://www.kernel.org/doc/man-pages/online/pages/man2/kill.2.html

It would be better to avoid that happening. :-)

Mark

[Mark Seaborn, 2012-05-03 22:56:41]

On 3 May 2012 15:43, <dmichael@chromium.org> wrote:

> > There's no stable ABI to be compatible with.  getpid() was only ever
>> > exposed as a NaCl syscall, and direct NaCl syscalls are not part of our
>> > stable ABI.
>> >
>> So...  we're not promising to keep system call support stable? I would
>> have
>> considered it part of our ABI even if it's not part of the IRT. Isn't this
>> plausibly going to affect existing apps in the web store, or am I missing
>> something?
>>
>>  Disregard...  I see that getpid() will return -1 now, which makes sense.
> I
> thought you were doing something more drastic like removing it from the
> trampoline or something.
>

We don't have any plans to do that, but we could do it, because the
trampolines are not part of NaCl's stable interface.  That's why we should
fix http://code.google.com/p/nativeclient/issues/detail?id=2488.

Cheers,
Mark

[dmichael (off chromium), 2012-05-10 22:36:51]

Removed some of the placeholders, TODOs, and the "Hello" message stuff.  PTAL

[bbudge, 2012-05-16 23:50:06]

http://codereview.chromium.org/10174048/diff/21005/ipc/ipc_channel_nacl.cc
File ipc/ipc_channel_nacl.cc (right):

http://codereview.chromium.org/10174048/diff/21005/ipc/ipc_channel_nacl.cc#ne...
ipc/ipc_channel_nacl.cc:34: return null_ptr.Pass();
why not just 'return null_ptr;' or even 'return scoped_ptr<std::vector<char>
>();'?

http://codereview.chromium.org/10174048/diff/21005/ipc/ipc_channel_nacl.cc#ne...
ipc/ipc_channel_nacl.cc:48: return null_ptr.Pass();
ditto

[dmichael (off chromium), 2012-05-17 02:19:38]

On May 16, 2012 5:50 PM, <bbudge@chromium.org> wrote:
>
>
> http://codereview.chromium.org/10174048/diff/21005/ipc/ipc_channel_nacl.cc
> File ipc/ipc_channel_nacl.cc (right):
>
>
http://codereview.chromium.org/10174048/diff/21005/ipc/ipc_channel_nacl.cc#ne...
> ipc/ipc_channel_nacl.cc:34: return null_ptr.Pass();
> why not just 'return null_ptr;' or even 'return
> scoped_ptr<std::vector<char> >();'?
>
It's a scoped_ptr; you have to call Pass to transfer ownership (even though
it's NULL...  the compiler doesn't know that statically). But I also think
"scoped_ptr<std::vector<char> >()" would work, since it's a temporary, if
you think that's more readable.

>
http://codereview.chromium.org/10174048/diff/21005/ipc/ipc_channel_nacl.cc#ne...
> ipc/ipc_channel_nacl.cc:48: return null_ptr.Pass();
> ditto
>
> http://codereview.chromium.org/10174048/

[commit-bot: I haz the power, 2012-05-30 16:37:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10174048/31005

[commit-bot: I haz the power, 2012-05-30 19:35:24]

Try job failure for 10174048-31005 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-30 19:40:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10174048/31005

[commit-bot: I haz the power, 2012-05-30 21:12:27]

Change committed as 139635