[mojo] Allow value deserialization to fail

[mojo] Allow value deserialization to fail

This changes mojo::Deserialize_ et al to return a bool, opening
the door for deserialization code which performs custom validation
(in addition to baked-in mojom wire format validation) and may
reject incoming messages at the bindings layer.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. This CL
  5. https://codereview.chromium.org/1524613002
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=569669
Committed: https://crrev.com/13aa783fb893ac4c3b7977e3dcaee46abaf2cc47
Cr-Commit-Position: refs/heads/master@{#365728}

[Ken Rockot(use gerrit already), 2015-12-14 07:15:44]

Description was changed from

==========
[mojo] Allow value deserialization to fail

This changes mojo::Deserialize_ et al to return a bool, opening
the door for deserialization code which performs custom validation
(in addition to baked-in mojom wire format validation) and may
reject incoming messages at the bindings layer.

BUG=
==========

to

==========
[mojo] Allow value deserialization to fail

This changes mojo::Deserialize_ et al to return a bool, opening
the door for deserialization code which performs custom validation
(in addition to baked-in mojom wire format validation) and may
reject incoming messages at the bindings layer.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1524613002
  2. https://codereview.chromium.org/1515423002
  3. https://codereview.chromium.org/1517043004
  4. https://codereview.chromium.org/1524693002
  5. This CL
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=
==========

[Ken Rockot(use gerrit already), 2015-12-14 18:41:30]

rockot@chromium.org changed reviewers:
+ yzshen@chromium.org

[Ken Rockot(use gerrit already), 2015-12-14 18:46:27]

Description was changed from

==========
[mojo] Allow value deserialization to fail

This changes mojo::Deserialize_ et al to return a bool, opening
the door for deserialization code which performs custom validation
(in addition to baked-in mojom wire format validation) and may
reject incoming messages at the bindings layer.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1524613002
  2. https://codereview.chromium.org/1515423002
  3. https://codereview.chromium.org/1517043004
  4. https://codereview.chromium.org/1524693002
  5. This CL
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=
==========

to

==========
[mojo] Allow value deserialization to fail

This changes mojo::Deserialize_ et al to return a bool, opening
the door for deserialization code which performs custom validation
(in addition to baked-in mojom wire format validation) and may
reject incoming messages at the bindings layer.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. This CL
  5. https://codereview.chromium.org/1526533002
  6. https://codereview.chromium.org/1524703002

BUG=
==========

[Ken Rockot(use gerrit already), 2015-12-14 20:19:20]

Description was changed from

==========
[mojo] Allow value deserialization to fail

This changes mojo::Deserialize_ et al to return a bool, opening
the door for deserialization code which performs custom validation
(in addition to baked-in mojom wire format validation) and may
reject incoming messages at the bindings layer.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. This CL
  5. https://codereview.chromium.org/1526533002
  6. https://codereview.chromium.org/1524703002

BUG=
==========

to

==========
[mojo] Allow value deserialization to fail

This changes mojo::Deserialize_ et al to return a bool, opening
the door for deserialization code which performs custom validation
(in addition to baked-in mojom wire format validation) and may
reject incoming messages at the bindings layer.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. This CL
  5. https://codereview.chromium.org/1524613002
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=
==========

[yzshen1, 2015-12-14 20:45:19]

https://codereview.chromium.org/1520153002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/1520153002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:183: return false;
It may leak handles (here and elsewhere). Before deserialization, we have a step
called DecodePointersAndHandles(). One of its purposes is to take handles out of
Message::handles_ and place the raw handles in the right place of the message
payload.

Previously there isn't problem because Deserialize_() guarantees to consume all
those handles. With this CL it is no longer the case.

[Ken Rockot(use gerrit already), 2015-12-14 21:08:50]

Description was changed from

==========
[mojo] Allow value deserialization to fail

This changes mojo::Deserialize_ et al to return a bool, opening
the door for deserialization code which performs custom validation
(in addition to baked-in mojom wire format validation) and may
reject incoming messages at the bindings layer.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. This CL
  5. https://codereview.chromium.org/1524613002
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=
==========

to

==========
[mojo] Allow value deserialization to fail

This changes mojo::Deserialize_ et al to return a bool, opening
the door for deserialization code which performs custom validation
(in addition to baked-in mojom wire format validation) and may
reject incoming messages at the bindings layer.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. This CL
  5. https://codereview.chromium.org/1524613002
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=569669
==========

[Ken Rockot(use gerrit already), 2015-12-14 21:25:18]

On 2015/12/14 at 20:45:19, yzshen wrote:
>
https://codereview.chromium.org/1520153002/diff/20001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/lib/array_serialization.h (right):
> 
>
https://codereview.chromium.org/1520153002/diff/20001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/array_serialization.h:183: return false;
> It may leak handles (here and elsewhere). Before deserialization, we have a
step called DecodePointersAndHandles(). One of its purposes is to take handles
out of Message::handles_ and place the raw handles in the right place of the
message payload.
> 
> Previously there isn't problem because Deserialize_() guarantees to consume
all those handles. With this CL it is no longer the case.

Thanks, good catch! I've changed everything so that Deserialize_ should never
short-circuit in a way that leaks handles.

[yzshen1, 2015-12-14 22:18:33]

LGTM

https://codereview.chromium.org/1520153002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/1520153002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:312: return false;
Please consider commenting on why it is okay to short-circuit.

[Ken Rockot(use gerrit already), 2015-12-14 23:22:47]

On 2015/12/14 at 22:18:33, yzshen wrote:
> LGTM
> 
>
https://codereview.chromium.org/1520153002/diff/80001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/lib/array_serialization.h (right):
> 
>
https://codereview.chromium.org/1520153002/diff/80001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/array_serialization.h:312: return false;
> Please consider commenting on why it is okay to short-circuit.

Done. Thanks!

[Ken Rockot(use gerrit already), 2015-12-16 23:15:38]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-16 23:16:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1520153002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1520153002/260001

[commit-bot: I haz the power, 2015-12-16 23:52:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-16 23:52:39]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2015-12-17 03:42:22]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2015-12-17 03:42:22]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1520153002/#ps260001
(title: "merge")

[commit-bot: I haz the power, 2015-12-17 03:43:22]

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

[commit-bot: I haz the power, 2015-12-17 03:52:28]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-12-17 03:53:59]

Description was changed from

==========
[mojo] Allow value deserialization to fail

This changes mojo::Deserialize_ et al to return a bool, opening
the door for deserialization code which performs custom validation
(in addition to baked-in mojom wire format validation) and may
reject incoming messages at the bindings layer.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. This CL
  5. https://codereview.chromium.org/1524613002
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=569669
==========

to

==========
[mojo] Allow value deserialization to fail

This changes mojo::Deserialize_ et al to return a bool, opening
the door for deserialization code which performs custom validation
(in addition to baked-in mojom wire format validation) and may
reject incoming messages at the bindings layer.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. This CL
  5. https://codereview.chromium.org/1524613002
  6. https://codereview.chromium.org/1526533002
  7. https://codereview.chromium.org/1524703002

BUG=569669

Committed: https://crrev.com/13aa783fb893ac4c3b7977e3dcaee46abaf2cc47
Cr-Commit-Position: refs/heads/master@{#365728}
==========

[commit-bot: I haz the power, 2015-12-17 03:54:00]

Patchset 14 (id:??) landed as
https://crrev.com/13aa783fb893ac4c3b7977e3dcaee46abaf2cc47
Cr-Commit-Position: refs/heads/master@{#365728}