[Extensions Bindings] Add errors to signature parsing

extensions/renderer/api_signature.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/renderer/api_signature.h"
 
 #include <algorithm>
 
 #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"
 
 namespace extensions {
 
 namespace {
 
 bool HasCallback(const std::vector<std::unique_ptr<ArgumentSpec>>& signature) {
   // TODO(devlin): This is how extension APIs have always determined if a
   // function has a callback, but it seems a little silly. In the long run (once
@@ skipping 55 matching lines @@
   // Adds the parsed callback.
   virtual void SetCallback(v8::Local<v8::Function> callback) = 0;
 
   v8::Local<v8::Context> context_;
   const std::vector<std::unique_ptr<ArgumentSpec>>& signature_;
   const std::vector<v8::Local<v8::Value>>& arguments_;
   const APITypeReferenceMap& type_refs_;
   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);
 };
 
 class V8ArgumentParser : public ArgumentParser {
  public:
   V8ArgumentParser(v8::Local<v8::Context> context,
                    const std::vector<std::unique_ptr<ArgumentSpec>>& signature,
                    const std::vector<v8::Local<v8::Value>>& arguments,
                    const APITypeReferenceMap& type_refs,
                    std::string* error,
@@ skipping 54 matching lines @@
   }
 
   base::ListValue* list_value_;
   std::unique_ptr<base::Value> last_arg_;
   v8::Local<v8::Function> callback_;
 
   DISALLOW_COPY_AND_ASSIGN(BaseValueArgumentParser);
 };
 
 bool ArgumentParser::ParseArguments() {
+  if (arguments_.size() > signature_.size()) {
+    *error_ = api_errors::TooManyArguments();
+    return false;
+  }
+
   bool signature_has_callback = HasCallback(signature_);
 
   size_t end_size =
       signature_has_callback ? signature_.size() - 1 : signature_.size();
   for (size_t i = 0; i < end_size; ++i) {
     if (!ParseArgument(*signature_[i]))
       return false;
   }
 
   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;
 }
 
 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
     // if n optional arguments are omitted at the end of the signature).
     ConsumeArgument();
 
     AddNull();
     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;
     }
 
     AddNull();
     return true;
   }
 
   ConsumeArgument();
   AddParsedArgument(value);
   return true;
 }
 
 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();
     AddNullCallback();
     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;
   }
 
   ConsumeArgument();
   SetCallback(value.As<v8::Function>());
   return true;
 }
 
 }  // namespace
 
@@ skipping 80 matching lines @@
       return false;
     json->Append(std::move(converted));
   }
 
   *json_out = std::move(json);
   *callback_out = callback;
   return true;
 }
 
 }  // namespace extensions