ScriptValueSerializer should throw, not crash, when handling unknown types

Source/bindings/core/v8/ScriptValueSerializer.cpp

Index: Source/bindings/core/v8/ScriptValueSerializer.cpp
diff --git a/Source/bindings/core/v8/ScriptValueSerializer.cpp b/Source/bindings/core/v8/ScriptValueSerializer.cpp
index f3f745a957887b9da4ef0316266f0eb3b5a0a29c..688b058a3220e5b41c710883f0c3b3829c76889a 100644
--- a/Source/bindings/core/v8/ScriptValueSerializer.cpp
+++ b/Source/bindings/core/v8/ScriptValueSerializer.cpp
@@ -712,7 +712,6 @@ ScriptValueSerializer::StateBase* ScriptValueSerializer::doSerialize(v8::Local<v
 
 ScriptValueSerializer::StateBase* ScriptValueSerializer::doSerializeValue(v8::Local<v8::Value> value, ScriptValueSerializer::StateBase* next)
 {
-    uint32_t arrayBufferIndex;
     if (value.IsEmpty())
         return handleError(InputError, "The empty property name cannot be cloned.", next);
     if (value->IsUndefined()) {
@@ -729,25 +728,27 @@ ScriptValueSerializer::StateBase* ScriptValueSerializer::doSerializeValue(v8::Lo
         m_writer.writeUint32(value.As<v8::Uint32>()->Value());
     } else if (value->IsNumber()) {
         m_writer.writeNumber(value.As<v8::Number>()->Value());
-    } else if (V8ArrayBufferView::hasInstance(value, isolate())) {
-        return writeAndGreyArrayBufferView(value.As<v8::Object>(), next);
     } else if (value->IsString()) {
         writeString(value);
-    } else if (V8MessagePort::hasInstance(value, isolate())) {
-        uint32_t messagePortIndex;
-        if (m_transferredMessagePorts.tryGet(value.As<v8::Object>(), &messagePortIndex)) {
-            m_writer.writeTransferredMessagePort(messagePortIndex);
-        } else {
-            return handleError(DataCloneError, "A MessagePort could not be cloned.", next);
-        }
-    } else if (V8ArrayBuffer::hasInstance(value, isolate()) && m_transferredArrayBuffers.tryGet(value.As<v8::Object>(), &arrayBufferIndex)) {
-        return writeTransferredArrayBuffer(value, arrayBufferIndex, next);
-    } else if (V8SharedArrayBuffer::hasInstance(value, isolate()) && m_transferredArrayBuffers.tryGet(value.As<v8::Object>(), &arrayBufferIndex)) {
-        return writeTransferredSharedArrayBuffer(value, arrayBufferIndex, next);
-    } else {
+    } else if (value->IsObject()) {
         v8::Local<v8::Object> jsObject = value.As<v8::Object>();
-        if (jsObject.IsEmpty())
-            return handleError(DataCloneError, "An object could not be cloned.", next);
+
+        uint32_t arrayBufferIndex;
+        if (V8ArrayBufferView::hasInstance(value, isolate())) {
+            return writeAndGreyArrayBufferView(jsObject, next);
+        } else if (V8MessagePort::hasInstance(value, isolate())) {
+            uint32_t messagePortIndex;
+            if (m_transferredMessagePorts.tryGet(jsObject, &messagePortIndex)) {
+                m_writer.writeTransferredMessagePort(messagePortIndex);

[jsbell, 2015/08/19 01:24:11]

This needs an early exit here

[adamk, 2015/08/19 20:19:34]

Switched the logic around here to return if tryGet fails.

It's unclear to me why this tryGet failure is treated as an error while the
tryGets for ArrayBuffer and SharedArrayBuffer are not.

[jsbell, 2015/08/19 20:53:15]

A MessagePort can't be just cloned, it must be transferred (i.e. the original is
modified by the operation, and this must be specified by including the
MessagePort in the transferrables list). An ArrayBuffer can be just cloned or
transferred. (Ditto for SharedBuffer apparently?)

+            } else {
+                return handleError(DataCloneError, "A MessagePort could not be cloned.", next);
+            }
+        } else if (V8ArrayBuffer::hasInstance(value, isolate()) && m_transferredArrayBuffers.tryGet(jsObject, &arrayBufferIndex)) {
+            return writeTransferredArrayBuffer(value, arrayBufferIndex, next);
+        } else if (V8SharedArrayBuffer::hasInstance(value, isolate()) && m_transferredArrayBuffers.tryGet(jsObject, &arrayBufferIndex)) {
+            return writeTransferredSharedArrayBuffer(value, arrayBufferIndex, next);
+        }
+
         greyObject(jsObject);
         if (value->IsDate()) {
             m_writer.writeDate(value.As<v8::Date>()->ValueOf());

[jsbell, 2015/08/19 01:24:11]

this needs an early exit, or the "return startObjectState" down below needs to
be in an `else` clause

[adamk, 2015/08/19 20:19:34]

Oops, didn't see that still needed to be in an 'else'. Changed back to that.

@@ -777,13 +778,12 @@ ScriptValueSerializer::StateBase* ScriptValueSerializer::doSerializeValue(v8::Lo
             return writeArrayBuffer(value, next);
         } else if (V8CompositorProxy::hasInstance(value, isolate())) {
             return writeCompositorProxy(value, next);
-        } else if (value->IsObject()) {
-            if (isHostObject(jsObject) || jsObject->IsCallable() || value->IsNativeError())
-                return handleError(DataCloneError, "An object could not be cloned.", next);
-            return startObjectState(jsObject, next);
-        } else {
-            return handleError(DataCloneError, "A value could not be cloned.", next);
+        } else if (isHostObject(jsObject) || jsObject->IsCallable() || value->IsNativeError()) {
+            return handleError(DataCloneError, "An object could not be cloned.", next);
         }
+        return startObjectState(jsObject, next);
+    } else {
+        return handleError(DataCloneError, "A value could not be cloned.", next);
     }
     return 0;

[jsbell, 2015/08/19 01:24:11]

nullptr, while you're here?

[adamk, 2015/08/19 20:19:34]

Done.

 }