Throw exceptions for errors in v8::ValueSerializer.

src/value-serializer.cc

Index: src/value-serializer.cc
diff --git a/src/value-serializer.cc b/src/value-serializer.cc
index 8416ea3cd61dea9ad11ba148f83339153b4e4f4a..a41c182e328432f563c7b683deab1c8639ed31ae 100644
--- a/src/value-serializer.cc
+++ b/src/value-serializer.cc
@@ -124,8 +124,10 @@ enum class ArrayBufferViewTag : uint8_t {
 
 }  // namespace
 
-ValueSerializer::ValueSerializer(Isolate* isolate)
+ValueSerializer::ValueSerializer(Isolate* isolate,
+                                 v8::ValueSerializer::Delegate* delegate)
     : isolate_(isolate),
+      delegate_(delegate),
       zone_(isolate->allocator()),
       id_map_(isolate->heap(), &zone_),
       array_buffer_transfer_map_(isolate->heap(), &zone_) {}
@@ -247,9 +249,10 @@ Maybe<bool> ValueSerializer::WriteObject(Handle<Object> object) {
         return Just(true);
       } else if (object->IsJSReceiver()) {
         return WriteJSReceiver(Handle<JSReceiver>::cast(object));
+      } else {
+        ThrowDataCloneError(MessageTemplate::kDataCloneError, object);

[Jakob Kummerow, 2016/09/02 10:20:28]

I'm not sure about this case. UNIMPLEMENTED() crashes even in Release mode. If
hitting this case can happen during normal operation and must be handled by
embedders, then throwing an error is of course appropriate. On the other hand,
if you'd say that hitting this case would be a V8/Chrome bug and we should learn
about it in the form of a crash report, then you should keep the
UNIMPLEMENTED().

[jbroman, 2016/09/02 14:51:08]

At the moment, this is the case that handles Symbol, SIMD, and other objects,
which a caller might pass to postMessage. While it might be possible to
eventually enumerate all of those, I think it's reasonable to just throw an
error here.

+        return Nothing<bool>();
       }
-      UNIMPLEMENTED();
-      return Nothing<bool>();
   }
 }
 
@@ -337,11 +340,12 @@ Maybe<bool> ValueSerializer::WriteJSReceiver(Handle<JSReceiver> receiver) {
   // Eliminate callable and exotic objects, which should not be serialized.
   InstanceType instance_type = receiver->map()->instance_type();
   if (receiver->IsCallable() || instance_type <= LAST_SPECIAL_RECEIVER_TYPE) {
+    ThrowDataCloneError(MessageTemplate::kDataCloneError, receiver);
     return Nothing<bool>();
   }
 
   // If we are at the end of the stack, abort. This function may recurse.
-  if (StackLimitCheck(isolate_).HasOverflowed()) return Nothing<bool>();
+  STACK_CHECK(isolate_, Nothing<bool>());
 
   HandleScope scope(isolate_);
   switch (instance_type) {
@@ -368,8 +372,8 @@ Maybe<bool> ValueSerializer::WriteJSReceiver(Handle<JSReceiver> receiver) {
     case JS_DATA_VIEW_TYPE:
       return WriteJSArrayBufferView(JSArrayBufferView::cast(*receiver));
     default:
-      UNIMPLEMENTED();
-      break;
+      ThrowDataCloneError(MessageTemplate::kDataCloneError, receiver);

[Jakob Kummerow, 2016/09/02 10:20:28]

same here

[jbroman, 2016/09/02 14:51:08]

Similarly, there are a number of kinds of receiver that should throw an error
when you try to clone them, like Promise, WeakMap, the various collection
iterators, etc. Because these values come from JavaScript, it's not a bug in
Chrome if someone attempts to clone one of them.

+      return Nothing<bool>();
   }
   return Nothing<bool>();
 }
@@ -479,6 +483,7 @@ Maybe<bool> ValueSerializer::WriteJSValue(Handle<JSValue> value) {
                           v8::String::NO_NULL_TERMINATION);
   } else {
     DCHECK(inner_value->IsSymbol());
+    ThrowDataCloneError(MessageTemplate::kDataCloneError, value);
     return Nothing<bool>();
   }
   return Just(true);
@@ -567,10 +572,18 @@ Maybe<bool> ValueSerializer::WriteJSArrayBuffer(JSArrayBuffer* array_buffer) {
     return Just(true);
   }
 
-  if (array_buffer->is_shared()) return Nothing<bool>();
-  if (array_buffer->was_neutered()) return Nothing<bool>();
+  if (array_buffer->is_shared()) {
+    ThrowDataCloneError(
+        MessageTemplate::kDataCloneErrorSharedArrayBufferNotTransferred);
+    return Nothing<bool>();
+  }
+  if (array_buffer->was_neutered()) {
+    ThrowDataCloneError(MessageTemplate::kDataCloneErrorNeuteredArrayBuffer);
+    return Nothing<bool>();
+  }
   double byte_length = array_buffer->byte_length()->Number();
   if (byte_length > std::numeric_limits<uint32_t>::max()) {
+    ThrowDataCloneError(MessageTemplate::kDataCloneError, handle(array_buffer));
     return Nothing<bool>();
   }
   WriteTag(SerializationTag::kArrayBuffer);
@@ -629,6 +642,24 @@ Maybe<uint32_t> ValueSerializer::WriteJSObjectProperties(
   return Just(properties_written);
 }
 
+void ValueSerializer::ThrowDataCloneError(
+    MessageTemplate::Template template_index) {
+  return ThrowDataCloneError(template_index,
+                             isolate_->factory()->empty_string());
+}
+
+void ValueSerializer::ThrowDataCloneError(
+    MessageTemplate::Template template_index, Handle<Object> arg0) {
+  Handle<String> message =
+      MessageTemplate::FormatMessage(isolate_, template_index, arg0);
+  if (delegate_) {
+    delegate_->ThrowDataCloneError(Utils::ToLocal(message));
+  } else {
+    isolate_->Throw(
+        *isolate_->factory()->NewError(isolate_->error_function(), message));
+  }
+}
+
 ValueDeserializer::ValueDeserializer(Isolate* isolate,
                                      Vector<const uint8_t> data)
     : isolate_(isolate),
@@ -887,7 +918,7 @@ MaybeHandle<String> ValueDeserializer::ReadTwoByteString() {
 
 MaybeHandle<JSObject> ValueDeserializer::ReadJSObject() {
   // If we are at the end of the stack, abort. This function may recurse.
-  if (StackLimitCheck(isolate_).HasOverflowed()) return MaybeHandle<JSObject>();
+  STACK_CHECK(isolate_, MaybeHandle<JSObject>());
 
   uint32_t id = next_id_++;
   HandleScope scope(isolate_);
@@ -910,7 +941,7 @@ MaybeHandle<JSObject> ValueDeserializer::ReadJSObject() {
 
 MaybeHandle<JSArray> ValueDeserializer::ReadSparseJSArray() {
   // If we are at the end of the stack, abort. This function may recurse.
-  if (StackLimitCheck(isolate_).HasOverflowed()) return MaybeHandle<JSArray>();
+  STACK_CHECK(isolate_, MaybeHandle<JSArray>());
 
   uint32_t length;
   if (!ReadVarint<uint32_t>().To(&length)) return MaybeHandle<JSArray>();
@@ -938,7 +969,7 @@ MaybeHandle<JSArray> ValueDeserializer::ReadSparseJSArray() {
 
 MaybeHandle<JSArray> ValueDeserializer::ReadDenseJSArray() {
   // If we are at the end of the stack, abort. This function may recurse.
-  if (StackLimitCheck(isolate_).HasOverflowed()) return MaybeHandle<JSArray>();
+  STACK_CHECK(isolate_, MaybeHandle<JSArray>());
 
   uint32_t length;
   if (!ReadVarint<uint32_t>().To(&length)) return MaybeHandle<JSArray>();
@@ -1042,7 +1073,7 @@ MaybeHandle<JSRegExp> ValueDeserializer::ReadJSRegExp() {
 
 MaybeHandle<JSMap> ValueDeserializer::ReadJSMap() {
   // If we are at the end of the stack, abort. This function may recurse.
-  if (StackLimitCheck(isolate_).HasOverflowed()) return MaybeHandle<JSMap>();
+  STACK_CHECK(isolate_, MaybeHandle<JSMap>());
 
   HandleScope scope(isolate_);
   uint32_t id = next_id_++;
@@ -1079,7 +1110,7 @@ MaybeHandle<JSMap> ValueDeserializer::ReadJSMap() {
 
 MaybeHandle<JSSet> ValueDeserializer::ReadJSSet() {
   // If we are at the end of the stack, abort. This function may recurse.
-  if (StackLimitCheck(isolate_).HasOverflowed()) return MaybeHandle<JSSet>();
+  STACK_CHECK(isolate_, MaybeHandle<JSSet>());
 
   HandleScope scope(isolate_);
   uint32_t id = next_id_++;