[Extensions Bindings] Add argument parsing errors

extensions/renderer/argument_spec.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/argument_spec.h"
 
 #include "base/memory/ptr_util.h"
-#include "base/strings/stringprintf.h"
+#include "base/strings/string_piece.h"
 #include "base/values.h"
 #include "content/public/child/v8_value_converter.h"
+#include "extensions/renderer/api_invocation_errors.h"
 #include "extensions/renderer/api_type_reference_map.h"
 #include "gin/converter.h"
 #include "gin/dictionary.h"
 
 namespace extensions {
 
 namespace {
 
+// Returns a type string for the given |value|.
+const char* GetV8ValueTypeString(v8::Local<v8::Value> value) {
+  DCHECK(!value.IsEmpty());
+
+  if (value->IsNull())
+    return api_errors::kTypeNull;
+  if (value->IsUndefined())
+    return api_errors::kTypeUndefined;
+  if (value->IsInt32())
+    return api_errors::kTypeInteger;
+  if (value->IsNumber())
+    return api_errors::kTypeDouble;
+  if (value->IsBoolean())
+    return api_errors::kTypeBoolean;
+  if (value->IsString())
+    return api_errors::kTypeString;
+
+  // Note: check IsArray(), IsFunction(), and IsArrayBuffer[View]() before
+  // IsObject() since arrays, functions, and array buffers are objects.
+  if (value->IsArray())
+    return api_errors::kTypeList;
+  if (value->IsFunction())
+    return api_errors::kTypeFunction;
+  if (value->IsArrayBuffer() || value->IsArrayBufferView())
+    return api_errors::kTypeBinary;
+  if (value->IsObject())
+    return api_errors::kTypeObject;
+
+  return "";

[jbroman, 2017/04/26 14:34:26]

This isn't exhaustive (Symbol is missing, at least). Should the default be some
non-empty string? "" seems a little odd. :)

[Devlin, 2017/04/26 16:18:26]

Good point.  Added "other" as a placeholder and a TODO to potentially expand
this to include more types.

+}
+
+// Returns true if |value| is within the bounds specified by |minimum| and
+// |maximum|, populating |error| otherwise.
 template <class T>
-bool ParseFundamentalValueHelper(v8::Local<v8::Value> arg,
-                                 v8::Local<v8::Context> context,
-                                 const base::Optional<int>& minimum,
-                                 const base::Optional<int>& maximum,
-                                 std::unique_ptr<base::Value>* out_value) {
-  T val;
-  if (!gin::Converter<T>::FromV8(context->GetIsolate(), arg, &val))
+bool CheckFundamentalBounds(T value,
+                            const base::Optional<int>& minimum,
+                            const base::Optional<int>& maximum,
+                            std::string* error) {
+  if (minimum && value < *minimum) {
+    *error = api_errors::NumberTooSmall(*minimum);
     return false;
-  if (minimum && val < *minimum)
+  }
+  if (maximum && value > *maximum) {
+    *error = api_errors::NumberTooLarge(*maximum);
     return false;
-  if (maximum && val > *maximum)
-    return false;
-  if (out_value)
-    *out_value = base::MakeUnique<base::Value>(val);
+  }
   return true;
 }
 
 }  // namespace
 
 ArgumentSpec::ArgumentSpec(const base::Value& value)
     : type_(ArgumentType::INTEGER), optional_(false) {
   const base::DictionaryValue* dict = nullptr;
   CHECK(value.GetAsDictionary(&dict));
   dict->GetBoolean("optional", &optional_);
@@ skipping 117 matching lines @@
 }
 
 ArgumentSpec::~ArgumentSpec() {}
 
 bool ArgumentSpec::ParseArgument(v8::Local<v8::Context> context,
                                  v8::Local<v8::Value> value,
                                  const APITypeReferenceMap& refs,
                                  std::unique_ptr<base::Value>* out_value,
                                  std::string* error) const {
   if (type_ == ArgumentType::FUNCTION) {
-    if (!value->IsFunction())
+    if (!value->IsFunction()) {
+      *error = GetInvalidTypeError(value);
       return false;
+    }
 
     if (out_value) {
       // Certain APIs (contextMenus) have functions as parameters other than the
       // callback (contextMenus uses it for an onclick listener). Our generated
       // types have adapted to consider functions "objects" and serialize them
       // as dictionaries.
       // TODO(devlin): It'd be awfully nice to get rid of this eccentricity.
       *out_value = base::MakeUnique<base::DictionaryValue>();
     }
     return true;
   }
 
   if (type_ == ArgumentType::REF) {
     DCHECK(ref_);
     const ArgumentSpec* reference = refs.GetSpec(ref_.value());
     DCHECK(reference) << ref_.value();
     return reference->ParseArgument(context, value, refs, out_value, error);
   }
 
   if (type_ == ArgumentType::CHOICES) {
     for (const auto& choice : choices_) {
       if (choice->ParseArgument(context, value, refs, out_value, error))
         return true;
     }
-    *error = "Did not match any of the choices";
+    *error = api_errors::InvalidChoice();
     return false;
   }
 
   if (IsFundamentalType())
     return ParseArgumentToFundamental(context, value, out_value, error);
   if (type_ == ArgumentType::OBJECT) {
     // Don't allow functions or arrays (even though they are technically
     // objects). This is to make it easier to match otherwise-ambiguous
     // signatures. For instance, if an API method has an optional object
     // parameter and then an optional callback, we wouldn't necessarily be able
     // to match the arguments if we allowed functions as objects.
     if (!value->IsObject() || value->IsFunction() || value->IsArray()) {
-      *error = "Wrong type";
+      *error = GetInvalidTypeError(value);
       return false;
     }
     v8::Local<v8::Object> object = value.As<v8::Object>();
     return ParseArgumentToObject(context, object, refs, out_value, error);
   }
   if (type_ == ArgumentType::LIST) {
     if (!value->IsArray()) {
-      *error = "Wrong type";
+      *error = GetInvalidTypeError(value);
       return false;
     }
     v8::Local<v8::Array> array = value.As<v8::Array>();
     return ParseArgumentToArray(context, array, refs, out_value, error);
   }
   if (type_ == ArgumentType::BINARY) {
     if (!value->IsArrayBuffer() && !value->IsArrayBufferView()) {
-      *error = "Wrong type";
+      *error = GetInvalidTypeError(value);
       return false;
     }
     return ParseArgumentToAny(context, value, out_value, error);
   }
   if (type_ == ArgumentType::ANY)
     return ParseArgumentToAny(context, value, out_value, error);
   NOTREACHED();
   return false;
 }
 
 bool ArgumentSpec::IsFundamentalType() const {
   return type_ == ArgumentType::INTEGER || type_ == ArgumentType::DOUBLE ||
          type_ == ArgumentType::BOOLEAN || type_ == ArgumentType::STRING;
 }
 
 bool ArgumentSpec::ParseArgumentToFundamental(
     v8::Local<v8::Context> context,
     v8::Local<v8::Value> value,
     std::unique_ptr<base::Value>* out_value,
     std::string* error) const {
   DCHECK(IsFundamentalType());
+
   switch (type_) {
-    case ArgumentType::INTEGER:
-      return ParseFundamentalValueHelper<int32_t>(value, context, minimum_,
-                                                  maximum_, out_value);
-    case ArgumentType::DOUBLE:
-      return ParseFundamentalValueHelper<double>(value, context, minimum_,
-                                                 maximum_, out_value);
+    case ArgumentType::INTEGER: {
+      if (!value->IsInt32()) {
+        *error = GetInvalidTypeError(value);
+        return false;
+      }
+      int int_val = value.As<v8::Int32>()->Value();
+      if (!CheckFundamentalBounds(int_val, minimum_, maximum_, error))
+        return false;
+      if (out_value)
+        *out_value = base::MakeUnique<base::Value>(int_val);
+      return true;
+    }
+    case ArgumentType::DOUBLE: {
+      if (!value->IsNumber()) {
+        *error = GetInvalidTypeError(value);
+        return false;
+      }
+      double double_val = value.As<v8::Number>()->Value();
+      if (!CheckFundamentalBounds(double_val, minimum_, maximum_, error))
+        return false;
+      if (out_value)
+        *out_value = base::MakeUnique<base::Value>(double_val);
+      return true;
+    }
     case ArgumentType::STRING: {
-      if (!value->IsString())
+      if (!value->IsString()) {
+        *error = GetInvalidTypeError(value);
         return false;
+      }
 
       v8::Local<v8::String> v8_string = value.As<v8::String>();
       size_t length = static_cast<size_t>(v8_string->Length());
       if (min_length_ && length < *min_length_) {
-        *error = "Less than min length";
+        *error = api_errors::TooFewStringChars(*min_length_);
         return false;
       }
 
       if (max_length_ && length > *max_length_) {
-        *error = "Greater than max length";
+        *error = api_errors::TooManyStringChars(*max_length_);
         return false;
       }
 
       // If we don't need to match enum values and don't need to convert, we're
       // done...
       if (!out_value && enum_values_.empty())
         return true;
       // ...Otherwise, we need to convert to a std::string.
       std::string s;
       // We already checked that this is a string, so this should never fail.
       CHECK(gin::Converter<std::string>::FromV8(context->GetIsolate(), value,
                                                 &s));
-      if (!enum_values_.empty() && enum_values_.count(s) == 0)
+      if (!enum_values_.empty() && enum_values_.count(s) == 0) {
+        *error = api_errors::InvalidEnumValue(enum_values_);
         return false;
+      }
       if (out_value) {
         // TODO(devlin): If base::Value ever takes a std::string&&, we
         // could use std::move to construct.
         *out_value = base::MakeUnique<base::Value>(s);
       }
       return true;
     }
     case ArgumentType::BOOLEAN: {
-      if (!value->IsBoolean())
+      if (!value->IsBoolean()) {
+        *error = GetInvalidTypeError(value);
         return false;
+      }
       if (out_value) {
         *out_value =
             base::MakeUnique<base::Value>(value.As<v8::Boolean>()->Value());
       }
       return true;
     }
     default:
       NOTREACHED();
   }
   return false;
 }
 
 bool ArgumentSpec::ParseArgumentToObject(
     v8::Local<v8::Context> context,
     v8::Local<v8::Object> object,
     const APITypeReferenceMap& refs,
     std::unique_ptr<base::Value>* out_value,
     std::string* error) const {
   DCHECK_EQ(ArgumentType::OBJECT, type_);
   std::unique_ptr<base::DictionaryValue> result;
   // Only construct the result if we have an |out_value| to populate.
   if (out_value)
     result = base::MakeUnique<base::DictionaryValue>();
 
   v8::Local<v8::Array> own_property_names;
-  if (!object->GetOwnPropertyNames(context).ToLocal(&own_property_names))
+  if (!object->GetOwnPropertyNames(context).ToLocal(&own_property_names)) {
+    *error = api_errors::ScriptThrewError();
     return false;
+  }
 
   // Track all properties we see from |properties_| to check if any are missing.
   // Use ArgumentSpec* instead of std::string for comparison + copy efficiency.
   std::set<const ArgumentSpec*> seen_properties;
   uint32_t length = own_property_names->Length();
+  std::string property_error;
   for (uint32_t i = 0; i < length; ++i) {
     v8::Local<v8::Value> key;
-    if (!own_property_names->Get(context, i).ToLocal(&key))
+    if (!own_property_names->Get(context, i).ToLocal(&key)) {
+      *error = api_errors::ScriptThrewError();
       return false;
+    }
     // In JS, all keys are strings or numbers (or symbols, but those are
     // excluded by GetOwnPropertyNames()). If you try to set anything else
     // (e.g. an object), it is converted to a string.
     DCHECK(key->IsString() || key->IsNumber());
     v8::String::Utf8Value utf8_key(key);
 
     ArgumentSpec* property_spec = nullptr;
     auto iter = properties_.find(*utf8_key);
     if (iter != properties_.end()) {
       property_spec = iter->second.get();
       seen_properties.insert(property_spec);
     } else if (additional_properties_) {
       property_spec = additional_properties_.get();
     } else {
-      *error = base::StringPrintf("Unknown property: %s", *utf8_key);
+      *error = api_errors::UnexpectedProperty(*utf8_key);
       return false;
     }
 
     v8::Local<v8::Value> prop_value;
     // Fun: It's possible that a previous getter has removed the property from
     // the object. This isn't that big of a deal, since it would only manifest
     // in the case of some reasonably-crazy script objects, and it's probably
     // not worth optimizing for the uncommon case to the detriment of the
     // common (and either should be totally safe). We can always add a
     // HasOwnProperty() check here in the future, if we desire.
     // See also comment in ParseArgumentToArray() about passing in custom
     // crazy values here.
     if (!object->Get(context, key).ToLocal(&prop_value))
       return false;
 
     // Note: We don't serialize undefined or null values.
     // TODO(devlin): This matches current behavior, but it is correct?
     if (prop_value->IsUndefined() || prop_value->IsNull()) {
       if (!property_spec->optional_) {
-        *error = base::StringPrintf("Missing key: %s", *utf8_key);
+        *error = api_errors::MissingRequiredProperty(*utf8_key);
         return false;
       }
       continue;
     }
 
     std::unique_ptr<base::Value> property;
     if (!property_spec->ParseArgument(context, prop_value, refs,
-                                      result ? &property : nullptr, error)) {
+                                      result ? &property : nullptr,
+                                      &property_error)) {
+      *error = api_errors::PropertyError(*utf8_key, property_error);
       return false;
     }
     if (result)
       result->SetWithoutPathExpansion(*utf8_key, std::move(property));
   }
 
   for (const auto& pair : properties_) {
     const ArgumentSpec* spec = pair.second.get();
     if (!spec->optional_ && seen_properties.count(spec) == 0) {
-      *error = "Missing key: " + pair.first;
+      *error = api_errors::MissingRequiredProperty(pair.first.c_str());
       return false;
     }
   }
 
   if (instance_of_) {
     // Check for the instance somewhere in the object's prototype chain.
     // NOTE: This only checks that something in the prototype chain was
     // constructed with the same name as the desired instance, but doesn't
     // validate that it's the same constructor as the expected one. For
     // instance, if we expect isInstanceOf == 'Date', script could pass in
@@ skipping 12 matching lines @@
       v8::String::Utf8Value constructor(current->GetConstructorName());
       if (*instance_of_ ==
           base::StringPiece(*constructor, constructor.length())) {
         found = true;
         break;
       }
       next_check = current->GetPrototype();
     } while (next_check->IsObject());
 
     if (!found) {
-      *error = "Object is not of correct instance";
+      *error = api_errors::NotAnInstance(instance_of_->c_str());
       return false;
     }
   }
 
   if (out_value)
     *out_value = std::move(result);
   return true;
 }
 
 bool ArgumentSpec::ParseArgumentToArray(v8::Local<v8::Context> context,
                                         v8::Local<v8::Array> value,
                                         const APITypeReferenceMap& refs,
                                         std::unique_ptr<base::Value>* out_value,
                                         std::string* error) const {
   DCHECK_EQ(ArgumentType::LIST, type_);
 
   uint32_t length = value->Length();
   if (min_length_ && length < *min_length_) {
-    *error = "Less than min length";
+    *error = api_errors::TooFewArrayItems(*min_length_);
     return false;
   }
 
   if (max_length_ && length > *max_length_) {
-    *error = "Greater than max length";
+    *error = api_errors::TooManyArrayItems(*max_length_);
     return false;
   }
 
   std::unique_ptr<base::ListValue> result;
   // Only construct the result if we have an |out_value| to populate.
   if (out_value)
     result = base::MakeUnique<base::ListValue>();
 
+  std::string item_error;
   for (uint32_t i = 0; i < length; ++i) {
     v8::MaybeLocal<v8::Value> maybe_subvalue = value->Get(context, i);
     v8::Local<v8::Value> subvalue;
     // Note: This can fail in the case of a developer passing in the following:
     // var a = [];
     // Object.defineProperty(a, 0, { get: () => { throw new Error('foo'); } });
     // Currently, this will cause the developer-specified error ('foo') to be
     // thrown.
     // TODO(devlin): This is probably fine, but it's worth contemplating
     // catching the error and throwing our own.
     if (!maybe_subvalue.ToLocal(&subvalue))
       return false;
     std::unique_ptr<base::Value> item;
-    if (!list_element_type_->ParseArgument(context, subvalue, refs,
-                                           result ? &item : nullptr, error)) {
+    if (!list_element_type_->ParseArgument(
+            context, subvalue, refs, result ? &item : nullptr, &item_error)) {
+      *error = api_errors::IndexError(i, item_error);
       return false;
     }
     if (result)
       result->Append(std::move(item));
   }
   if (out_value)
     *out_value = std::move(result);
   return true;
 }
 
 bool ArgumentSpec::ParseArgumentToAny(v8::Local<v8::Context> context,
                                       v8::Local<v8::Value> value,
                                       std::unique_ptr<base::Value>* out_value,
                                       std::string* error) const {
   DCHECK(type_ == ArgumentType::ANY || type_ == ArgumentType::BINARY);
   if (out_value) {
     std::unique_ptr<content::V8ValueConverter> converter(
         content::V8ValueConverter::create());
     std::unique_ptr<base::Value> converted(
         converter->FromV8Value(value, context));
     if (!converted) {
-      *error = "Could not convert to 'any'.";
+      *error = api_errors::UnserializableValue();
       return false;
     }
     if (type_ == ArgumentType::BINARY)
       DCHECK_EQ(base::Value::Type::BINARY, converted->GetType());
     *out_value = std::move(converted);
   }
   return true;
 }
 
+std::string ArgumentSpec::GetInvalidTypeError(
+    v8::Local<v8::Value> value) const {
+  const char* expected_type = nullptr;
+  switch (type_) {
+    case ArgumentType::INTEGER:
+      expected_type = api_errors::kTypeInteger;
+      break;
+    case ArgumentType::DOUBLE:
+      expected_type = api_errors::kTypeDouble;
+      break;
+    case ArgumentType::BOOLEAN:
+      expected_type = api_errors::kTypeBoolean;
+      break;
+    case ArgumentType::STRING:
+      expected_type = api_errors::kTypeString;
+      break;
+    case ArgumentType::OBJECT:
+      expected_type =
+          instance_of_ ? instance_of_->c_str() : api_errors::kTypeObject;
+      break;
+    case ArgumentType::LIST:
+      expected_type = api_errors::kTypeList;
+      break;
+    case ArgumentType::BINARY:
+      expected_type = api_errors::kTypeBinary;
+      break;
+    case ArgumentType::FUNCTION:
+      expected_type = api_errors::kTypeFunction;
+      break;
+    case ArgumentType::REF:
+      expected_type = ref_->c_str();
+      break;
+    case ArgumentType::CHOICES:
+    case ArgumentType::ANY:
+      NOTREACHED();
+  }
+
+  return api_errors::InvalidType(expected_type, GetV8ValueTypeString(value));
+}
+
 }  // namespace extensions