Disallow passing a SharedArrayBuffer in the transfer list.

This behavior changed recently. SharedArrayBuffers should not be put in the
transfer list, because they are not detached, and that is the meaning of being
in the transfer list.

This is the V8 side of the change, the Blink side will come next.

Reland of https://codereview.chromium.org/2570433005, it was reverted because
of a Blink-side test failure which has been temporarily disabled; see
https://codereview.chromium.org/2590003002.

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=676063

Review-Url: https://codereview.chromium.org/2594793005
Cr-Commit-Position: refs/heads/master@{#42054}

include/v8.h

@@ -1709,6 +1709,19 @@ class V8_EXPORT ValueSerializer {
      */
     virtual Maybe<bool> WriteHostObject(Isolate* isolate, Local<Object> object);
 
+    /*
+     * Called when the ValueSerializer is going to serialize a
+     * SharedArrayBuffer object. The embedder must return an ID for the
+     * object, using the same ID if this SharedArrayBuffer has already been
+     * serialized in this buffer. When deserializing, this ID will be passed to
+     * ValueDeserializer::TransferSharedArrayBuffer as |transfer_id|.
+     *
+     * If the object cannot be serialized, an
+     * exception should be thrown and Nothing<uint32_t>() returned.
+     */
+    virtual Maybe<uint32_t> GetSharedArrayBufferId(
+        Isolate* isolate, Local<SharedArrayBuffer> shared_array_buffer);
+
     /*
      * Allocates memory for the buffer of at least the size provided. The actual
      * size (which may be greater or equal) is written to |actual_size|. If no
@@ -1763,8 +1776,10 @@ class V8_EXPORT ValueSerializer {
   /*
    * Similar to TransferArrayBuffer, but for SharedArrayBuffer.
    */
-  void TransferSharedArrayBuffer(uint32_t transfer_id,
-                                 Local<SharedArrayBuffer> shared_array_buffer);
+  V8_DEPRECATE_SOON("Use Delegate::GetSharedArrayBufferId",
+                    void TransferSharedArrayBuffer(
+                        uint32_t transfer_id,
+                        Local<SharedArrayBuffer> shared_array_buffer));
 
   /*
    * Write raw data in various common formats to the buffer.
@@ -1831,9 +1846,10 @@ class V8_EXPORT ValueDeserializer {
 
   /*
    * Similar to TransferArrayBuffer, but for SharedArrayBuffer.
-   * transfer_id exists in the same namespace as unshared ArrayBuffer objects.
+   * The id is not necessarily in the same namespace as unshared ArrayBuffer
+   * objects.
    */
-  void TransferSharedArrayBuffer(uint32_t transfer_id,
+  void TransferSharedArrayBuffer(uint32_t id,
                                  Local<SharedArrayBuffer> shared_array_buffer);
 
   /*

src/api.cc

@@ -3071,6 +3071,15 @@ Maybe<bool> ValueSerializer::Delegate::WriteHostObject(Isolate* v8_isolate,
   return Nothing<bool>();
 }
 
+Maybe<uint32_t> ValueSerializer::Delegate::GetSharedArrayBufferId(
+    Isolate* v8_isolate, Local<SharedArrayBuffer> shared_array_buffer) {
+  i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
+  isolate->ScheduleThrow(*isolate->factory()->NewError(
+      isolate->error_function(), i::MessageTemplate::kDataCloneError,
+      Utils::OpenHandle(*shared_array_buffer)));
+  return Nothing<uint32_t>();
+}
+
 void* ValueSerializer::Delegate::ReallocateBufferMemory(void* old_buffer,
                                                         size_t size,
                                                         size_t* actual_size) {

src/messages.h

@@ -676,8 +676,8 @@ class ErrorUtils : public AllStatic {
   T(DataCloneError, "% could not be cloned.")                                  \
   T(DataCloneErrorNeuteredArrayBuffer,                                         \
     "An ArrayBuffer is neutered and could not be cloned.")                     \
-  T(DataCloneErrorSharedArrayBufferNotTransferred,                             \
-    "A SharedArrayBuffer could not be cloned. SharedArrayBuffer must be "      \
+  T(DataCloneErrorSharedArrayBufferTransferred,                                \
+    "A SharedArrayBuffer could not be cloned. SharedArrayBuffer must not be "  \
     "transferred.")                                                            \
   T(DataCloneDeserializationError, "Unable to deserialize cloned data.")       \
   T(DataCloneDeserializationVersionError,                                      \

src/value-serializer.cc

@@ -110,8 +110,8 @@ enum class SerializationTag : uint8_t {
   // ObjectReference to one) serialized just before it. This is a quirk arising
   // from the previous stack-based implementation.
   kArrayBufferView = 'V',
-  // Shared array buffer (transferred). transferID:uint32_t
-  kSharedArrayBufferTransfer = 'u',
+  // Shared array buffer. transferID:uint32_t
+  kSharedArrayBuffer = 'u',
   // Compiled WebAssembly module. encodingType:(one-byte tag).
   // If encodingType == 'y' (raw bytes):
   //  wasmWireByteLength:uint32_t, then raw data
@@ -269,6 +269,7 @@ std::pair<uint8_t*, size_t> ValueSerializer::Release() {
 void ValueSerializer::TransferArrayBuffer(uint32_t transfer_id,
                                           Handle<JSArrayBuffer> array_buffer) {
   DCHECK(!array_buffer_transfer_map_.Find(array_buffer));
+  DCHECK(!array_buffer->is_shared());
   array_buffer_transfer_map_.Set(array_buffer, transfer_id);
 }
 
@@ -442,7 +443,7 @@ Maybe<bool> ValueSerializer::WriteJSReceiver(Handle<JSReceiver> receiver) {
     case JS_SET_TYPE:
       return WriteJSSet(Handle<JSSet>::cast(receiver));
     case JS_ARRAY_BUFFER_TYPE:
-      return WriteJSArrayBuffer(JSArrayBuffer::cast(*receiver));
+      return WriteJSArrayBuffer(Handle<JSArrayBuffer>::cast(receiver));
     case JS_TYPED_ARRAY_TYPE:
     case JS_DATA_VIEW_TYPE:
       return WriteJSArrayBufferView(JSArrayBufferView::cast(*receiver));
@@ -724,28 +725,37 @@ Maybe<bool> ValueSerializer::WriteJSSet(Handle<JSSet> set) {
   return Just(true);
 }
 
-Maybe<bool> ValueSerializer::WriteJSArrayBuffer(JSArrayBuffer* array_buffer) {
+Maybe<bool> ValueSerializer::WriteJSArrayBuffer(
+    Handle<JSArrayBuffer> array_buffer) {
+  if (array_buffer->is_shared()) {
+    if (!delegate_) {
+      ThrowDataCloneError(MessageTemplate::kDataCloneError, array_buffer);
+      return Nothing<bool>();
+    }
+
+    v8::Isolate* v8_isolate = reinterpret_cast<v8::Isolate*>(isolate_);
+    Maybe<uint32_t> index = delegate_->GetSharedArrayBufferId(
+        v8_isolate, Utils::ToLocalShared(array_buffer));
+    RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate_, Nothing<bool>());
+
+    WriteTag(SerializationTag::kSharedArrayBuffer);
+    WriteVarint(index.FromJust());
+    return Just(true);
+  }
+
   uint32_t* transfer_entry = array_buffer_transfer_map_.Find(array_buffer);
   if (transfer_entry) {
-    WriteTag(array_buffer->is_shared()
-                 ? SerializationTag::kSharedArrayBufferTransfer
-                 : SerializationTag::kArrayBufferTransfer);
+    WriteTag(SerializationTag::kArrayBufferTransfer);
     WriteVarint(*transfer_entry);
     return Just(true);
   }
-
-  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));
+    ThrowDataCloneError(MessageTemplate::kDataCloneError, array_buffer);
     return Nothing<bool>();
   }
   WriteTag(SerializationTag::kArrayBuffer);
@@ -1105,7 +1115,7 @@ MaybeHandle<Object> ValueDeserializer::ReadObjectInternal() {
       const bool is_shared = false;
       return ReadTransferredJSArrayBuffer(is_shared);
     }
-    case SerializationTag::kSharedArrayBufferTransfer: {
+    case SerializationTag::kSharedArrayBuffer: {
       const bool is_shared = true;
       return ReadTransferredJSArrayBuffer(is_shared);
     }

src/value-serializer.h

@@ -112,7 +112,8 @@ class ValueSerializer {
   void WriteJSRegExp(JSRegExp* regexp);
   Maybe<bool> WriteJSMap(Handle<JSMap> map) WARN_UNUSED_RESULT;
   Maybe<bool> WriteJSSet(Handle<JSSet> map) WARN_UNUSED_RESULT;
-  Maybe<bool> WriteJSArrayBuffer(JSArrayBuffer* array_buffer);
+  Maybe<bool> WriteJSArrayBuffer(Handle<JSArrayBuffer> array_buffer)
+      WARN_UNUSED_RESULT;
   Maybe<bool> WriteJSArrayBufferView(JSArrayBufferView* array_buffer);
   Maybe<bool> WriteWasmModule(Handle<JSObject> object) WARN_UNUSED_RESULT;
   Maybe<bool> WriteHostObject(Handle<JSObject> object) WARN_UNUSED_RESULT;

test/unittests/value-serializer-unittest.cc

@@ -19,6 +19,7 @@ namespace {
 
 using ::testing::_;
 using ::testing::Invoke;
+using ::testing::Return;
 
 class ValueSerializerTest : public TestWithIsolate {
  protected:
@@ -129,15 +130,22 @@ class ValueSerializerTest : public TestWithIsolate {
     encoded_data_functor(buffer);
   }
 
-  template <typename MessageFunctor>
-  void InvalidEncodeTest(const char* source, const MessageFunctor& functor) {
+  template <typename InputFunctor, typename MessageFunctor>
+  void InvalidEncodeTest(const InputFunctor& input_functor,
+                         const MessageFunctor& functor) {
     Context::Scope scope(serialization_context());
     TryCatch try_catch(isolate());
-    Local<Value> input_value = EvaluateScriptForInput(source);
+    Local<Value> input_value = input_functor();
     ASSERT_TRUE(DoEncode(input_value).IsNothing());
     functor(try_catch.Message());
   }
 
+  template <typename MessageFunctor>
+  void InvalidEncodeTest(const char* source, const MessageFunctor& functor) {
+    InvalidEncodeTest(
+        [this, source]() { return EvaluateScriptForInput(source); }, functor);
+  }
+
   void InvalidEncodeTest(const char* source) {
     InvalidEncodeTest(source, [](Local<Message>) {});
   }
@@ -2042,7 +2050,8 @@ class ValueSerializerTestWithSharedArrayBufferTransfer
  protected:
   static const size_t kTestByteLength = 4;
 
-  ValueSerializerTestWithSharedArrayBufferTransfer() {
+  ValueSerializerTestWithSharedArrayBufferTransfer()
+      : serializer_delegate_(this) {
     const uint8_t data[kTestByteLength] = {0x00, 0x01, 0x80, 0xff};
     memcpy(data_, data, kTestByteLength);
     {
@@ -2060,10 +2069,6 @@ class ValueSerializerTestWithSharedArrayBufferTransfer
   const Local<SharedArrayBuffer>& input_buffer() { return input_buffer_; }
   const Local<SharedArrayBuffer>& output_buffer() { return output_buffer_; }
 
-  void BeforeEncode(ValueSerializer* serializer) override {
-    serializer->TransferSharedArrayBuffer(0, input_buffer_);
-  }
-
   void BeforeDecode(ValueDeserializer* deserializer) override {
     deserializer->TransferSharedArrayBuffer(0, output_buffer_);
   }
@@ -2080,6 +2085,39 @@ class ValueSerializerTestWithSharedArrayBufferTransfer
     flag_was_enabled_ = false;
   }
 
+ protected:
+// GMock doesn't use the "override" keyword.
+#if __clang__
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Winconsistent-missing-override"
+#endif
+
+  class SerializerDelegate : public ValueSerializer::Delegate {
+   public:
+    explicit SerializerDelegate(
+        ValueSerializerTestWithSharedArrayBufferTransfer* test)
+        : test_(test) {}
+    MOCK_METHOD2(GetSharedArrayBufferId,
+                 Maybe<uint32_t>(Isolate* isolate,
+                                 Local<SharedArrayBuffer> shared_array_buffer));
+    void ThrowDataCloneError(Local<String> message) override {
+      test_->isolate()->ThrowException(Exception::Error(message));
+    }
+
+   private:
+    ValueSerializerTestWithSharedArrayBufferTransfer* test_;
+  };
+
+#if __clang__
+#pragma clang diagnostic pop
+#endif
+
+  ValueSerializer::Delegate* GetSerializerDelegate() override {
+    return &serializer_delegate_;
+  }
+
+  SerializerDelegate serializer_delegate_;
+
  private:
   static bool flag_was_enabled_;
   uint8_t data_[kTestByteLength];
@@ -2092,6 +2130,10 @@ bool ValueSerializerTestWithSharedArrayBufferTransfer::flag_was_enabled_ =
 
 TEST_F(ValueSerializerTestWithSharedArrayBufferTransfer,
        RoundTripSharedArrayBufferTransfer) {
+  EXPECT_CALL(serializer_delegate_,
+              GetSharedArrayBufferId(isolate(), input_buffer()))
+      .WillRepeatedly(Return(Just(0U)));
+
   RoundTripTest([this]() { return input_buffer(); },
                 [this](Local<Value> value) {
                   ASSERT_TRUE(value->IsSharedArrayBuffer());
@@ -2123,12 +2165,6 @@ TEST_F(ValueSerializerTestWithSharedArrayBufferTransfer,
       });
 }
 
-TEST_F(ValueSerializerTestWithSharedArrayBufferTransfer,
-       SharedArrayBufferMustBeTransferred) {
-  // A SharedArrayBuffer which was not marked for transfer should fail encoding.
-  InvalidEncodeTest("new SharedArrayBuffer(32)");
-}
-
 TEST_F(ValueSerializerTest, UnsupportedHostObject) {
   InvalidEncodeTest("new ExampleHostObject()");
   InvalidEncodeTest("({ a: new ExampleHostObject() })");