Mojo: Report bindings validation errors via MojoNotifyBadMessage

Mojo: Report bindings validation errors via MojoNotifyBadMessage

Renames BoundsChecker to ValidationContext.

Plumbs a ValidationContext through to any code which might
report a message validation error. Calls NotifyBadMessage from
ReportValidationError so that all such errors may be bubbled
up to the embedder through the EDK.

R=yzshen@chromium.org
BUG=607293
Committed: https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa
Cr-Commit-Position: refs/heads/master@{#400075}

[Ken Rockot(use gerrit already), 2016-06-13 21:59:22]

Please take a look. Sorry about the size. Most of it is just plumbing Error
arguments around to all the MessageReceiver implementations. WDYT?

[yzshen1, 2016-06-14 16:45:32]

https://codereview.chromium.org/2064903002/diff/1/mojo/public/cpp/bindings/er...
File mojo/public/cpp/bindings/error.h (right):

https://codereview.chromium.org/2064903002/diff/1/mojo/public/cpp/bindings/er...
mojo/public/cpp/bindings/error.h:23: // read from a pipe. This may be raised on
the receiving end of a request.
Do you want to also return this code when the request message ID is not
recognizable?

(I am fine returning detailed error type. But if it is too much trouble to get
it accurate. I am fine with tristate: Error { OK, FAILED_BAD_MESSAGE, FAILED }.)

https://codereview.chromium.org/2064903002/diff/1/mojo/public/cpp/bindings/er...
mojo/public/cpp/bindings/error.h:28: // on the receiving side of a request, i.e.
the responder.
Does this error code also include the case that the receiving side of a response
cannot find its originating request?

https://codereview.chromium.org/2064903002/diff/1/mojo/public/cpp/bindings/er...
mojo/public/cpp/bindings/error.h:34: REQUEST_CANCELED,
Reading the code, it seems this error code is used at the receiving side of a
request?

https://codereview.chromium.org/2064903002/diff/1/mojo/public/cpp/bindings/me...
File mojo/public/cpp/bindings/message.h (right):

https://codereview.chromium.org/2064903002/diff/1/mojo/public/cpp/bindings/me...
mojo/public/cpp/bindings/message.h:125: virtual bool Accept(Message* message,
Error* error) WARN_UNUSED_RESULT = 0;
Does it make sense to merge the return value with Error? IIUC, we don't need
orthogonal "accepted" and "error".

Error Accept(Message* message);
or void Accept(Message* message, Error* error);

[Ken Rockot(use gerrit already), 2016-06-14 21:20:40]

https://codereview.chromium.org/2064903002/diff/1/mojo/public/cpp/bindings/er...
File mojo/public/cpp/bindings/error.h (right):

https://codereview.chromium.org/2064903002/diff/1/mojo/public/cpp/bindings/er...
mojo/public/cpp/bindings/error.h:23: // read from a pipe. This may be raised on
the receiving end of a request.
On 2016/06/14 at 16:45:32, yzshen1 wrote:
> Do you want to also return this code when the request message ID is not
recognizable?
> 
> (I am fine returning detailed error type. But if it is too much trouble to get
it accurate. I am fine with tristate: Error { OK, FAILED_BAD_MESSAGE, FAILED }.)

Done

https://codereview.chromium.org/2064903002/diff/1/mojo/public/cpp/bindings/er...
mojo/public/cpp/bindings/error.h:28: // on the receiving side of a request, i.e.
the responder.
On 2016/06/14 at 16:45:32, yzshen1 wrote:
> Does this error code also include the case that the receiving side of a
response cannot find its originating request?

That case is actually treated as a BAD_MESSAGE (see ForUnexpectedResponse and
its usage)

https://codereview.chromium.org/2064903002/diff/1/mojo/public/cpp/bindings/er...
mojo/public/cpp/bindings/error.h:34: REQUEST_CANCELED,
On 2016/06/14 at 16:45:32, yzshen1 wrote:
> Reading the code, it seems this error code is used at the receiving side of a
request?

Oops, this is gone now. The only uses that existed should have been
RESPONSE_DROPPED (fixed).

https://codereview.chromium.org/2064903002/diff/1/mojo/public/cpp/bindings/me...
File mojo/public/cpp/bindings/message.h (right):

https://codereview.chromium.org/2064903002/diff/1/mojo/public/cpp/bindings/me...
mojo/public/cpp/bindings/message.h:125: virtual bool Accept(Message* message,
Error* error) WARN_UNUSED_RESULT = 0;
On 2016/06/14 at 16:45:32, yzshen1 wrote:
> Does it make sense to merge the return value with Error? IIUC, we don't need
orthogonal "accepted" and "error".
> 
> Error Accept(Message* message);
> or void Accept(Message* message, Error* error);

Done!

[yzshen1, 2016-06-15 16:22:34]

Comments before our discussions about using ReportValidationError() to do the
work. Please ignore if we decide to go down that direction and re-write the
patch...

https://codereview.chromium.org/2064903002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/message.cc (right):

https://codereview.chromium.org/2064903002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/message.cc:95: MessageReceiver::Result::Result(Type
type) : Result(type, "") {}
nit: it is preferred to use a default constructed std::string than "".

https://codereview.chromium.org/2064903002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/message.cc:148: Result error(Type::BAD_MESSAGE,
It seems the difference between unexpected request/ request dropped is not very
obvious. (And also unexpected response / response dropped.)

https://codereview.chromium.org/2064903002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/message.cc:167: MessageReceiver::Result
MessageReceiver::Result::ForBadControlMessage(
Does this also apply to "pipe-level" control message? In that case I guess the
interface name is the master interface?

https://codereview.chromium.org/2064903002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/message.h (right):

https://codereview.chromium.org/2064903002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/message.h:143: // e.g. when an code is reached which
should be unreachable.
nit: an code ->  some code ?

https://codereview.chromium.org/2064903002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/message.h:195: // The receiver may mutate the given
message.  Returns true if the message
Please update the comment about the return value. (And also line 210 and 250)

https://codereview.chromium.org/2064903002/diff/20001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/2064903002/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:20:
return {{create_error}}("{{class_name}}", "{{method_name}}", {{message}});
Please consider using {{class_name}}::Name_ instead, here and other places.  We
probably compile with duplicate string elimination, but nice to not rely on
that.

Besides, "{{method_name}}" have a new string for each method, although it may
not be a lot, it adds to the binary size. Maybe the message type (method ordinal
value) is sufficient for debugging? If we do want method names, does it make
sense to define constants, instead of using string literals directly?

https://codereview.chromium.org/2064903002/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:272:
// TODO(darin): Failure here indicates a malformed message, and that may be
Does it make sense to replace this comment with the on line 208?

[Ken Rockot(use gerrit already), 2016-06-15 21:29:02]

Patchset #3 (id:40001) has been deleted

[Ken Rockot(use gerrit already), 2016-06-15 21:32:42]

OK per our offline discussion I've reworked the CL to report bad messages via
ReportValidationError. Also added error reporting for deserialization failures.
PTAL!

[yzshen1, 2016-06-15 22:20:10]

Great! LGTM with a few nits.

https://codereview.chromium.org/2064903002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_errors.cc (right):

https://codereview.chromium.org/2064903002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_errors.cc:75: if (context->message()) {
When g_validation_error_observer is set, we are in validation tests. Do we still
want to NotifyBadMessage in that case?

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

https://codereview.chromium.org/2064903002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_errors.h:67: // Deserialization of some
message parameter or struct field failed, for
nit: maybe simply say "Message deserialization failure, for example..."

"message parameter or struct field" don't cover everything (it could be a union
field, array element etc.)

https://codereview.chromium.org/2064903002/diff/80001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/2064903002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:19:
mojo::internal::ValidationContext validation_context(
Please create this context inside the "if (!success)" block.

[Ken Rockot(use gerrit already), 2016-06-15 22:38:23]

Thanks!

https://codereview.chromium.org/2064903002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_errors.cc (right):

https://codereview.chromium.org/2064903002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_errors.cc:75: if (context->message()) {
On 2016/06/15 at 22:20:09, yzshen1 wrote:
> When g_validation_error_observer is set, we are in validation tests. Do we
still want to NotifyBadMessage in that case?

I guess not. Done.

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

https://codereview.chromium.org/2064903002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_errors.h:67: // Deserialization of some
message parameter or struct field failed, for
On 2016/06/15 at 22:20:09, yzshen1 wrote:
> nit: maybe simply say "Message deserialization failure, for example..."
> 
> "message parameter or struct field" don't cover everything (it could be a
union field, array element etc.)

Done

https://codereview.chromium.org/2064903002/diff/80001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/2064903002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:19:
mojo::internal::ValidationContext validation_context(
On 2016/06/15 at 22:20:09, yzshen1 wrote:
> Please create this context inside the "if (!success)" block.

Done

[Ken Rockot(use gerrit already), 2016-06-15 22:38:26]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-06-15 22:38:26]

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/2064903002/#ps100001
(title: ".")

[commit-bot: I haz the power, 2016-06-15 22:38:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2064903002/100001

[Ken Rockot(use gerrit already), 2016-06-15 22:39:18]

Description was changed from

==========
Mojo: Propagate additional details for bindings errors

Adds a mojo::Error type which carries details about possible
failure modes at the bindings layer.

If an Error is encountered by the Connector when processing
an inbound message from its pipe, the Connector will notify
the EDK about the error through the new MojoNotifyBadMessage
API.

R=yzshen@chromium.org
BUG=607293
==========

to

==========
Mojo: Report bindings validation errors via MojoNotifyBadMessage
    
Plumbs a ValidationContext through to any code which might
report a message validation error. Calls NotifyBadMessage from
ReportValidationError so that all such errors may be bubbled
up to the embedder through the EDK.

R=yzshen@chromium.org
BUG=607293
==========

[Ken Rockot(use gerrit already), 2016-06-15 22:39:45]

Description was changed from

==========
Mojo: Report bindings validation errors via MojoNotifyBadMessage
    
Plumbs a ValidationContext through to any code which might
report a message validation error. Calls NotifyBadMessage from
ReportValidationError so that all such errors may be bubbled
up to the embedder through the EDK.

R=yzshen@chromium.org
BUG=607293
==========

to

==========
Mojo: Report bindings validation errors via MojoNotifyBadMessage

Renames BoundsChecker to ValidationContext.

Plumbs a ValidationContext through to any code which might
report a message validation error. Calls NotifyBadMessage from
ReportValidationError so that all such errors may be bubbled
up to the embedder through the EDK.

R=yzshen@chromium.org
BUG=607293
==========

[commit-bot: I haz the power, 2016-06-16 02:30:42]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-06-16 02:30:44]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-16 02:32:27]

Description was changed from

==========
Mojo: Report bindings validation errors via MojoNotifyBadMessage

Renames BoundsChecker to ValidationContext.

Plumbs a ValidationContext through to any code which might
report a message validation error. Calls NotifyBadMessage from
ReportValidationError so that all such errors may be bubbled
up to the embedder through the EDK.

R=yzshen@chromium.org
BUG=607293
==========

to

==========
Mojo: Report bindings validation errors via MojoNotifyBadMessage

Renames BoundsChecker to ValidationContext.

Plumbs a ValidationContext through to any code which might
report a message validation error. Calls NotifyBadMessage from
ReportValidationError so that all such errors may be bubbled
up to the embedder through the EDK.

R=yzshen@chromium.org
BUG=607293

Committed: https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa
Cr-Commit-Position: refs/heads/master@{#400075}
==========

[commit-bot: I haz the power, 2016-06-16 02:32:28]

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