Mojo: Report bindings validation errors via MojoNotifyBadMessage

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

 {%- import "interface_macros.tmpl" as interface_macros %}
 {%- import "struct_macros.tmpl" as struct_macros %}
 
 {%- set class_name = interface.name %}
 {%- 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;
 {%-   for param in struct.packed.packed_fields_in_ordinal_order %}
   {{param.field.kind|cpp_wrapper_type}} p_{{param.field.name}}{};
 {%-   endfor %}
   {{struct.name}}DataView input_data_view({{params}},
                                           {{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) %}
 {%-   for param in parameters %}
 {%-     if param.kind|is_move_only_kind -%}
 std::move(p_{{param.name}})
 {%-     else -%}
 p_{{param.name}}
 {%-     endif -%}
 {%-     if not loop.last %}, {% endif %}
@@ skipping 45 matching lines @@
       scoped_refptr<mojo::internal::MultiplexRouter> router, bool* result
 {%-       for param in method.response_parameters -%}
       , {{param.kind|cpp_wrapper_type}}* out_{{param.name}}
 {%-       endfor %})
       : serialization_context_(std::move(router)), result_(result)
 {%-       for param in method.response_parameters -%}
         , out_{{param.name}}_(out_{{param.name}})
 {%-       endfor %} {
     DCHECK(!*result_);
   }
-  bool Accept(mojo::Message* message) override;
+  Result Accept(mojo::Message* message) override;
  private:
   mojo::internal::SerializationContext serialization_context_;
   bool* result_;
 {%-       for param in method.response_parameters %}
   {{param.kind|cpp_wrapper_type}}* out_{{param.name}}_;
 {%-       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 %}
 {%-         if param.kind|is_move_only_kind %}
   *out_{{param.name}}_ = std::move(p_{{param.name}});
 {%-         else %}
   *out_{{param.name}}_ = p_{{param.name}};
 {%-         endif %}
 {%-       endfor %}
   *result_ = true;
-  return true;
+  return Result::ForSuccess();
 }
 {%-     endif %}
 
 class {{class_name}}_{{method.name}}_ForwardToCallback
     : public mojo::MessageReceiver {
  public:
   {{class_name}}_{{method.name}}_ForwardToCallback(
       const {{class_name}}::{{method.name}}Callback& callback,
       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 %}
 
 {{proxy_name}}::{{proxy_name}}(mojo::MessageReceiverWithResponder* receiver)
     : ControlMessageProxy(receiver) {
 }
 
 {#--- Proxy definitions #}
 
@@ skipping 15 matching lines @@
   {{build_message(params_struct, "param_%s", params_description,
                   "&serialization_context_")}}
 
   bool result = false;
   mojo::MessageReceiver* responder =
       new {{class_name}}_{{method.name}}_HandleSyncResponse(
           serialization_context_.router, &result
 {%-     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 %}
 
 void {{proxy_name}}::{{method.name}}(
     {{interface_macros.declare_request_params("in_", method)}}) {
   {{struct_macros.get_serialized_size(params_struct, "in_%s",
                                       "&serialization_context_")}}
 
 {%- if method.response_parameters != None %}
   mojo::internal::RequestMessageBuilder builder({{message_name}}, size);
 {%- else %}
   mojo::internal::MessageBuilder builder({{message_name}}, size);
 {%- endif %}
 
   {{build_message(params_struct, "in_%s", params_description,
                   "&serialization_context_")}}
 
 {%- 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 %}
 
 {#--- ProxyToResponder definition #}
 {%- for method in interface.methods -%}
 {%-   if method.response_parameters != None %}
 {%-     set message_name =
             "internal::k%s_%s_Name"|format(interface.name, method.name) %}
 {%-     set response_params_struct = method.response_param_struct %}
@@ skipping 40 matching lines @@
 };
 void {{class_name}}_{{method.name}}_ProxyToResponder::Run(
     {{interface_macros.declare_params("in_", method.response_parameters)}}) {
   {{struct_macros.get_serialized_size(response_params_struct, "in_%s",
                                       "&serialization_context_")}}
   mojo::internal::ResponseMessageBuilder builder(
       {{message_name}}, size, request_id_,
       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;
 }
 {%-   endif -%}
 {%- endfor %}
 
 {{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 %}
   switch (message->header()->name) {
 {%-   for method in interface.methods %}
     case internal::k{{class_name}}_{{method.name}}_Name: {
 {%-     if method.response_parameters == None %}
       internal::{{class_name}}_{{method.name}}_Params_Data* params =
           reinterpret_cast<internal::{{class_name}}_{{method.name}}_Params_Data*>(
               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 %}
     }
 {%-   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 %}
   switch (message->header()->name) {
 {%-   for method in interface.methods %}
     case internal::k{{class_name}}_{{method.name}}_Name: {
 {%-     if method.response_parameters != None %}
       internal::{{class_name}}_{{method.name}}_Params_Data* params =
           reinterpret_cast<internal::{{class_name}}_{{method.name}}_Params_Data*>(
               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(
               message->request_id(),
               message->has_flag(mojo::internal::kMessageIsSync),
               responder,
               serialization_context_.router);
       {{class_name}}::{{method.name}}Callback callback(runnable);
       // A null |sink_| means no implementation was bound.
       assert(sink_);
       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 %}
     }
 {%-   endfor %}
   }
 {%- endif %}
-  return false;
+  return Result::ForUnexpectedRequest("{{class_name}}", message);
 }
 
 {#--- Request validator definitions #}
 
 {{class_name}}RequestValidator::{{class_name}}RequestValidator(
     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);
   }
 
   switch (message->header()->name) {
 {%- 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);
     }
 {%- endfor %}
     default:
       break;
   }
 
   // Unrecognized message.
   ReportValidationError(
       mojo::internal::VALIDATION_ERROR_MESSAGE_HEADER_UNKNOWN_METHOD);
-  return false;
+  return Result::ForUnexpectedRequest("{{class_name}}", message);
 }
 
 {#--- Response validator definitions #}
 {% if interface|has_callbacks %}
 {{class_name}}ResponseValidator::{{class_name}}ResponseValidator(
     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);
     }
 {%- endfor %}
     default:
       break;
   }
 
   // Unrecognized message.
   ReportValidationError(
       mojo::internal::VALIDATION_ERROR_MESSAGE_HEADER_UNKNOWN_METHOD);
-  return false;
+  return Result::ForUnexpectedResponse("{{class_name}}", message);
 }
 {%- endif -%}