Mojo: Report bindings validation errors via MojoNotifyBadMessage

mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl

Index: mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
diff --git a/mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl b/mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
index aef7e9d3d78ac3354f1183be91189099eb9fb8b1..2539c86c0fa2bb2a0041d85c3134e548b8e7d7d8 100644
--- a/mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
+++ b/mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
@@ -5,7 +5,8 @@
 {%- set proxy_name = interface.name ~ "Proxy" %}
 {%- set namespace_as_string = "%s"|format(namespace|replace(".","::")) %}
 
-{%- macro alloc_params(struct, params, message, serialization_context) %}
+{%- macro alloc_params(struct, params, message, create_error, method_name,
+                       serialization_context) %}
   ({{params}})->DecodePointers();
   ({{serialization_context}})->handles.Swap(({{message}})->mutable_handles());
   bool success = true;
@@ -16,7 +17,7 @@
                                           {{serialization_context}});
   {{struct_macros.deserialize(struct, "input_data_view", "p_%s", "success")}}
   if (!success)
-    return false;
+    return {{create_error}}("{{class_name}}", "{{method_name}}", {{message}});

[yzshen1, 2016/06/15 16:22:33]

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?

 {%- endmacro %}
 
 {%- macro pass_params(parameters) %}
@@ -82,7 +83,7 @@ class {{class_name}}_{{method.name}}_HandleSyncResponse
 {%-       endfor %} {
     DCHECK(!*result_);
   }
-  bool Accept(mojo::Message* message) override;
+  Result Accept(mojo::Message* message) override;
  private:
   mojo::internal::SerializationContext serialization_context_;
   bool* result_;
@@ -91,13 +92,15 @@ class {{class_name}}_{{method.name}}_HandleSyncResponse
 {%-       endfor -%}
   DISALLOW_COPY_AND_ASSIGN({{class_name}}_{{method.name}}_HandleSyncResponse);
 };
-bool {{class_name}}_{{method.name}}_HandleSyncResponse::Accept(
+mojo::MessageReceiver::Result
+{{class_name}}_{{method.name}}_HandleSyncResponse::Accept(
     mojo::Message* message) {
   internal::{{class_name}}_{{method.name}}_ResponseParams_Data* params =
       reinterpret_cast<internal::{{class_name}}_{{method.name}}_ResponseParams_Data*>(
           message->mutable_payload());
 
   {{alloc_params(method.response_param_struct, "params", "message",
+                 "Result::ForBadResponse", method.name,
                  "&serialization_context_")}}
 
 {%-       for param in method.response_parameters %}
@@ -108,7 +111,7 @@ bool {{class_name}}_{{method.name}}_HandleSyncResponse::Accept(
 {%-         endif %}
 {%-       endfor %}
   *result_ = true;
-  return true;
+  return Result::ForSuccess();
 }
 {%-     endif %}
 
@@ -120,22 +123,24 @@ class {{class_name}}_{{method.name}}_ForwardToCallback
       scoped_refptr<mojo::internal::MultiplexRouter> router)
       : callback_(callback), serialization_context_(std::move(router)) {
   }
-  bool Accept(mojo::Message* message) override;
+  Result Accept(mojo::Message* message) override;
  private:
   {{class_name}}::{{method.name}}Callback callback_;
   mojo::internal::SerializationContext serialization_context_;
   DISALLOW_COPY_AND_ASSIGN({{class_name}}_{{method.name}}_ForwardToCallback);
 };
-bool {{class_name}}_{{method.name}}_ForwardToCallback::Accept(
+mojo::MessageReceiver::Result
+{{class_name}}_{{method.name}}_ForwardToCallback::Accept(
     mojo::Message* message) {
   internal::{{class_name}}_{{method.name}}_ResponseParams_Data* params =
       reinterpret_cast<internal::{{class_name}}_{{method.name}}_ResponseParams_Data*>(
           message->mutable_payload());
 
   {{alloc_params(method.response_param_struct, "params", "message",
+                 "Result::ForBadResponse", method.name,
                  "&serialization_context_")}}
   callback_.Run({{pass_params(method.response_parameters)}});
-  return true;
+  return Result::ForSuccess();
 }
 {%-   endif %}
 {%- endfor %}
@@ -171,8 +176,10 @@ bool {{proxy_name}}::{{method.name}}(
 {%-     for param in method.response_parameters -%}
           , param_{{param.name}}
 {%-     endfor %});
-  if (!receiver_->AcceptWithResponder(builder.message(), responder))
+  if (!receiver_->AcceptWithResponder(builder.message(),
+                                      responder).Succeeded()) {
     delete responder;
+  }
   return result;
 }
 {%-   endif %}
@@ -193,14 +200,15 @@ void {{proxy_name}}::{{method.name}}(
 
 {%- if method.response_parameters != None %}
   mojo::MessageReceiver* responder =
-      new {{class_name}}_{{method.name}}_ForwardToCallback(callback, serialization_context_.router);
-  if (!receiver_->AcceptWithResponder(builder.message(), responder))
+      new {{class_name}}_{{method.name}}_ForwardToCallback(
+          callback, serialization_context_.router);
+  if (!receiver_->AcceptWithResponder(builder.message(), responder).Succeeded())
     delete responder;
 {%- else %}
-  bool ok = receiver_->Accept(builder.message());
-  // This return value may be ignored as !ok implies the Connector has
-  // encountered an error, which will be visible through other means.
-  ALLOW_UNUSED_LOCAL(ok);
+  // This return value may be ignored as any unsuccessful result implies the
+  // Connector has encountered an error, which will be visible through other
+  // means.
+  ignore_result(receiver_->Accept(builder.message()));
 {%- endif %}
 }
 {%- endfor %}
@@ -261,11 +269,10 @@ void {{class_name}}_{{method.name}}_ProxyToResponder::Run(
       is_sync_ ? mojo::internal::kMessageIsSync : 0);
   {{build_message(response_params_struct, "in_%s", params_description,
                   "&serialization_context_")}}
-  bool ok = responder_->Accept(builder.message());
-  ALLOW_UNUSED_LOCAL(ok);
-  // TODO(darin): !ok returned here indicates a malformed message, and that may
-  // be good reason to close the connection. However, we don't have a way to do
+  // TODO(darin): Failure here indicates a malformed message, and that may be

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

Does it make sense to replace this comment with the on line 208?

+  // good reason to close the connection. However, we don't have a way to do
   // that from here. We should add a way.
+  ignore_result(responder_->Accept(builder.message()));
   delete responder_;
   responder_ = nullptr;
 }
@@ -274,14 +281,16 @@ void {{class_name}}_{{method.name}}_ProxyToResponder::Run(
 
 {{class_name}}Stub::{{class_name}}Stub()
     : sink_(nullptr),
-      control_message_handler_({{interface.name}}::Version_) {
+      control_message_handler_({{interface.name}}::Name_,
+                               {{interface.name}}::Version_) {
 }
 
 {{class_name}}Stub::~{{interface.name}}Stub() {}
 
 {#--- Stub definition #}
 
-bool {{class_name}}Stub::Accept(mojo::Message* message) {
+mojo::MessageReceiver::Result {{class_name}}Stub::Accept(
+    mojo::Message* message) {
   if (mojo::internal::ControlMessageHandler::IsControlMessage(message))
     return control_message_handler_.Accept(message);
 {%- if interface.methods %}
@@ -294,12 +303,13 @@ bool {{class_name}}Stub::Accept(mojo::Message* message) {
               message->mutable_payload());
 
       {{alloc_params(method.param_struct, "params", "message",
+          "Result::ForBadRequest", method.name,
           "&serialization_context_")|indent(4)}}
       // A null |sink_| means no implementation was bound.
       assert(sink_);
       TRACE_EVENT0("mojom", "{{class_name}}::{{method.name}}");
       sink_->{{method.name}}({{pass_params(method.parameters)}});
-      return true;
+      return Result::ForSuccess();
 {%-     else %}
       break;
 {%-     endif %}
@@ -307,11 +317,12 @@ bool {{class_name}}Stub::Accept(mojo::Message* message) {
 {%-   endfor %}
   }
 {%- endif %}
-  return false;
+  return Result::ForUnexpectedRequest("{{class_name}}", message);
 }
 
-bool {{class_name}}Stub::AcceptWithResponder(
-    mojo::Message* message, mojo::MessageReceiverWithStatus* responder) {
+mojo::MessageReceiver::Result {{class_name}}Stub::AcceptWithResponder(
+    mojo::Message* message,
+    mojo::MessageReceiverWithStatus* responder) {
   if (mojo::internal::ControlMessageHandler::IsControlMessage(message))
     return control_message_handler_.AcceptWithResponder(message, responder);
 {%- if interface.methods %}
@@ -324,6 +335,7 @@ bool {{class_name}}Stub::AcceptWithResponder(
               message->mutable_payload());
 
       {{alloc_params(method.param_struct, "params", "message",
+          "Result::ForBadRequest", method.name,
           "&serialization_context_")|indent(4)}}
       {{class_name}}::{{method.name}}Callback::Runnable* runnable =
           new {{class_name}}_{{method.name}}_ProxyToResponder(
@@ -337,7 +349,7 @@ bool {{class_name}}Stub::AcceptWithResponder(
       TRACE_EVENT0("mojom", "{{class_name}}::{{method.name}}");
       sink_->{{method.name}}(
 {%- if method.parameters -%}{{pass_params(method.parameters)}}, {% endif -%}callback);
-      return true;
+      return Result::ForSuccess();
 {%-     else %}
       break;
 {%-     endif %}
@@ -345,7 +357,7 @@ bool {{class_name}}Stub::AcceptWithResponder(
 {%-   endfor %}
   }
 {%- endif %}
-  return false;
+  return Result::ForUnexpectedRequest("{{class_name}}", message);
 }
 
 {#--- Request validator definitions #}
@@ -354,12 +366,13 @@ bool {{class_name}}Stub::AcceptWithResponder(
     mojo::MessageReceiver* sink) : MessageFilter(sink) {
 }
 
-bool {{class_name}}RequestValidator::Accept(mojo::Message* message) {
+mojo::MessageReceiver::Result {{class_name}}RequestValidator::Accept(
+    mojo::Message* message) {
   assert(sink_);
 
   if (mojo::internal::ControlMessageHandler::IsControlMessage(message)) {
     if (!mojo::internal::ValidateControlRequest(message))
-      return false;
+      return Result::ForBadControlMessage("{{class_name}}", message);
     return sink_->Accept(message);
   }
 
@@ -367,15 +380,20 @@ bool {{class_name}}RequestValidator::Accept(mojo::Message* message) {
 {%- for method in interface.methods %}
     case internal::k{{class_name}}_{{method.name}}_Name: {
 {%-   if method.response_parameters != None %}
-      if (!mojo::internal::ValidateMessageIsRequestExpectingResponse(message))
-        return false;
+      if (!mojo::internal::ValidateMessageIsRequestExpectingResponse(message)) {
+        return Result::ForBadRequest(
+            "{{class_name}}", "{{method.name}}", message);
+      }
 {%-   else %}
-      if (!mojo::internal::ValidateMessageIsRequestWithoutResponse(message))
-        return false;
+      if (!mojo::internal::ValidateMessageIsRequestWithoutResponse(message)) {
+        return Result::ForBadRequest(
+            "{{class_name}}", "{{method.name}}", message);
+      }
 {%-   endif %}
       if (!mojo::internal::ValidateMessagePayload<
                internal::{{class_name}}_{{method.name}}_Params_Data>(message)) {
-        return false;
+        return Result::ForBadRequest(
+            "{{class_name}}", "{{method.name}}", message);
       }
       return sink_->Accept(message);
     }
@@ -387,7 +405,7 @@ bool {{class_name}}RequestValidator::Accept(mojo::Message* message) {
   // Unrecognized message.
   ReportValidationError(
       mojo::internal::VALIDATION_ERROR_MESSAGE_HEADER_UNKNOWN_METHOD);
-  return false;
+  return Result::ForUnexpectedRequest("{{class_name}}", message);
 }
 
 {#--- Response validator definitions #}
@@ -396,23 +414,26 @@ bool {{class_name}}RequestValidator::Accept(mojo::Message* message) {
     mojo::MessageReceiver* sink) : MessageFilter(sink) {
 }
 
-bool {{class_name}}ResponseValidator::Accept(mojo::Message* message) {
+mojo::MessageReceiver::Result {{class_name}}ResponseValidator::Accept(
+    mojo::Message* message) {
   assert(sink_);
 
   if (mojo::internal::ControlMessageHandler::IsControlMessage(message)) {
     if (!mojo::internal::ValidateControlResponse(message))
-      return false;
+      return Result::ForBadControlMessage("{{class_name}}", message);
     return sink_->Accept(message);
   }
 
   if (!mojo::internal::ValidateMessageIsResponse(message))
-    return false;
+    return Result::ForUnexpectedRequest("{{class_name}}", message);
   switch (message->header()->name) {
 {%- for method in interface.methods if method.response_parameters != None %}
     case internal::k{{class_name}}_{{method.name}}_Name: {
       if (!mojo::internal::ValidateMessagePayload<
-               internal::{{class_name}}_{{method.name}}_ResponseParams_Data>(message)) {
-        return false;
+               internal::{{class_name}}_{{method.name}}_ResponseParams_Data>(
+                    message)) {
+        return Result::ForBadResponse(
+            "{{class_name}}", "{{method.name}}", message);
       }
       return sink_->Accept(message);
     }
@@ -424,6 +445,6 @@ bool {{class_name}}ResponseValidator::Accept(mojo::Message* message) {
   // Unrecognized message.
   ReportValidationError(
       mojo::internal::VALIDATION_ERROR_MESSAGE_HEADER_UNKNOWN_METHOD);
-  return false;
+  return Result::ForUnexpectedResponse("{{class_name}}", message);
 }
 {%- endif -%}