Make V8ValueConverter.FromV8Value be more consistent with JSON.stringify: don't

serialize undefined as null, don't serialize functions as objects, omit values
from objects when they don't serialize, and insert null into arrays when they
don't serialize.

It is now possible for FromV8Value to return NULL; previously it would return
Value::CreateNullValue on failure.

This is needed for the Storage API, where we promise that the values that are
passed in are serialized as JSON, yet the value conversion doesn't work in a
way that allows it.

However, the null-stripping behavior needs to be configurable so that existing
extension APIs (which only expect null/undefined to appear for optional values)
still work.

BUG=145081

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

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

13 files changed, 274 insertions, 147 deletions

diff --git a/chrome/renderer/extensions/event_bindings.cc b/chrome/renderer/extensions/event_bindings.cc
index 329a70b..94258db 100644
--- a/chrome/renderer/extensions/event_bindings.cc
+++ b/chrome/renderer/extensions/event_bindings.cc
@@ -191,6 +191,8 @@ class ExtensionImpl : public ChromeV8Extension {
     base::DictionaryValue* filter_dict = NULL;
     base::Value* filter_value = converter->FromV8Value(args[1]->ToObject(),
         v8::Context::GetCurrent());
+    if (!filter_value)
+      return v8::Integer::New(-1);
     if (!filter_value->GetAsDictionary(&filter_dict)) {
       delete filter_value;
       return v8::Integer::New(-1);
diff --git a/chrome/renderer/extensions/send_request_natives.cc b/chrome/renderer/extensions/send_request_natives.cc
index a5a8834..b788a8a 100644
--- a/chrome/renderer/extensions/send_request_natives.cc
+++ b/chrome/renderer/extensions/send_request_natives.cc
@@ -37,14 +37,16 @@ v8::Handle<v8::Value> SendRequestNatives::StartRequest(
   int request_id = args[2]->Int32Value();
   bool has_callback = args[3]->BooleanValue();
   bool for_io_thread = args[4]->BooleanValue();
+  bool preserve_null_in_objects = args[5]->BooleanValue();
 
   scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create());
 
-  // 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);
+  // See http://crbug.com/149880. The context menus APIs relies on this, but
+  // we shouln't really be doing it (e.g. for the sake of the storage API).
+  converter->SetFunctionAllowed(true);
+
+  if (!preserve_null_in_objects)
+    converter->SetStripNullFromObjects(true);
 
   scoped_ptr<Value> value_args(
       converter->FromV8Value(args[1], v8::Context::GetCurrent()));
diff --git a/chrome/renderer/extensions/user_script_scheduler.cc b/chrome/renderer/extensions/user_script_scheduler.cc
index 3d98aea..fec5a98 100644
--- a/chrome/renderer/extensions/user_script_scheduler.cc
+++ b/chrome/renderer/extensions/user_script_scheduler.cc
@@ -203,7 +203,6 @@ void UserScriptScheduler::ExecuteCodeImpl(
 
       scoped_ptr<content::V8ValueConverter> v8_converter(
           content::V8ValueConverter::create());
-      v8_converter->SetUndefinedAllowed(true);
       v8::Handle<v8::Value> script_value;
       if (params.in_main_world) {
         script_value = child_frame->executeScriptAndReturnValue(source);
@@ -223,7 +222,10 @@ void UserScriptScheduler::ExecuteCodeImpl(
       if (!script_value.IsEmpty()) {
         base::Value* base_val =
             v8_converter->FromV8Value(script_value, context);
-        execution_results.Append(base_val);
+        // Always append an execution result (i.e. no result == null result) so
+        // that |execution_results| lines up with the frames.
+        execution_results.Append(base_val ? base_val :
+                                            base::Value::CreateNullValue());
         script_value.Clear();
       }
     } else {
diff --git a/chrome/renderer/resources/extensions/context_menus_custom_bindings.js b/chrome/renderer/resources/extensions/context_menus_custom_bindings.js
index 9e4016a..e2629f7 100644
--- a/chrome/renderer/resources/extensions/context_menus_custom_bindings.js
+++ b/chrome/renderer/resources/extensions/context_menus_custom_bindings.js
@@ -48,10 +48,10 @@ chromeHidden.registerCustomHook('contextMenus', function(bindingsAPI) {
     var args = arguments;
     var id = GetNextContextMenuId();
     args[0].generatedId = id;
-    sendRequest(this.name,
-                args,
-                this.definition.parameters,
-                {customCallback: this.customCallback});
+    var optArgs = {
+      customCallback: this.customCallback,
+    };
+    sendRequest(this.name, args, this.definition.parameters, optArgs);
     return chromeHidden.contextMenus.getIdFromCreateProperties(args[0]);
   });
 
diff --git a/chrome/renderer/resources/extensions/schema_generated_bindings.js b/chrome/renderer/resources/extensions/schema_generated_bindings.js
index ecf46f3..76bda1e 100644
--- a/chrome/renderer/resources/extensions/schema_generated_bindings.js
+++ b/chrome/renderer/resources/extensions/schema_generated_bindings.js
@@ -311,9 +311,12 @@
             if (this.handleRequest) {
               retval = this.handleRequest.apply(this, args);
             } else {
+              var optArgs = {
+                customCallback: this.customCallback
+              };
               retval = sendRequest(this.name, args,
                                    this.definition.parameters,
-                                   {customCallback: this.customCallback});
+                                   optArgs);
             }
 
             // Validate return value if defined - only in debug.
diff --git a/chrome/renderer/resources/extensions/send_request.js b/chrome/renderer/resources/extensions/send_request.js
index 1d7231f..5aeb995 100644
--- a/chrome/renderer/resources/extensions/send_request.js
+++ b/chrome/renderer/resources/extensions/send_request.js
@@ -77,7 +77,7 @@ function prepareRequest(args, argSchemas) {
 }
 
 // Send an API request and optionally register a callback.
-// |opt_args| is an object with optional parameters as follows:
+// |optArgs| is an object with optional parameters as follows:
 // - noStringify: true if we should not stringify the request arguments.
 // - customCallback: a callback that should be called instead of the standard
 //   callback.
@@ -85,12 +85,13 @@ function prepareRequest(args, argSchemas) {
 //   StartRequest if missing.
 // - forIOThread: true if this function should be handled on the browser IO
 //   thread.
-function sendRequest(functionName, args, argSchemas, opt_args) {
-  if (!opt_args)
-    opt_args = {};
+// - preserveNullInObjects: true if it is safe for null to be in objects.
+function sendRequest(functionName, args, argSchemas, optArgs) {
+  if (!optArgs)
+    optArgs = {};
   var request = prepareRequest(args, argSchemas);
-  if (opt_args.customCallback) {
-    request.customCallback = opt_args.customCallback;
+  if (optArgs.customCallback) {
+    request.customCallback = optArgs.customCallback;
   }
   // JSON.stringify doesn't support a root object which is undefined.
   if (request.args === undefined)
@@ -99,19 +100,22 @@ function sendRequest(functionName, args, argSchemas, opt_args) {
   // TODO(asargent) - convert all optional native functions to accept raw
   // v8 values instead of expecting JSON strings.
   var doStringify = false;
-  if (opt_args.nativeFunction && !opt_args.noStringify)
+  if (optArgs.nativeFunction && !optArgs.noStringify)
     doStringify = true;
   var requestArgs = doStringify ?
       chromeHidden.JSON.stringify(request.args) : request.args;
-  var nativeFunction = opt_args.nativeFunction || natives.StartRequest;
+  var nativeFunction = optArgs.nativeFunction || natives.StartRequest;
 
   var requestId = natives.GetNextRequestId();
   request.id = requestId;
   requests[requestId] = request;
-  var hasCallback =
-      (request.callback || opt_args.customCallback) ? true : false;
-  return nativeFunction(functionName, requestArgs, requestId, hasCallback,
-                        opt_args.forIOThread);
+  var hasCallback = request.callback || optArgs.customCallback;
+  return nativeFunction(functionName,
+                        requestArgs,
+                        requestId,
+                        hasCallback,
+                        optArgs.forIOThread,
+                        optArgs.preserveNullInObjects);
 }
 
 exports.sendRequest = sendRequest;
diff --git a/chrome/renderer/resources/extensions/storage_custom_bindings.js b/chrome/renderer/resources/extensions/storage_custom_bindings.js
index 9b0ece5..d230099 100644
--- a/chrome/renderer/resources/extensions/storage_custom_bindings.js
+++ b/chrome/renderer/resources/extensions/storage_custom_bindings.js
@@ -33,7 +33,8 @@ chromeHidden.registerCustomType('storage.StorageArea', function() {
         return sendRequest(
             'storage.' + functionName,
             [namespace].concat(args),
-            extendSchema(funSchema.definition.parameters));
+            extendSchema(funSchema.definition.parameters),
+            {preserveNullInObjects: true});
       };
     }
     var apiFunctions = ['get', 'set', 'remove', 'clear', 'getBytesInUse'];
diff --git a/chrome/test/data/extensions/api_test/settings/simple_test/background.js b/chrome/test/data/extensions/api_test/settings/simple_test/background.js
index 0ac0b5e..02fe318 100644
--- a/chrome/test/data/extensions/api_test/settings/simple_test/background.js
+++ b/chrome/test/data/extensions/api_test/settings/simple_test/background.js
@@ -2,6 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+var assertEq = chrome.test.assertEq;
+var assertTrue = chrome.test.assertTrue;
+var succeed = chrome.test.succeed;
+
 function test(stage0) {
   var apis = [
     chrome.storage.sync,
@@ -19,15 +23,15 @@ chrome.test.runTests([
       this.get('foo', stage1.bind(this));
     }
     function stage1(settings) {
-      chrome.test.assertEq({}, settings);
+      assertEq({}, settings);
       this.get(['foo', 'bar'], stage2.bind(this));
     }
     function stage2(settings) {
-      chrome.test.assertEq({}, settings);
+      assertEq({}, settings);
       this.get(undefined, stage3.bind(this));
     }
     function stage3(settings) {
-      chrome.test.assertEq({}, settings);
+      assertEq({}, settings);
       this.succeed();
     }
     test(stage0);
@@ -45,21 +49,21 @@ chrome.test.runTests([
       this.get(['foo', 'baz'], stage2.bind(this));
     }
     function stage2(settings) {
-      chrome.test.assertEq({
+      assertEq({
         'foo': 'bar',
         'baz': 'qux'
       }, settings);
       this.get(['nothing', 'baz', 'hello', 'ignore'], stage3.bind(this));
     }
     function stage3(settings) {
-      chrome.test.assertEq({
+      assertEq({
         'baz'  : 'qux',
         'hello': 'world'
       }, settings);
       this.get(null, stage4.bind(this));
     }
     function stage4(settings) {
-      chrome.test.assertEq({
+      assertEq({
         'foo'  : 'bar',
         'baz'  : 'qux',
         'hello': 'world'
@@ -94,7 +98,7 @@ chrome.test.runTests([
       this.get(null, stage3.bind(this));
     }
     function stage3(settings) {
-      chrome.test.assertEq({
+      assertEq({
         'baz'  : 'qux',
         'hello': 'world'
       }, settings);
@@ -104,7 +108,7 @@ chrome.test.runTests([
       this.get(null, stage5.bind(this));
     }
     function stage5(settings) {
-      chrome.test.assertEq({
+      assertEq({
         'hello': 'world'
       }, settings);
       this.remove('hello', stage6.bind(this));
@@ -113,7 +117,7 @@ chrome.test.runTests([
       this.get(null, stage7.bind(this));
     }
     function stage7(settings) {
-      chrome.test.assertEq({}, settings);
+      assertEq({}, settings);
       this.succeed();
     }
     test(stage0);
@@ -137,7 +141,7 @@ chrome.test.runTests([
       this.get(null, stage3.bind(this));
     }
     function stage3(settings) {
-      chrome.test.assertEq({
+      assertEq({
         'foo'  : 'otherBar',
         'baz'  : 'otherQux',
         'hello': 'world'
@@ -152,7 +156,7 @@ chrome.test.runTests([
       this.get(null, stage5.bind(this));
     }
     function stage5(settings) {
-      chrome.test.assertEq({
+      assertEq({
         'foo'  : 'otherBar',
         'baz'  : 'anotherQux',
         'hello': 'otherWorld',
@@ -171,7 +175,7 @@ chrome.test.runTests([
       this.get(null, stage2.bind(this));
     }
     function stage2(settings) {
-      chrome.test.assertEq({}, settings);
+      assertEq({}, settings);
       this.succeed();
     }
     test(stage0);
@@ -192,7 +196,7 @@ chrome.test.runTests([
       this.get(null, stage3.bind(this));
     }
     function stage3(settings) {
-      chrome.test.assertEq({}, settings);
+      assertEq({}, settings);
       this.succeed();
     }
     test(stage0);
@@ -209,21 +213,21 @@ chrome.test.runTests([
       this.get(['foo.bar', 'one'], stage2.bind(this));
     }
     function stage2(settings) {
-      chrome.test.assertEq({
+      assertEq({
         'foo.bar' : 'baz',
         'one'     : {'two': 'three'}
       }, settings);
       this.get('one.two', stage3.bind(this));
     }
     function stage3(settings) {
-      chrome.test.assertEq({}, settings);
+      assertEq({}, settings);
       this.remove(['foo.bar', 'one.two'], stage4.bind(this));
     }
     function stage4() {
       this.get(null, stage5.bind(this));
     }
     function stage5(settings) {
-      chrome.test.assertEq({
+      assertEq({
         'one'     : {'two': 'three'}
       }, settings);
       this.succeed();
@@ -239,14 +243,14 @@ chrome.test.runTests([
       }, stage1.bind(this));
     }
     function stage1(settings) {
-      chrome.test.assertEq({
+      assertEq({
         'foo': 'defaultBar',
         'baz': [1, 2, 3]
       }, settings);
       this.get(null, stage2.bind(this));
     }
     function stage2(settings) {
-      chrome.test.assertEq({}, settings);
+      assertEq({}, settings);
       this.set({'foo': 'bar'}, stage3.bind(this));
     }
     function stage3() {
@@ -256,7 +260,7 @@ chrome.test.runTests([
       }, stage4.bind(this));
     }
     function stage4(settings) {
-      chrome.test.assertEq({
+      assertEq({
         'foo': 'bar',
         'baz': [1, 2, 3]
       }, settings);
@@ -269,7 +273,7 @@ chrome.test.runTests([
       }, stage6.bind(this));
     }
     function stage6(settings) {
-      chrome.test.assertEq({
+      assertEq({
         'foo': 'bar',
         'baz': {}
       }, settings);
@@ -282,7 +286,7 @@ chrome.test.runTests([
       }, stage8.bind(this));
     }
     function stage8(settings) {
-      chrome.test.assertEq({
+      assertEq({
         'foo': 'defaultBar',
         'baz': {}
       }, settings);
@@ -295,38 +299,66 @@ chrome.test.runTests([
   function quota() {
     // Just check that the constants are defined; no need to be forced to
     // update them here as well if/when they change.
-    chrome.test.assertTrue(chrome.storage.sync.QUOTA_BYTES > 0);
-    chrome.test.assertTrue(chrome.storage.sync.QUOTA_BYTES_PER_ITEM > 0);
-    chrome.test.assertTrue(chrome.storage.sync.MAX_ITEMS > 0);
+    assertTrue(chrome.storage.sync.QUOTA_BYTES > 0);
+    assertTrue(chrome.storage.sync.QUOTA_BYTES_PER_ITEM > 0);
+    assertTrue(chrome.storage.sync.MAX_ITEMS > 0);
 
-    chrome.test.assertTrue(chrome.storage.local.QUOTA_BYTES > 0);
-    chrome.test.assertEq('undefined',
-                         typeof chrome.storage.local.QUOTA_BYTES_PER_ITEM);
-    chrome.test.assertEq('undefined',
-                         typeof chrome.storage.local.MAX_ITEMS);
+    assertTrue(chrome.storage.local.QUOTA_BYTES > 0);
+    assertEq('undefined', typeof chrome.storage.local.QUOTA_BYTES_PER_ITEM);
+    assertEq('undefined', typeof chrome.storage.local.MAX_ITEMS);
 
     var area = chrome.storage.sync;
     function stage0() {
       area.getBytesInUse(null, stage1);
     }
     function stage1(bytesInUse) {
-      chrome.test.assertEq(0, bytesInUse);
+      assertEq(0, bytesInUse);
       area.set({ a: 42, b: 43, c: 44 }, stage2);
     }
     function stage2() {
       area.getBytesInUse(null, stage3);
     }
     function stage3(bytesInUse) {
-      chrome.test.assertEq(9, bytesInUse);
+      assertEq(9, bytesInUse);
       area.getBytesInUse('a', stage4);
     }
     function stage4(bytesInUse) {
-      chrome.test.assertEq(3, bytesInUse);
+      assertEq(3, bytesInUse);
       area.getBytesInUse(['a', 'b'], stage5);
     }
     function stage5(bytesInUse) {
-      chrome.test.assertEq(6, bytesInUse);
-      chrome.test.succeed();
+      assertEq(6, bytesInUse);
+      succeed();
+    }
+    area.clear(stage0);
+  },
+
+  function nullsInArgs() {
+    var area = chrome.storage.local;
+    function stage0() {
+      area.get({
+        foo: 'foo',
+        bar: null,
+        baz: undefined
+      }, stage1);
+    }
+    function stage1(values) {
+      assertEq({
+        foo: 'foo',
+        bar: null,
+      }, values);
+      area.set({
+        foo: 'foo',
+        bar: null,
+        baz: undefined
+      }, area.get.bind(area, stage2));
+    }
+    function stage2(values) {
+      assertEq({
+        foo: 'foo',
+        bar: null,
+      }, values);
+      succeed();
     }
     area.clear(stage0);
   },
@@ -366,5 +398,5 @@ chrome.test.runTests([
         }));
       }));
     }));
-  }
+  },
 ]);
diff --git a/content/public/renderer/v8_value_converter.h b/content/public/renderer/v8_value_converter.h
index be45ea9..b71b32d 100644
--- a/content/public/renderer/v8_value_converter.h
+++ b/content/public/renderer/v8_value_converter.h
@@ -28,31 +28,48 @@ class CONTENT_EXPORT V8ValueConverter {
 
   virtual ~V8ValueConverter() {}
 
-  // Use the following setters to support additional types other than the
-  // default ones.
-  virtual bool GetUndefinedAllowed() const = 0;
-  virtual void SetUndefinedAllowed(bool val) = 0;
-
-  virtual bool GetDateAllowed() const = 0;
+  // If true, Date objects are converted into DoubleValues with the number of
+  // seconds since Unix epoch.
+  //
+  // Otherwise they are converted into DictionaryValues with whatever additional
+  // properties has been set on them.
   virtual void SetDateAllowed(bool val) = 0;
 
-  virtual bool GetRegexpAllowed() const = 0;
-  virtual void SetRegexpAllowed(bool val) = 0;
+  // If true, RegExp objects are converted into StringValues with the regular
+  // expression between / and /, for example "/ab?c/".
+  //
+  // Otherwise they are converted into DictionaryValues with whatever additional
+  // properties has been set on them.
+  virtual void SetRegExpAllowed(bool val) = 0;
+
+  // If true, Function objects are converted into DictionaryValues with whatever
+  // additional properties has been set on them.
+  //
+  // Otherwise they are treated as unsupported, see FromV8Value.
+  virtual void SetFunctionAllowed(bool val) = 0;
 
-  // Gets/sets whether to treat undefined or null in objects as nonexistent.
-  virtual bool GetStripNullFromObjects() const = 0;
+  // If true, null values are stripped from objects. This is often useful when
+  // converting arguments to extension APIs.
   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.
+  // Converts a base::Value to a 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.
   virtual v8::Handle<v8::Value> ToV8Value(
       const base::Value* value,
       v8::Handle<v8::Context> context) const = 0;
 
-  // Converts v8::Value to Value. Unsupported types are replaced with null.
-  // If an array or object throws while getting a value, that property or item
-  // is replaced with null.
+  // Converts a v8::Value to base::Value.
+  //
+  // Unsupported types (unless explicitly configured) are not converted, so
+  // this method may return NULL -- the exception is when converting arrays,
+  // where unsupported types are converted to Value(TYPE_NULL).
+  //
+  // Likewise, if an object throws while converting a property it will not be
+  // converted, whereas if an array throws while converting an item it will be
+  // converted to Value(TYPE_NULL).
   virtual base::Value* FromV8Value(v8::Handle<v8::Value> value,
                                    v8::Handle<v8::Context> context) const = 0;
 };
diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc
index 8e473184..224cb16 100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -4219,7 +4219,6 @@ WebKit::WebPlugin* RenderViewImpl::CreatePlugin(
         pepper_module.get(), params, pepper_delegate_.AsWeakPtr());
   }
 
-
 #if defined(USE_AURA) && !defined(OS_WIN)
   return NULL;
 #else
@@ -4244,8 +4243,10 @@ void RenderViewImpl::EvaluateScript(const string16& frame_xpath,
       v8::Context::Scope context_scope(context);
       V8ValueConverterImpl converter;
       converter.SetDateAllowed(true);
-      converter.SetRegexpAllowed(true);
-      list.Set(0, converter.FromV8Value(result, context));
+      converter.SetRegExpAllowed(true);
+      base::Value* result_value = converter.FromV8Value(result, context);
+      list.Set(0, result_value ? result_value :
+                                 base::Value::CreateNullValue());
     } else {
       list.Set(0, Value::CreateNullValue());
     }
diff --git a/content/renderer/v8_value_converter_impl.cc b/content/renderer/v8_value_converter_impl.cc
index 8d55ecc..4c2130d 100644
--- a/content/renderer/v8_value_converter_impl.cc
+++ b/content/renderer/v8_value_converter_impl.cc
@@ -27,38 +27,22 @@ V8ValueConverter* V8ValueConverter::create() {
 }  // namespace content
 
 V8ValueConverterImpl::V8ValueConverterImpl()
-    : undefined_allowed_(false),
-      date_allowed_(false),
-      regexp_allowed_(false),
+    : date_allowed_(false),
+      reg_exp_allowed_(false),
+      function_allowed_(false),
       strip_null_from_objects_(false) {
 }
 
-bool V8ValueConverterImpl::GetUndefinedAllowed() const {
-  return undefined_allowed_;
-}
-
-void V8ValueConverterImpl::SetUndefinedAllowed(bool val) {
-  undefined_allowed_ = val;
-}
-
-bool V8ValueConverterImpl::GetDateAllowed() const {
-  return date_allowed_;
-}
-
 void V8ValueConverterImpl::SetDateAllowed(bool val) {
   date_allowed_ = val;
 }
 
-bool V8ValueConverterImpl::GetRegexpAllowed() const {
-  return regexp_allowed_;
-}
-
-void V8ValueConverterImpl::SetRegexpAllowed(bool val) {
-  regexp_allowed_ = val;
+void V8ValueConverterImpl::SetRegExpAllowed(bool val) {
+  reg_exp_allowed_ = val;
 }
 
-bool V8ValueConverterImpl::GetStripNullFromObjects() const {
-  return strip_null_from_objects_;
+void V8ValueConverterImpl::SetFunctionAllowed(bool val) {
+  function_allowed_ = val;
 }
 
 void V8ValueConverterImpl::SetStripNullFromObjects(bool val) {
@@ -200,23 +184,37 @@ Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val,
     return Value::CreateStringValue(std::string(*utf8, utf8.length()));
   }
 
-  if (undefined_allowed_ && val->IsUndefined())
-    return Value::CreateNullValue();
+  if (val->IsUndefined())
+    // JSON.stringify ignores undefined.
+    return NULL;
 
-  if (date_allowed_ && val->IsDate()) {
+  if (val->IsDate()) {
+    if (!date_allowed_)
+      // JSON.stringify would convert this to a string, but an object is more
+      // consistent within this class.
+      return FromV8Object(val->ToObject(), unique_set);
     v8::Date* date = v8::Date::Cast(*val);
     return Value::CreateDoubleValue(date->NumberValue() / 1000.0);
   }
 
-  if (regexp_allowed_ && val->IsRegExp()) {
-    return Value::CreateStringValue(
-        *v8::String::Utf8Value(val->ToString()));
+  if (val->IsRegExp()) {
+    if (!reg_exp_allowed_)
+      // JSON.stringify converts to an object.
+      return FromV8Object(val->ToObject(), unique_set);
+    return Value::CreateStringValue(*v8::String::Utf8Value(val->ToString()));
   }
 
   // v8::Value doesn't have a ToArray() method for some reason.
   if (val->IsArray())
     return FromV8Array(val.As<v8::Array>(), unique_set);
 
+  if (val->IsFunction()) {
+    if (!function_allowed_)
+      // JSON.stringify refuses to convert function(){}.
+      return NULL;
+    return FromV8Object(val->ToObject(), unique_set);
+  }
+
   if (val->IsObject()) {
     BinaryValue* binary_value = FromV8Buffer(val);
     if (binary_value) {
@@ -225,8 +223,9 @@ Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val,
       return FromV8Object(val->ToObject(), unique_set);
     }
   }
+
   LOG(ERROR) << "Unexpected v8 value type encountered.";
-  return Value::CreateNullValue();
+  return NULL;
 }
 
 Value* V8ValueConverterImpl::FromV8Array(v8::Handle<v8::Array> val,
@@ -258,9 +257,12 @@ Value* V8ValueConverterImpl::FromV8Array(v8::Handle<v8::Array> val,
       continue;
 
     Value* child = FromV8ValueImpl(child_v8, unique_set);
-    CHECK(child);
-
-    result->Append(child);
+    if (child)
+      result->Append(child);
+    else
+      // JSON.stringify puts null in places where values don't serialize, for
+      // example undefined and functions. Emulate that behavior.
+      result->Append(Value::CreateNullValue());
   }
   return result;
 }
@@ -335,7 +337,10 @@ Value* V8ValueConverterImpl::FromV8Object(
     }
 
     scoped_ptr<Value> child(FromV8ValueImpl(child_v8, unique_set));
-    CHECK(child.get());
+    if (!child.get())
+      // JSON.stringify skips properties whose values don't serialize, for
+      // example undefined and functions. Emulate that behavior.
+      continue;
 
     // Strip null if asked (and since undefined is turned into null, undefined
     // too). The use case for supporting this is JSON-schema support,
diff --git a/content/renderer/v8_value_converter_impl.h b/content/renderer/v8_value_converter_impl.h
index baf061a..c6b9511 100644
--- a/content/renderer/v8_value_converter_impl.h
+++ b/content/renderer/v8_value_converter_impl.h
@@ -23,13 +23,9 @@ class CONTENT_EXPORT V8ValueConverterImpl : public content::V8ValueConverter {
   V8ValueConverterImpl();
 
   // V8ValueConverter implementation.
-  virtual bool GetUndefinedAllowed() const OVERRIDE;
-  virtual void SetUndefinedAllowed(bool val) OVERRIDE;
-  virtual bool GetDateAllowed() const OVERRIDE;
   virtual void SetDateAllowed(bool val) OVERRIDE;
-  virtual bool GetRegexpAllowed() const OVERRIDE;
-  virtual void SetRegexpAllowed(bool val) OVERRIDE;
-  virtual bool GetStripNullFromObjects() const OVERRIDE;
+  virtual void SetRegExpAllowed(bool val) OVERRIDE;
+  virtual void SetFunctionAllowed(bool val) OVERRIDE;
   virtual void SetStripNullFromObjects(bool val) OVERRIDE;
   virtual v8::Handle<v8::Value> ToV8Value(
       const base::Value* value,
@@ -58,14 +54,14 @@ class CONTENT_EXPORT V8ValueConverterImpl : public content::V8ValueConverter {
   base::Value* FromV8Object(v8::Handle<v8::Object> object,
                             std::set<int>* unique_set) const;
 
-  // If true, we will convert undefined JavaScript values to null.
-  bool undefined_allowed_;
-
   // If true, we will convert Date JavaScript objects to doubles.
   bool date_allowed_;
 
-  // If true, we will convet RegExp JavaScript objects to string.
-  bool regexp_allowed_;
+  // If true, we will convert RegExp JavaScript objects to string.
+  bool reg_exp_allowed_;
+
+  // If true, we will convert Function JavaScript objects to dictionaries.
+  bool function_allowed_;
 
   // If true, undefined and null values are ignored when converting v8 objects
   // into Values.
diff --git a/content/renderer/v8_value_converter_impl_unittest.cc b/content/renderer/v8_value_converter_impl_unittest.cc
index 9146fb3..b4b12bd 100644
--- a/content/renderer/v8_value_converter_impl_unittest.cc
+++ b/content/renderer/v8_value_converter_impl_unittest.cc
@@ -10,6 +10,8 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "v8/include/v8.h"
 
+using content::V8ValueConverter;
+
 namespace {
 
 // A dumb getter for an object's named callback.
@@ -112,10 +114,14 @@ class V8ValueConverterImplTest : public testing::Test {
                      base::Value::Type expected_type,
                      scoped_ptr<Value> expected_value) {
     scoped_ptr<Value> raw(converter.FromV8Value(val, context_));
-    ASSERT_TRUE(raw.get());
-    EXPECT_EQ(expected_type, raw->GetType());
-    if (expected_value.get())
+
+    if (expected_value.get()) {
+      ASSERT_TRUE(raw.get());
       EXPECT_TRUE(expected_value->Equals(raw.get()));
+      EXPECT_EQ(expected_type, raw->GetType());
+    } else {
+      EXPECT_FALSE(raw.get());
+    }
 
     v8::Handle<v8::Object> object(v8::Object::New());
     object->Set(v8::String::New("test"), val);
@@ -124,11 +130,14 @@ class V8ValueConverterImplTest : public testing::Test {
             converter.FromV8Value(object, context_)));
     ASSERT_TRUE(dictionary.get());
 
-    Value* temp = NULL;
-    ASSERT_TRUE(dictionary->Get("test", &temp));
-    EXPECT_EQ(expected_type, temp->GetType());
-    if (expected_value.get())
+    if (expected_value.get()) {
+      Value* temp = NULL;
+      ASSERT_TRUE(dictionary->Get("test", &temp));
+      EXPECT_EQ(expected_type, temp->GetType());
       EXPECT_TRUE(expected_value->Equals(temp));
+    } else {
+      EXPECT_FALSE(dictionary->HasKey("test"));
+    }
 
     v8::Handle<v8::Array> array(v8::Array::New());
     array->Set(0, val);
@@ -136,10 +145,18 @@ class V8ValueConverterImplTest : public testing::Test {
         static_cast<ListValue*>(
             converter.FromV8Value(array, context_)));
     ASSERT_TRUE(list.get());
-    ASSERT_TRUE(list->Get(0, &temp));
-    EXPECT_EQ(expected_type, temp->GetType());
-    if (expected_value.get())
+    if (expected_value.get()) {
+      Value* temp = NULL;
+      ASSERT_TRUE(list->Get(0, &temp));
+      EXPECT_EQ(expected_type, temp->GetType());
       EXPECT_TRUE(expected_value->Equals(temp));
+    } else {
+      // Arrays should preserve their length, and convert unconvertible
+      // types into null.
+      Value* temp = NULL;
+      ASSERT_TRUE(list->Get(0, &temp));
+      EXPECT_EQ(Value::TYPE_NULL, temp->GetType());
+    }
   }
 
   // Context for the JavaScript in the test.
@@ -300,23 +317,29 @@ TEST_F(V8ValueConverterImplTest, WeirdTypes) {
       v8::RegExp::New(v8::String::New("."), v8::RegExp::kNone));
 
   V8ValueConverterImpl converter;
-  TestWeirdType(converter, v8::Undefined(), Value::TYPE_NULL,
-                scoped_ptr<Value>(NULL));
-  TestWeirdType(converter, v8::Date::New(1000), Value::TYPE_DICTIONARY,
-                scoped_ptr<Value>(NULL));
-  TestWeirdType(converter, regex, Value::TYPE_DICTIONARY,
-                scoped_ptr<Value>(NULL));
-
-  converter.SetUndefinedAllowed(true);
-  TestWeirdType(converter, v8::Undefined(), Value::TYPE_NULL,
+  TestWeirdType(converter,
+                v8::Undefined(),
+                Value::TYPE_NULL,  // Arbitrary type, result is NULL.
                 scoped_ptr<Value>(NULL));
+  TestWeirdType(converter,
+                v8::Date::New(1000),
+                Value::TYPE_DICTIONARY,
+                scoped_ptr<Value>(new DictionaryValue()));
+  TestWeirdType(converter,
+                regex,
+                Value::TYPE_DICTIONARY,
+                scoped_ptr<Value>(new DictionaryValue()));
 
   converter.SetDateAllowed(true);
-  TestWeirdType(converter, v8::Date::New(1000), Value::TYPE_DOUBLE,
+  TestWeirdType(converter,
+                v8::Date::New(1000),
+                Value::TYPE_DOUBLE,
                 scoped_ptr<Value>(Value::CreateDoubleValue(1)));
 
-  converter.SetRegexpAllowed(true);
-  TestWeirdType(converter, regex, Value::TYPE_STRING,
+  converter.SetRegExpAllowed(true);
+  TestWeirdType(converter,
+                regex,
+                Value::TYPE_STRING,
                 scoped_ptr<Value>(Value::CreateStringValue("/./")));
 }
 
@@ -353,7 +376,6 @@ TEST_F(V8ValueConverterImplTest, StripNullFromObjects) {
   ASSERT_FALSE(object.IsEmpty());
 
   V8ValueConverterImpl converter;
-  converter.SetUndefinedAllowed(true);
   converter.SetStripNullFromObjects(true);
 
   scoped_ptr<DictionaryValue> result(
@@ -485,3 +507,43 @@ TEST_F(V8ValueConverterImplTest, ArrayGetters) {
   ASSERT_TRUE(result.get());
   EXPECT_EQ(2u, result->GetSize());
 }
+
+TEST_F(V8ValueConverterImplTest, UndefinedValueBehavior) {
+  v8::Context::Scope context_scope(context_);
+  v8::HandleScope handle_scope;
+
+  v8::Handle<v8::Object> object;
+  {
+    const char* source = "(function() {"
+        "return { foo: undefined, bar: null, baz: function(){} };"
+        "})();";
+    v8::Handle<v8::Script> script(v8::Script::New(v8::String::New(source)));
+    object = script->Run().As<v8::Object>();
+    ASSERT_FALSE(object.IsEmpty());
+  }
+
+  v8::Handle<v8::Array> array;
+  {
+    const char* source = "(function() {"
+        "return [ undefined, null, function(){} ];"
+        "})();";
+    v8::Handle<v8::Script> script(v8::Script::New(v8::String::New(source)));
+    array = script->Run().As<v8::Array>();
+    ASSERT_FALSE(array.IsEmpty());
+  }
+
+  V8ValueConverterImpl converter;
+
+  DictionaryValue expected_object;
+  expected_object.Set("bar", Value::CreateNullValue());
+  scoped_ptr<Value> actual_object(converter.FromV8Value(object, context_));
+  EXPECT_TRUE(Value::Equals(&expected_object, actual_object.get()));
+
+  ListValue expected_array;
+  // Everything is null because JSON stringification preserves array length.
+  expected_array.Append(Value::CreateNullValue());
+  expected_array.Append(Value::CreateNullValue());
+  expected_array.Append(Value::CreateNullValue());
+  scoped_ptr<Value> actual_array(converter.FromV8Value(array, context_));
+  EXPECT_TRUE(Value::Equals(&expected_array, actual_array.get()));
+}