Add message sizing to basic IPC traits and struct macros.

Add message sizing to basic IPC traits and struct macros.

BUG=577685
Committed: https://crrev.com/0457af10a70c2ecf0e0b11a54881fe129232b739
Cr-Commit-Position: refs/heads/master@{#373720}

[Ken Rockot(use gerrit already), 2016-02-03 20:47:27]

rockot@chromium.org changed reviewers:
+ jam@chromium.org, tsepez@chromium.org

[Ken Rockot(use gerrit already), 2016-02-03 20:47:28]

Follow-up to the previous CL. This adds base::PickleSizer, and defines sizing
traits for all basic ParamTraits as well as IPC_STRUCT macros.

No message generators are updated yet, as most would require changing additional
ParamTraits definitions and the intent is to keep this CL relatively focused.

Most sizers are trivial to verify next to their writers, but I added a test for
base::Value since it's decidedly non-trivial.

[Ken Rockot(use gerrit already), 2016-02-03 20:48:19]

Description was changed from

==========
Add message sizing to basic IPC structs and traits macros.

BUG=577685
==========

to

==========
Add message sizing to basic IPC traits and struct macros.

BUG=577685
==========

[Tom Sepez, 2016-02-03 20:55:50]

On 2016/02/03 20:47:28, Ken Rockot wrote:
> Follow-up to the previous CL. This adds base::PickleSizer, and defines sizing
> traits for all basic ParamTraits as well as IPC_STRUCT macros.
> 
> No message generators are updated yet, as most would require changing
additional
> ParamTraits definitions and the intent is to keep this CL relatively focused.
> 
> Most sizers are trivial to verify next to their writers, but I added a test
for
> base::Value since it's decidedly non-trivial.

Be sure to do a build on linux with

GYP_DEFINES="enable_ipc_fuzzer=1" gclient runhooks
ninja -C out/Debug ipc_fuzzer

[Ken Rockot(use gerrit already), 2016-02-03 21:30:58]

On 2016/02/03 at 20:55:50, tsepez wrote:
> On 2016/02/03 20:47:28, Ken Rockot wrote:
> > Follow-up to the previous CL. This adds base::PickleSizer, and defines
sizing
> > traits for all basic ParamTraits as well as IPC_STRUCT macros.
> > 
> > No message generators are updated yet, as most would require changing
additional
> > ParamTraits definitions and the intent is to keep this CL relatively
focused.
> > 
> > Most sizers are trivial to verify next to their writers, but I added a test
for
> > base::Value since it's decidedly non-trivial.
> 
> Be sure to do a build on linux with
> 
> GYP_DEFINES="enable_ipc_fuzzer=1" gclient runhooks
> ninja -C out/Debug ipc_fuzzer

Done, no issues.

[jam, 2016-02-03 22:47:11]

https://codereview.chromium.org/1655333002/diff/80001/base/pickle.h
File base/pickle.h (right):

https://codereview.chromium.org/1655333002/diff/80001/base/pickle.h#newcode114
base/pickle.h:114: // Pickle's payload without thinking too hard about the
actual size of each
nit: thinking too hard->having to reference the Pickle implementation

https://codereview.chromium.org/1655333002/diff/80001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/1655333002/diff/80001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:111: ParamTraits<Type>::Write(m, static_cast<const Type&
>(p));
can we have some debug-only code here that checks that GetParamSize returned the
difference between Pickle's size() before and after Write?

https://codereview.chromium.org/1655333002/diff/80001/ipc/param_traits_size_m...
File ipc/param_traits_size_macros.h (right):

https://codereview.chromium.org/1655333002/diff/80001/ipc/param_traits_size_m...
ipc/param_traits_size_macros.h:20: // Set up so next include will generate write
methods.
nit: size methods

[Ken Rockot(use gerrit already), 2016-02-04 03:55:12]

I updated the CL with a prototype (see ipc_message_utils.h) and test for GetSize
validation, but it's not going to work. See comments below.

https://codereview.chromium.org/1655333002/diff/80001/base/pickle.h
File base/pickle.h (right):

https://codereview.chromium.org/1655333002/diff/80001/base/pickle.h#newcode114
base/pickle.h:114: // Pickle's payload without thinking too hard about the
actual size of each
On 2016/02/03 at 22:47:11, jam wrote:
> nit: thinking too hard->having to reference the Pickle implementation

done

https://codereview.chromium.org/1655333002/diff/80001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/1655333002/diff/80001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:111: ParamTraits<Type>::Write(m, static_cast<const Type&
>(p));
On 2016/02/03 at 22:47:11, jam wrote:
> can we have some debug-only code here that checks that GetParamSize returned
the difference between Pickle's size() before and after Write?

I explored this in some depth today. I don't think it's practical unfortunately
unless we ensure that all ParamTraits define GetSize
up front, which would make this a much larger CL.

I came up with a solution that only attempts to validate the GetSize() result
within WriteParam() if the parameter type "supports GetSize()"; in the trivial
case this just means ParamTrait<T>::GetSize is defined. Slightly more annoying
was supporting arbitrary parameterized types, i.e. making it so we don't try to
call ParamTraits<std::vector<T>>::GetSize() when T::GetSize() is not defined.

Where everything falls apart though is that of course arbitrary structs call
WriteParam on their fields. We'd have to add a whole new set of param traits
macros to do some magic like:

  static const bool kSupportsGetSize =
      internal::SupportsGetSize<decltype(field_name1)> &&
      internal::SupportsGetSize<decltype(field_name2)> &&
      ...;

and even that would still only cover IPC_STRUCT definitions, leaving custom
ParamTraits to be updated as needed.

Having said all this, I do really like the prospect of having explicit checks in
debug mode. If either of you feel strongly about this, I'll just rip the bandaid
off and add GetSize traits for everything that exists.

One advantage to ripping the bandaid off now is that we could experiment (at
very development low cost) with pre-allocating IPC::Message buffers which could
lead to easy performance gains.

One alternative which provides less coverage is to do this check within the Mojo
bindings code that consumes ParamTraits, since that is otherwise the only place
that will be calling GetSize().

https://codereview.chromium.org/1655333002/diff/80001/ipc/param_traits_size_m...
File ipc/param_traits_size_macros.h (right):

https://codereview.chromium.org/1655333002/diff/80001/ipc/param_traits_size_m...
ipc/param_traits_size_macros.h:20: // Set up so next include will generate write
methods.
On 2016/02/03 at 22:47:11, jam wrote:
> nit: size methods

done

[Ken Rockot(use gerrit already), 2016-02-04 03:58:54]

On 2016/02/04 at 03:55:12, Ken Rockot wrote:
> I updated the CL with a prototype (see ipc_message_utils.h) and test for
GetSize validation, but it's not going to work. See comments below.
> 
> https://codereview.chromium.org/1655333002/diff/80001/base/pickle.h
> File base/pickle.h (right):
> 
> https://codereview.chromium.org/1655333002/diff/80001/base/pickle.h#newcode114
> base/pickle.h:114: // Pickle's payload without thinking too hard about the
actual size of each
> On 2016/02/03 at 22:47:11, jam wrote:
> > nit: thinking too hard->having to reference the Pickle implementation
> 
> done
> 
> https://codereview.chromium.org/1655333002/diff/80001/ipc/ipc_message_utils.h
> File ipc/ipc_message_utils.h (right):
> 
>
https://codereview.chromium.org/1655333002/diff/80001/ipc/ipc_message_utils.h...
> ipc/ipc_message_utils.h:111: ParamTraits<Type>::Write(m, static_cast<const
Type& >(p));
> On 2016/02/03 at 22:47:11, jam wrote:
> > can we have some debug-only code here that checks that GetParamSize returned
the difference between Pickle's size() before and after Write?
> 
> I explored this in some depth today. I don't think it's practical
unfortunately unless we ensure that all ParamTraits define GetSize
> up front, which would make this a much larger CL.
> 
> I came up with a solution that only attempts to validate the GetSize() result
within WriteParam() if the parameter type "supports GetSize()"; in the trivial
case this just means ParamTrait<T>::GetSize is defined. Slightly more annoying
was supporting arbitrary parameterized types, i.e. making it so we don't try to
call ParamTraits<std::vector<T>>::GetSize() when T::GetSize() is not defined.
> 
> Where everything falls apart though is that of course arbitrary structs call
WriteParam on their fields. We'd have to add a whole new set of param traits
macros to do some magic like:
> 
>   static const bool kSupportsGetSize =
>       internal::SupportsGetSize<decltype(field_name1)> &&
>       internal::SupportsGetSize<decltype(field_name2)> &&
>       ...;
> 
> and even that would still only cover IPC_STRUCT definitions, leaving custom
ParamTraits to be updated as needed.
> 
> Having said all this, I do really like the prospect of having explicit checks
in debug mode. If either of you feel strongly about this, I'll just rip the
bandaid off and add GetSize traits for everything that exists.
> 
> One advantage to ripping the bandaid off now is that we could experiment (at
very development low cost) with pre-allocating IPC::Message buffers which could
lead to easy performance gains.
> 
> One alternative which provides less coverage is to do this check within the
Mojo bindings code that consumes ParamTraits, since that is otherwise the only
place that will be calling GetSize().

Another compromise option would be (given my prototype) to set kSupportsGetSize
to false in all macro-generated ParamTraits, as well as any custom ParamTraits 
that try to serialize unsupported types. We'd still get lots coverage this way,
but it wouldn't be 100%.

> 
>
https://codereview.chromium.org/1655333002/diff/80001/ipc/param_traits_size_m...
> File ipc/param_traits_size_macros.h (right):
> 
>
https://codereview.chromium.org/1655333002/diff/80001/ipc/param_traits_size_m...
> ipc/param_traits_size_macros.h:20: // Set up so next include will generate
write methods.
> On 2016/02/03 at 22:47:11, jam wrote:
> > nit: size methods
> 
> done

[jam, 2016-02-04 04:14:34]

lgtm so not to slow this down

doing this in mojo bindings only seems sufficient, since that's the only place
where a mistake can cause problems.

On 2016/02/04 03:58:54, Ken Rockot wrote:
> On 2016/02/04 at 03:55:12, Ken Rockot wrote:
> > I updated the CL with a prototype (see ipc_message_utils.h) and test for
> GetSize validation, but it's not going to work. See comments below.
> > 
> > https://codereview.chromium.org/1655333002/diff/80001/base/pickle.h
> > File base/pickle.h (right):
> > 
> >
https://codereview.chromium.org/1655333002/diff/80001/base/pickle.h#newcode114
> > base/pickle.h:114: // Pickle's payload without thinking too hard about the
> actual size of each
> > On 2016/02/03 at 22:47:11, jam wrote:
> > > nit: thinking too hard->having to reference the Pickle implementation
> > 
> > done
> > 
> >
https://codereview.chromium.org/1655333002/diff/80001/ipc/ipc_message_utils.h
> > File ipc/ipc_message_utils.h (right):
> > 
> >
>
https://codereview.chromium.org/1655333002/diff/80001/ipc/ipc_message_utils.h...
> > ipc/ipc_message_utils.h:111: ParamTraits<Type>::Write(m, static_cast<const
> Type& >(p));
> > On 2016/02/03 at 22:47:11, jam wrote:
> > > can we have some debug-only code here that checks that GetParamSize
returned
> the difference between Pickle's size() before and after Write?
> > 
> > I explored this in some depth today. I don't think it's practical
> unfortunately unless we ensure that all ParamTraits define GetSize
> > up front, which would make this a much larger CL.
> > 
> > I came up with a solution that only attempts to validate the GetSize()
result
> within WriteParam() if the parameter type "supports GetSize()"; in the trivial
> case this just means ParamTrait<T>::GetSize is defined. Slightly more annoying
> was supporting arbitrary parameterized types, i.e. making it so we don't try
to
> call ParamTraits<std::vector<T>>::GetSize() when T::GetSize() is not defined.
> > 
> > Where everything falls apart though is that of course arbitrary structs call
> WriteParam on their fields. We'd have to add a whole new set of param traits
> macros to do some magic like:
> > 
> >   static const bool kSupportsGetSize =
> >       internal::SupportsGetSize<decltype(field_name1)> &&
> >       internal::SupportsGetSize<decltype(field_name2)> &&
> >       ...;
> > 
> > and even that would still only cover IPC_STRUCT definitions, leaving custom
> ParamTraits to be updated as needed.
> > 
> > Having said all this, I do really like the prospect of having explicit
checks
> in debug mode. If either of you feel strongly about this, I'll just rip the
> bandaid off and add GetSize traits for everything that exists.
> > 
> > One advantage to ripping the bandaid off now is that we could experiment (at
> very development low cost) with pre-allocating IPC::Message buffers which
could
> lead to easy performance gains.
> > 
> > One alternative which provides less coverage is to do this check within the
> Mojo bindings code that consumes ParamTraits, since that is otherwise the only
> place that will be calling GetSize().
> 
> Another compromise option would be (given my prototype) to set
kSupportsGetSize
> to false in all macro-generated ParamTraits, as well as any custom ParamTraits

> that try to serialize unsupported types. We'd still get lots coverage this
way,
> but it wouldn't be 100%.
> 
> > 
> >
>
https://codereview.chromium.org/1655333002/diff/80001/ipc/param_traits_size_m...
> > File ipc/param_traits_size_macros.h (right):
> > 
> >
>
https://codereview.chromium.org/1655333002/diff/80001/ipc/param_traits_size_m...
> > ipc/param_traits_size_macros.h:20: // Set up so next include will generate
> write methods.
> > On 2016/02/03 at 22:47:11, jam wrote:
> > > nit: size methods
> > 
> > done

[Ken Rockot(use gerrit already), 2016-02-04 04:50:38]

On 2016/02/04 at 04:14:34, jam wrote:
> lgtm so not to slow this down
> 
> doing this in mojo bindings only seems sufficient, since that's the only place
where a mistake can cause problems.

word, done

> 
> On 2016/02/04 03:58:54, Ken Rockot wrote:
> > On 2016/02/04 at 03:55:12, Ken Rockot wrote:
> > > I updated the CL with a prototype (see ipc_message_utils.h) and test for
> > GetSize validation, but it's not going to work. See comments below.
> > > 
> > > https://codereview.chromium.org/1655333002/diff/80001/base/pickle.h
> > > File base/pickle.h (right):
> > > 
> > >
https://codereview.chromium.org/1655333002/diff/80001/base/pickle.h#newcode114
> > > base/pickle.h:114: // Pickle's payload without thinking too hard about the
> > actual size of each
> > > On 2016/02/03 at 22:47:11, jam wrote:
> > > > nit: thinking too hard->having to reference the Pickle implementation
> > > 
> > > done
> > > 
> > >
https://codereview.chromium.org/1655333002/diff/80001/ipc/ipc_message_utils.h
> > > File ipc/ipc_message_utils.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/1655333002/diff/80001/ipc/ipc_message_utils.h...
> > > ipc/ipc_message_utils.h:111: ParamTraits<Type>::Write(m, static_cast<const
> > Type& >(p));
> > > On 2016/02/03 at 22:47:11, jam wrote:
> > > > can we have some debug-only code here that checks that GetParamSize
returned
> > the difference between Pickle's size() before and after Write?
> > > 
> > > I explored this in some depth today. I don't think it's practical
> > unfortunately unless we ensure that all ParamTraits define GetSize
> > > up front, which would make this a much larger CL.
> > > 
> > > I came up with a solution that only attempts to validate the GetSize()
result
> > within WriteParam() if the parameter type "supports GetSize()"; in the
trivial
> > case this just means ParamTrait<T>::GetSize is defined. Slightly more
annoying
> > was supporting arbitrary parameterized types, i.e. making it so we don't try
to
> > call ParamTraits<std::vector<T>>::GetSize() when T::GetSize() is not
defined.
> > > 
> > > Where everything falls apart though is that of course arbitrary structs
call
> > WriteParam on their fields. We'd have to add a whole new set of param traits
> > macros to do some magic like:
> > > 
> > >   static const bool kSupportsGetSize =
> > >       internal::SupportsGetSize<decltype(field_name1)> &&
> > >       internal::SupportsGetSize<decltype(field_name2)> &&
> > >       ...;
> > > 
> > > and even that would still only cover IPC_STRUCT definitions, leaving
custom
> > ParamTraits to be updated as needed.
> > > 
> > > Having said all this, I do really like the prospect of having explicit
checks
> > in debug mode. If either of you feel strongly about this, I'll just rip the
> > bandaid off and add GetSize traits for everything that exists.
> > > 
> > > One advantage to ripping the bandaid off now is that we could experiment
(at
> > very development low cost) with pre-allocating IPC::Message buffers which
could
> > lead to easy performance gains.
> > > 
> > > One alternative which provides less coverage is to do this check within
the
> > Mojo bindings code that consumes ParamTraits, since that is otherwise the
only
> > place that will be calling GetSize().
> > 
> > Another compromise option would be (given my prototype) to set
kSupportsGetSize
> > to false in all macro-generated ParamTraits, as well as any custom
ParamTraits 
> > that try to serialize unsupported types. We'd still get lots coverage this
way,
> > but it wouldn't be 100%.
> > 
> > > 
> > >
> >
https://codereview.chromium.org/1655333002/diff/80001/ipc/param_traits_size_m...
> > > File ipc/param_traits_size_macros.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/1655333002/diff/80001/ipc/param_traits_size_m...
> > > ipc/param_traits_size_macros.h:20: // Set up so next include will generate
> > write methods.
> > > On 2016/02/03 at 22:47:11, jam wrote:
> > > > nit: size methods
> > > 
> > > done

[jam, 2016-02-04 18:33:47]

On 2016/02/04 04:50:38, Ken Rockot wrote:
> On 2016/02/04 at 04:14:34, jam wrote:
> > lgtm so not to slow this down
> > 
> > doing this in mojo bindings only seems sufficient, since that's the only
place
> where a mistake can cause problems.
> 
> word, done

LGTM

[jam, 2016-02-04 18:34:01]

https://codereview.chromium.org/1655333002/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/native_serialization.h (right):

https://codereview.chromium.org/1655333002/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/native_serialization.h:58:
DCHECK_GE(pickle->payload_size(), size_before_write);
this is checking that the payload size didn't shrink? how could that ever
happen?

[Ken Rockot(use gerrit already), 2016-02-04 18:36:12]

It can't unless pickle is broken. I guess the point is just to be certain
and highlight the assumption that the difference is >= 0.

On Thu, Feb 4, 2016 at 10:34 AM, <jam@chromium.org> wrote:

>
https://codereview.chromium.org/1655333002/diff/120001/mojo/public/cpp/bindin...
> File mojo/public/cpp/bindings/lib/native_serialization.h (right):
>
https://codereview.chromium.org/1655333002/diff/120001/mojo/public/cpp/bindin...
> mojo/public/cpp/bindings/lib/native_serialization.h:58
<https://codereview.chromium.org/1655333002/diff/120001/mojo/public/cpp/bindin...>:
> DCHECK_GE(pickle->payload_size(), size_before_write);
> this is checking that the payload size didn't shrink? how could that
> ever happen?
> https://codereview.chromium.org/1655333002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-02-04 23:44:54]

friendly ping tsepez@

[Tom Sepez, 2016-02-05 00:00:56]

lgtm

[Ken Rockot(use gerrit already), 2016-02-05 00:09:17]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-02-05 00:09:17]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1655333002/#ps140001
(title: "include ArrayHeader in size (oops!) and ensure the buffer doesn't
relocate")

[commit-bot: I haz the power, 2016-02-05 00:10:18]

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

[commit-bot: I haz the power, 2016-02-05 02:12:43]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-05 02:14:10]

Description was changed from

==========
Add message sizing to basic IPC traits and struct macros.

BUG=577685
==========

to

==========
Add message sizing to basic IPC traits and struct macros.

BUG=577685

Committed: https://crrev.com/0457af10a70c2ecf0e0b11a54881fe129232b739
Cr-Commit-Position: refs/heads/master@{#373720}
==========

[commit-bot: I haz the power, 2016-02-05 02:14:11]

Patchset 8 (id:??) landed as
https://crrev.com/0457af10a70c2ecf0e0b11a54881fe129232b739
Cr-Commit-Position: refs/heads/master@{#373720}