Mojo: Implement passing of shared buffers across processes on POSIX.

Mojo: Implement passing of shared buffers across processes on POSIX.

(Currently, the only test is in-process. I'll rectify that separately.)

R=yzshen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273887

[yzshen1, 2014-05-30 00:09:34]

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
File mojo/system/raw_shared_buffer.cc (right):

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
mojo/system/raw_shared_buffer.cc:22: scoped_refptr<RawSharedBuffer> deleter(rv);
With the raw and zero-ref-count |rv|, if something (e.g., in |InitNoLock()|)
AddRef() and then Release(), the object will be killed.

It seems safer to use scoped_refptr<...> as the type of |rv|, and change the
return type to scoped_refptr<...>.

(Besides, I feel a little bit uneasy if we return a raw ref-counted object
without increasing the ref count on behalf of the caller. Habit from COM and
Pepper. :))

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
mojo/system/raw_shared_buffer.cc:34: RawSharedBuffer* rv = new
RawSharedBuffer(num_bytes);
ditto.

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
mojo/system/raw_shared_buffer.cc:74: embedder::ScopedPlatformHandle
RawSharedBuffer::DuplicatePlatformHandle() {
Is it necessary to protect the access to handle_ for the two methods below?

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
File mojo/system/raw_shared_buffer_posix.cc (right):

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
mojo/system/raw_shared_buffer_posix.cc:98: PLOG(ERROR) << "fstat";
nit: maybe also output the errno?

https://codereview.chromium.org/304233005/diff/1/mojo/system/shared_buffer_di...
File mojo/system/shared_buffer_dispatcher.cc (right):

https://codereview.chromium.org/304233005/diff/1/mojo/system/shared_buffer_di...
mojo/system/shared_buffer_dispatcher.cc:80: LOG(ERROR) << "Invalid serialized
shared buffer  dispatcher (bad size)";
nit: extra space before "dispatcher".

https://codereview.chromium.org/304233005/diff/1/mojo/system/shared_buffer_di...
mojo/system/shared_buffer_dispatcher.cc:113: LOG(ERROR)
wrong indent.

[viettrungluu, 2014-05-30 00:23:41]

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
File mojo/system/raw_shared_buffer.cc (right):

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
mojo/system/raw_shared_buffer.cc:22: scoped_refptr<RawSharedBuffer> deleter(rv);
On 2014/05/30 00:09:35, yzshen1 wrote:
> With the raw and zero-ref-count |rv|, if something (e.g., in |InitNoLock()|)
> AddRef() and then Release(), the object will be killed.

This is equivalent to the scoped_refptr<>.

> 
> It seems safer to use scoped_refptr<...> as the type of |rv|, and change the
> return type to scoped_refptr<...>.

That leads to annoying refcount churn. Until we make scoped_refptr<>s passable,
I don't want to do this.

> 
> (Besides, I feel a little bit uneasy if we return a raw ref-counted object
> without increasing the ref count on behalf of the caller. Habit from COM and
> Pepper. :))

Shrug.

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
mojo/system/raw_shared_buffer.cc:74: embedder::ScopedPlatformHandle
RawSharedBuffer::DuplicatePlatformHandle() {
On 2014/05/30 00:09:35, yzshen1 wrote:
> Is it necessary to protect the access to handle_ for the two methods below?

No and no.

In the case of DuplicatePlatformHandle(), nothing ever changes the handle (see
below), except on deletion (in which case you'd have much bigger problems).

PassPlatformHandle() is designed to be used (and only safe to use) when you can
guarantee that your thread has the only ref to it.

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
File mojo/system/raw_shared_buffer_posix.cc (right):

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
mojo/system/raw_shared_buffer_posix.cc:98: PLOG(ERROR) << "fstat";
On 2014/05/30 00:09:35, yzshen1 wrote:
> nit: maybe also output the errno?

That's what PLOG does.

[yzshen1, 2014-05-30 00:27:48]

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
File mojo/system/raw_shared_buffer.cc (right):

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
mojo/system/raw_shared_buffer.cc:74: embedder::ScopedPlatformHandle
RawSharedBuffer::DuplicatePlatformHandle() {
On 2014/05/30 00:23:42, viettrungluu wrote:
> On 2014/05/30 00:09:35, yzshen1 wrote:
> > Is it necessary to protect the access to handle_ for the two methods below?
> 
> No and no.
> 
> In the case of DuplicatePlatformHandle(), nothing ever changes the handle (see
> below), except on deletion (in which case you'd have much bigger problems).
> 
> PassPlatformHandle() is designed to be used (and only safe to use) when you
can
> guarantee that your thread has the only ref to it.

Sounds good. Maybe adding your reply as comments?

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
File mojo/system/raw_shared_buffer_posix.cc (right):

https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
mojo/system/raw_shared_buffer_posix.cc:98: PLOG(ERROR) << "fstat";
On 2014/05/30 00:23:42, viettrungluu wrote:
> On 2014/05/30 00:09:35, yzshen1 wrote:
> > nit: maybe also output the errno?
> 
> That's what PLOG does.

Right. Didn't notice that. Thanks.

[viettrungluu, 2014-05-30 16:36:04]

Thanks, PTAL.

On 2014/05/30 00:27:48, yzshen1 wrote:
>
https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
> File mojo/system/raw_shared_buffer.cc (right):
> 
>
https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
> mojo/system/raw_shared_buffer.cc:74: embedder::ScopedPlatformHandle
> RawSharedBuffer::DuplicatePlatformHandle() {
> On 2014/05/30 00:23:42, viettrungluu wrote:
> > On 2014/05/30 00:09:35, yzshen1 wrote:
> > > Is it necessary to protect the access to handle_ for the two methods
below?
> > 
> > No and no.
> > 
> > In the case of DuplicatePlatformHandle(), nothing ever changes the handle
(see
> > below), except on deletion (in which case you'd have much bigger problems).
> > 
> > PassPlatformHandle() is designed to be used (and only safe to use) when you
> can
> > guarantee that your thread has the only ref to it.
> 
> Sounds good. Maybe adding your reply as comments?

I just deleted the lock entirely (and updated the comments).

> 
>
https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
> File mojo/system/raw_shared_buffer_posix.cc (right):
> 
>
https://codereview.chromium.org/304233005/diff/1/mojo/system/raw_shared_buffe...
> mojo/system/raw_shared_buffer_posix.cc:98: PLOG(ERROR) << "fstat";
> On 2014/05/30 00:23:42, viettrungluu wrote:
> > On 2014/05/30 00:09:35, yzshen1 wrote:
> > > nit: maybe also output the errno?
> > 
> > That's what PLOG does.
> 
> Right. Didn't notice that. Thanks.

[yzshen1, 2014-05-30 16:44:15]

There are two nits in mojo/system/shared_buffer_dispatcher.cc that haven't been
addressed.

Other than that LGTM

[viettrungluu, 2014-05-30 17:12:27]

Thanks.

On 2014/05/30 16:44:15, yzshen1 wrote:
> There are two nits in mojo/system/shared_buffer_dispatcher.cc that haven't
been
> addressed.

Oops, I don't know how I missed those. (I blame rietveld.)

Fixed.

> 
> Other than that LGTM

[viettrungluu, 2014-05-30 19:37:12]

Committed patchset #5 manually as r273887 (presubmit successful).