Allow "null" to mean optional in extension APIs (for realz), but normalise it

for browser implementations by stripping it from objects.

The following are all now exactly the same, where previously they weren't and
would either not work or crash the browser/renderer:

chrome.tabs.create({});
chrome.tabs.create({windowId: undefined});
chrome.tabs.create({windowId: null});

BUG=129411
TEST=as in bug

Review URL: https://chromiumcodereview.appspot.com/10456010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@139510 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 85 insertions, 7 deletions

diff --git a/chrome/renderer/extensions/send_request_natives.cc b/chrome/renderer/extensions/send_request_natives.cc
index 5bf48e5..d17fc0a 100644
--- a/chrome/renderer/extensions/send_request_natives.cc
+++ b/chrome/renderer/extensions/send_request_natives.cc
@@ -41,7 +41,13 @@ v8::Handle<v8::Value> SendRequestNatives::StartRequest(
   bool for_io_thread = args[4]->BooleanValue();
 
   scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create());
-  converter->SetUndefinedAllowed(true);  // Allow passing optional values.
+
+  // Make "undefined" the same as "null" for optional arguments, but for objects
+  // strip nulls (like {foo: null}, and therefore {foo: undefined} as well) to
+  // make it easier for extension APIs to check for optional arguments.
+  converter->SetUndefinedAllowed(true);
+  converter->SetStripNullFromObjects(true);
+
   scoped_ptr<Value> value_args(
       converter->FromV8Value(args[1], v8::Context::GetCurrent()));
   if (!value_args.get() || !value_args->IsType(Value::TYPE_LIST)) {
diff --git a/chrome/renderer/resources/extensions/json_schema.js b/chrome/renderer/resources/extensions/json_schema.js
index ec4b48d..34e2378 100644
--- a/chrome/renderer/resources/extensions/json_schema.js
+++ b/chrome/renderer/resources/extensions/json_schema.js
@@ -50,6 +50,10 @@ function isInstanceOfClass(instance, className) {
   return isInstanceOfClass(Object.getPrototypeOf(instance), className);
 }
 
+function isOptionalValue(value) {
+  return typeof(value) === 'undefined' || value === null;
+}
+
 /**
  * Validates an instance against a schema and accumulates errors. Usage:
  *
@@ -177,6 +181,10 @@ chromeHidden.JSONSchemaValidator.prototype.getAllTypesForSchema =
  */
 chromeHidden.JSONSchemaValidator.prototype.isValidSchemaType =
     function(type, schema) {
+  if (type == 'any')
+    return true;
+
+  // TODO(kalman): I don't understand this code. How can type be "null"?
   if (schema.optional && (type == "null" || type == "undefined"))
     return true;
 
@@ -185,7 +193,8 @@ chromeHidden.JSONSchemaValidator.prototype.isValidSchemaType =
     if (schemaTypes[i] == "any" || type == schemaTypes[i])
       return true;
   }
-  return type == "any";
+
+  return false;
 };
 
 /**
@@ -328,7 +337,7 @@ chromeHidden.JSONSchemaValidator.prototype.validateObject =
       var propPath = path ? path + "." + prop : prop;
       if (schema.properties[prop] == undefined) {
         this.addError(propPath, "invalidPropertyType");
-      } else if (prop in instance && instance[prop] !== undefined) {
+      } else if (prop in instance && !isOptionalValue(instance[prop])) {
         this.validate(instance[prop], schema.properties[prop], propPath);
       } else if (!schema.properties[prop].optional) {
         this.addError(propPath, "propertyRequired");
@@ -397,7 +406,7 @@ chromeHidden.JSONSchemaValidator.prototype.validateArray =
     // validate against the corresponding schema.
     for (var i = 0; i < schema.items.length; i++) {
       var itemPath = path ? path + "." + i : String(i);
-      if (i in instance && instance[i] !== null && instance[i] !== undefined) {
+      if (i in instance && !isOptionalValue(instance[i])) {
         this.validate(instance[i], schema.items[i], itemPath);
       } else if (!schema.items[i].optional) {
         this.addError(itemPath, "itemRequired");
diff --git a/chrome/test/data/extensions/json_schema_test.js b/chrome/test/data/extensions/json_schema_test.js
index d7340d1..9619b4f 100644
--- a/chrome/test/data/extensions/json_schema_test.js
+++ b/chrome/test/data/extensions/json_schema_test.js
@@ -214,8 +214,7 @@ function testObject() {
   schema.properties.bar.optional = true;
   assertValid("Object", {foo:"foo", bar:42}, schema);
   assertValid("Object", {foo:"foo"}, schema);
-  assertNotValid("Object", {foo:"foo", bar:null}, schema,
-                 [formatError("invalidType", ["integer", "null"])]);
+  assertValid("Object", {foo:"foo", bar:null}, schema);
   assertValid("Object", {foo:"foo", bar:undefined}, schema);
   assertNotValid("Object", {foo:"foo", bar:"42"}, schema,
                  [formatError("invalidType", ["integer", "string"])]);
diff --git a/content/public/renderer/v8_value_converter.h b/content/public/renderer/v8_value_converter.h
index bed8cdb..be45ea9 100644
--- a/content/public/renderer/v8_value_converter.h
+++ b/content/public/renderer/v8_value_converter.h
@@ -39,6 +39,10 @@ class CONTENT_EXPORT V8ValueConverter {
   virtual bool GetRegexpAllowed() const = 0;
   virtual void SetRegexpAllowed(bool val) = 0;
 
+  // Gets/sets whether to treat undefined or null in objects as nonexistent.
+  virtual bool GetStripNullFromObjects() const = 0;
+  virtual void SetStripNullFromObjects(bool val) = 0;
+
   // Converts Value to v8::Value. Unsupported types are replaced with null.
   // If an array or object throws while setting a value, that property or item
   // is skipped, leaving a hole in the case of arrays.
diff --git a/content/renderer/v8_value_converter_impl.cc b/content/renderer/v8_value_converter_impl.cc
index 9161196..a21c4f0 100644
--- a/content/renderer/v8_value_converter_impl.cc
+++ b/content/renderer/v8_value_converter_impl.cc
@@ -31,7 +31,8 @@ V8ValueConverter* V8ValueConverter::create() {
 V8ValueConverterImpl::V8ValueConverterImpl()
     : undefined_allowed_(false),
       date_allowed_(false),
-      regexp_allowed_(false) {
+      regexp_allowed_(false),
+      strip_null_from_objects_(false) {
 }
 
 bool V8ValueConverterImpl::GetUndefinedAllowed() const {
@@ -58,6 +59,14 @@ void V8ValueConverterImpl::SetRegexpAllowed(bool val) {
   regexp_allowed_ = val;
 }
 
+bool V8ValueConverterImpl::GetStripNullFromObjects() const {
+  return strip_null_from_objects_;
+}
+
+void V8ValueConverterImpl::SetStripNullFromObjects(bool val) {
+  strip_null_from_objects_ = val;
+}
+
 v8::Handle<v8::Value> V8ValueConverterImpl::ToV8Value(
     const Value* value, v8::Handle<v8::Context> context) const {
   v8::Context::Scope context_scope(context);
@@ -293,6 +302,29 @@ DictionaryValue* V8ValueConverterImpl::FromV8Object(
     Value* child = FromV8ValueImpl(child_v8);
     CHECK(child);
 
+    // Strip null if asked (and since undefined is turned into null, undefined
+    // too). The use case for supporting this is JSON-schema support,
+    // specifically for extensions, where "optional" JSON properties may be
+    // represented as null, yet due to buggy legacy code elsewhere isn't
+    // treated as such (potentially causing crashes). For example, the
+    // "tabs.create" function takes an object as its first argument with an
+    // optional "windowId" property.
+    //
+    // Given just
+    //
+    //   tabs.create({})
+    //
+    // this will work as expected on code that only checks for the existence of
+    // a "windowId" property (such as that legacy code). However given
+    //
+    //   tabs.create({windowId: null})
+    //
+    // there *is* a "windowId" property, but since it should be an int, code
+    // on the browser which doesn't additionally check for null will fail.
+    // We can avoid all bugs related to this by stripping null.
+    if (strip_null_from_objects_ && child->IsType(Value::TYPE_NULL))
+      continue;
+
     result->SetWithoutPathExpansion(std::string(*name_utf8, name_utf8.length()),
                                     child);
   }
diff --git a/content/renderer/v8_value_converter_impl.h b/content/renderer/v8_value_converter_impl.h
index 7f9baea..7f75899 100644
--- a/content/renderer/v8_value_converter_impl.h
+++ b/content/renderer/v8_value_converter_impl.h
@@ -27,6 +27,8 @@ class CONTENT_EXPORT V8ValueConverterImpl : public content::V8ValueConverter {
   virtual void SetDateAllowed(bool val) OVERRIDE;
   virtual bool GetRegexpAllowed() const OVERRIDE;
   virtual void SetRegexpAllowed(bool val) OVERRIDE;
+  virtual bool GetStripNullFromObjects() const OVERRIDE;
+  virtual void SetStripNullFromObjects(bool val) OVERRIDE;
   virtual v8::Handle<v8::Value> ToV8Value(
       const base::Value* value,
       v8::Handle<v8::Context> context) const OVERRIDE;
@@ -59,6 +61,10 @@ class CONTENT_EXPORT V8ValueConverterImpl : public content::V8ValueConverter {
 
   // If true, we will convet RegExp JavaScript objects to string.
   bool regexp_allowed_;
+
+  // If true, undefined and null values are ignored when converting v8 objects
+  // into Values.
+  bool strip_null_from_objects_;
 };
 
 #endif  // CONTENT_RENDERER_V8_VALUE_CONVERTER_IMPL_H_
diff --git a/content/renderer/v8_value_converter_impl_unittest.cc b/content/renderer/v8_value_converter_impl_unittest.cc
index 79f6105..a56941a 100644
--- a/content/renderer/v8_value_converter_impl_unittest.cc
+++ b/content/renderer/v8_value_converter_impl_unittest.cc
@@ -324,3 +324,25 @@ TEST_F(V8ValueConverterImplTest, Prototype) {
   ASSERT_TRUE(result.get());
   EXPECT_EQ(0u, result->size());
 }
+
+TEST_F(V8ValueConverterImplTest, StripNullFromObjects) {
+  v8::Context::Scope context_scope(context_);
+  v8::HandleScope handle_scope;
+
+  const char* source = "(function() {"
+      "return { foo: undefined, bar: null };"
+      "})();";
+
+  v8::Handle<v8::Script> script(v8::Script::New(v8::String::New(source)));
+  v8::Handle<v8::Object> object = script->Run().As<v8::Object>();
+  ASSERT_FALSE(object.IsEmpty());
+
+  V8ValueConverterImpl converter;
+  converter.SetUndefinedAllowed(true);
+  converter.SetStripNullFromObjects(true);
+
+  scoped_ptr<DictionaryValue> result(
+      static_cast<DictionaryValue*>(converter.FromV8Value(object, context_)));
+  ASSERT_TRUE(result.get());
+  EXPECT_EQ(0u, result->size());
+}