POSIX: Clean up DescriptorSet

POSIX: Clean up DescriptorSet

In general, the IPC Message objects are const and the iterator state is a void*
kept by the code which is doing the deserialisation.

However, now that we have an array of file descriptors, the index of the next
file descriptor is part of the iteration state and is kept /inside/ the Message
(in the DescriptorSet). This means that it's a mutable member, which is never
too nice and also, since the logging functions want to parse a message multiple
times, we've had to turn off returning an error when one runs off the end of
the array.

Also, the Message copy constructor and assignment function alters the /source/
Message, by stealing all of its descriptors

This patch encodes the index of each file descriptor on the wire. So the state
is moved from inside the DescriptorSet into the serialised data.

Additionally, the DescriptorSet is made into a lazyily created scoped_refptr,
solving the problems with copying messages.

[agl, 2009-02-11 21:48:28]

(Renaming DescriptorSet will be in a future review. I didn't want to rename and
change the file in one CL.)

[darin (slow to review), 2009-02-11 22:40:34]

http://codereview.chromium.org/20275/diff/1/2
File chrome/common/descriptor_set_posix.cc (right):

http://codereview.chromium.org/20275/diff/1/2#newcode60
Line 60: // We should always walk the descriptors in order, so it's reasonable
to
it seems like this constraint should be documented in the header along with the
comments that describe this method.

it seems kind of strange to assume that this method will only ever be reached as
part of some kind of in-order iteration.  is there no way to de-couple the order
checking from this class?

why bother with this in-order checking?  it seems like the range checking is
sufficient.

http://codereview.chromium.org/20275/diff/1/3
File chrome/common/descriptor_set_posix.h (right):

http://codereview.chromium.org/20275/diff/1/3#newcode58
Line 58: int GetNthDescriptor(unsigned n) const;
nit: how about GetDescriptorAt to avoid the non-word "Nth" ?

http://codereview.chromium.org/20275/diff/1/7
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/20275/diff/1/7#newcode675
Line 675: // keep the current descriptor as extra decoding state when
deserialising.
you end up keeping that state inside DescriptorSet...

[agl, 2009-02-11 23:01:08]

http://codereview.chromium.org/20275/diff/1/2
File chrome/common/descriptor_set_posix.cc (right):

http://codereview.chromium.org/20275/diff/1/2#newcode60
Line 60: // We should always walk the descriptors in order, so it's reasonable
to
On 2009/02/11 22:40:34, darin wrote:
> it seems like this constraint should be documented in the header along with
the
> comments that describe this method.

Done.

> it seems kind of strange to assume that this method will only ever be reached
as
> part of some kind of in-order iteration.  is there no way to de-couple the
order
> checking from this class?

Unfortunately, not really. The ParamTraits classes are self-contained.
 
> why bother with this in-order checking?  it seems like the range checking is
> sufficient.

The comment below explains the case where a renderer could DOS us with
unexcepted file descriptors without it.

http://codereview.chromium.org/20275/diff/1/3
File chrome/common/descriptor_set_posix.h (right):

http://codereview.chromium.org/20275/diff/1/3#newcode58
Line 58: int GetNthDescriptor(unsigned n) const;
On 2009/02/11 22:40:34, darin wrote:
> nit: how about GetDescriptorAt to avoid the non-word "Nth" ?

Done.

http://codereview.chromium.org/20275/diff/1/7
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/20275/diff/1/7#newcode675
Line 675: // keep the current descriptor as extra decoding state when
deserialising.
On 2009/02/11 22:40:34, darin wrote:
> you end up keeping that state inside DescriptorSet...

Yea, although it's just a check now. If you removed it, everything would still
work fine. It's only to stop nasty renderers from leaking file descriptors into
us.

[jeremy, 2009-02-11 23:13:33]

http://codereview.chromium.org/20275/diff/14/1002
File chrome/common/descriptor_set_posix.cc (right):

http://codereview.chromium.org/20275/diff/14/1002#newcode23
Line 23: // (which could a DOS against the browser by a rouge renderer) then all
*rouge -> rogue (credit goes to Avi for finding the typo).

[agl, 2009-02-11 23:25:53]

http://codereview.chromium.org/20275/diff/14/1002
File chrome/common/descriptor_set_posix.cc (right):

http://codereview.chromium.org/20275/diff/14/1002#newcode23
Line 23: // (which could a DOS against the browser by a rouge renderer) then all
On 2009/02/11 23:13:33, jeremy wrote:
> *rouge -> rogue (credit goes to Avi for finding the typo).

Done.

[agl, 2009-02-12 00:35:21]

Have addresses Darin's comments from IM conversation.

[darin (slow to review), 2009-02-12 00:43:15]

LGTM w/ these two issues addressed:

http://codereview.chromium.org/20275/diff/15/20
File chrome/common/ipc_message.h (right):

http://codereview.chromium.org/20275/diff/15/20#newcode171
Line 171: // On POSIX, a message may have an array of file descriptors
associated with
nit: no need to mention "array of file descriptors".. instead, something like
this: "On POSIX, a message supports reading / writing FileDescriptor objects. 
This is used to pass a file descriptor to the peer of an IPC channel."

http://codereview.chromium.org/20275/diff/15/20#newcode175
Line 175: bool AddFileDescriptor(const base::FileDescriptor& descriptor);
please name these WriteFileDescriptor and ReadFileDescriptor since they are
meant to be used like WriteInt and ReadInt.

[agl, 2009-02-12 03:33:14]

http://codereview.chromium.org/20275/diff/15/20
File chrome/common/ipc_message.h (right):

http://codereview.chromium.org/20275/diff/15/20#newcode171
Line 171: // On POSIX, a message may have an array of file descriptors
associated with
On 2009/02/12 00:43:15, darin wrote:
> nit: no need to mention "array of file descriptors".. instead, something like
> this: "On POSIX, a message supports reading / writing FileDescriptor objects. 
> This is used to pass a file descriptor to the peer of an IPC channel."

Done.

http://codereview.chromium.org/20275/diff/15/20#newcode175
Line 175: bool AddFileDescriptor(const base::FileDescriptor& descriptor);
On 2009/02/12 00:43:15, darin wrote:
> please name these WriteFileDescriptor and ReadFileDescriptor since they are
> meant to be used like WriteInt and ReadInt.

Done.

[darin (slow to review), 2009-02-12 04:03:39]

LGTM