ScriptValueSerializer should throw, not crash, when handling unknown types

Source/bindings/core/v8/ScriptValueSerializer.cpp

 // Copyright 2014 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 "config.h"
 #include "bindings/core/v8/ScriptValueSerializer.h"
 
 #include "bindings/core/v8/V8ArrayBuffer.h"
 #include "bindings/core/v8/V8ArrayBufferView.h"
 #include "bindings/core/v8/V8Blob.h"
@@ skipping 694 matching lines @@
         ASSERT(!value->IsString());
         m_writer.writeObjectReference(objectReference);
     } else {
         return doSerializeValue(value, next);
     }
     return 0;
 }
 
 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()) {
         m_writer.writeUndefined();
     } else if (value->IsNull()) {
         m_writer.writeNull();
     } else if (value->IsTrue()) {
         m_writer.writeTrue();
     } else if (value->IsFalse()) {
         m_writer.writeFalse();
     } else if (value->IsInt32()) {
         m_writer.writeInt32(value.As<v8::Int32>()->Value());
     } else if (value->IsUint32()) {
         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 (value->IsObject()) {
+        v8::Local<v8::Object> jsObject = value.As<v8::Object>();
+
+        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);
         }
-    } 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 {
-        v8::Local<v8::Object> jsObject = value.As<v8::Object>();
-        if (jsObject.IsEmpty())
-            return handleError(DataCloneError, "An object could not be cloned.", 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.

         } else if (value->IsStringObject()) {
             writeStringObject(value);

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

ditto

         } else if (value->IsNumberObject()) {
             writeNumberObject(value);

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

ditto

         } else if (value->IsBooleanObject()) {
             writeBooleanObject(value);

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

ditto

         } else if (value->IsArray()) {
             return startArrayState(value.As<v8::Array>(), next);
         } else if (value->IsMap()) {
             return startMapState(value.As<v8::Map>(), next);
         } else if (value->IsSet()) {
             return startSetState(value.As<v8::Set>(), next);
         } else if (V8File::hasInstance(value, isolate())) {
             return writeFile(value, next);
         } else if (V8Blob::hasInstance(value, isolate())) {
             return writeBlob(value, next);
         } else if (V8FileList::hasInstance(value, isolate())) {
             return writeFileList(value, next);
         } else if (V8ImageData::hasInstance(value, isolate())) {
             writeImageData(value);

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

ditto

         } else if (value->IsRegExp()) {
             writeRegExp(value);

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

ditto

         } else if (V8ArrayBuffer::hasInstance(value, isolate())) {
             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.

 }
 
 ScriptValueSerializer::StateBase* ScriptValueSerializer::doSerializeArrayBuffer(v8::Local<v8::Value> arrayBuffer, ScriptValueSerializer::StateBase* next)
 {
     return doSerialize(arrayBuffer, next);
 }
 
 ScriptValueSerializer::StateBase* ScriptValueSerializer::checkException(ScriptValueSerializer::StateBase* state)
 {
     return m_tryCatch.HasCaught() ? handleError(JSException, "", state) : 0;
@@ skipping 1359 matching lines @@
         return false;
     uint32_t objectReference = m_openCompositeReferenceStack[m_openCompositeReferenceStack.size() - 1];
     m_openCompositeReferenceStack.shrink(m_openCompositeReferenceStack.size() - 1);
     if (objectReference >= m_objectPool.size())
         return false;
     *object = m_objectPool[objectReference];
     return true;
 }
 
 } // namespace blink