[Extensions Bindings] Add errors to signature parsing

extensions/renderer/api_signature.cc

Index: extensions/renderer/api_signature.cc
diff --git a/extensions/renderer/api_signature.cc b/extensions/renderer/api_signature.cc
index c83eb9105f367c6b3b877b788ce12cd58a18d81a..39255bcd343534e63bc379fdb02301713aaeeaaf 100644
--- a/extensions/renderer/api_signature.cc
+++ b/extensions/renderer/api_signature.cc
@@ -9,6 +9,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/values.h"
 #include "content/public/child/v8_value_converter.h"
+#include "extensions/renderer/api_invocation_errors.h"
 #include "extensions/renderer/argument_spec.h"
 #include "gin/arguments.h"
 
@@ -84,6 +85,10 @@ class ArgumentParser {
   std::string* error_;
   size_t current_index_ = 0;
 
+  // An error to pass while parsing arguments to avoid having to allocate a new
+  // std::string on the stack multiple times.
+  std::string parse_error_;
+
   DISALLOW_COPY_AND_ASSIGN(ArgumentParser);
 };
 
@@ -158,6 +163,11 @@ class BaseValueArgumentParser : public ArgumentParser {
 };
 
 bool ArgumentParser::ParseArguments() {
+  if (arguments_.size() > signature_.size()) {
+    *error_ = api_errors::TooManyArguments();
+    return false;
+  }
+
   bool signature_has_callback = HasCallback(signature_);
 
   size_t end_size =
@@ -170,8 +180,10 @@ bool ArgumentParser::ParseArguments() {
   if (signature_has_callback && !ParseCallback(*signature_.back()))
     return false;
 
-  if (current_index_ != arguments_.size())
+  if (current_index_ != arguments_.size()) {
+    *error_ = api_errors::TooManyArguments();

[jbroman, 2017/04/27 19:37:20]

Can this happen when the previous check for too many arguments succeeded? If so,
maybe a comment since it isn't immediately apparent to me.

[Devlin, 2017/05/01 22:22:09]

Added.

    // This can potentially happen even if the check above for too many
    // arguments succeeds when optional parameters are omitted. For instance,
    // if the signature expects (optional int, function callback) and the caller
    // provides (function callback, object random), the first size check and
    // callback spec would succeed, but we wouldn't consume all the arguments.

     return false;  // Extra arguments aren't allowed.
+  }
 
   return true;
 }
@@ -180,7 +192,7 @@ bool ArgumentParser::ParseArgument(const ArgumentSpec& spec) {
   v8::Local<v8::Value> value = next_argument();
   if (value.IsEmpty() || value->IsNull() || value->IsUndefined()) {
     if (!spec.optional()) {
-      *error_ = "Missing required argument: " + spec.name();
+      *error_ = api_errors::MissingRequiredArgument(spec.name().c_str());

[lazyboy, 2017/04/27 20:55:03]

Here and line 209, remove c_str().

[Devlin, 2017/05/01 22:22:09]

MissingRequiredArgument() takes a const char* (and only needs the char* since
it's passed directly to printf).  Any reason to prefer passing the full string?

[lazyboy, 2017/05/01 23:14:21]

I think I mistakenly read it as *error_ = some_string.c_str();

Any reason to prefer passing the full string?
Nope.

       return false;
     }
     // This is safe to call even if |arguments| is at the end (which can happen
@@ -191,9 +203,10 @@ bool ArgumentParser::ParseArgument(const ArgumentSpec& spec) {
     return true;
   }
 
-  if (!spec.ParseArgument(context_, value, type_refs_, GetBuffer(), error_)) {
+  if (!spec.ParseArgument(context_, value, type_refs_, GetBuffer(),
+                          &parse_error_)) {
     if (!spec.optional()) {
-      *error_ = "Missing required argument: " + spec.name();
+      *error_ = api_errors::ArgumentError(spec.name(), parse_error_);
       return false;
     }
 
@@ -210,7 +223,7 @@ bool ArgumentParser::ParseCallback(const ArgumentSpec& spec) {
   v8::Local<v8::Value> value = next_argument();
   if (value.IsEmpty() || value->IsNull() || value->IsUndefined()) {
     if (!spec.optional()) {
-      *error_ = "Missing required argument: " + spec.name();
+      *error_ = api_errors::MissingRequiredArgument(spec.name().c_str());
       return false;
     }
     ConsumeArgument();
@@ -218,8 +231,9 @@ bool ArgumentParser::ParseCallback(const ArgumentSpec& spec) {
     return true;
   }
 
-  if (!value->IsFunction()) {
-    *error_ = "Argument is wrong type: " + spec.name();
+  if (!spec.ParseArgument(context_, value, type_refs_, nullptr,
+                          &parse_error_)) {
+    *error_ = api_errors::ArgumentError(spec.name(), parse_error_);
     return false;
   }