Revert of Disallow passing a SharedArrayBuffer in the transfer list. (patchset...

Revert of Disallow passing a SharedArrayBuffer in the transfer list. (patchset #4 id:60001 of https://codereview.chromium.org/2570433005/ )

Reason for revert:
Breaks layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/builds/12098

See:
https://github.com/v8/v8/wiki/Blink-layout-tests

Original issue's description:
> 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.
>
> Review-Url: https://codereview.chromium.org/2570433005
> Cr-Commit-Position: refs/heads/master@{#41740}
> Committed: https://chromium.googlesource.com/v8/v8/+/1c5e1504e0305363cd262f1706cbd63f9a62ae46

TBR=jbroman@chromium.org,jkummerow@chromium.org,binji@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

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

include/v8.h

@@ -1709,19 +1709,6 @@ 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
@@ -1776,10 +1763,8 @@ class V8_EXPORT ValueSerializer {
   /*
    * Similar to TransferArrayBuffer, but for SharedArrayBuffer.
    */
-  V8_DEPRECATE_SOON("Use Delegate::GetSharedArrayBufferId",
-                    void TransferSharedArrayBuffer(
-                        uint32_t transfer_id,
-                        Local<SharedArrayBuffer> shared_array_buffer));
+  void TransferSharedArrayBuffer(uint32_t transfer_id,
+                                 Local<SharedArrayBuffer> shared_array_buffer);
 
   /*
    * Write raw data in various common formats to the buffer.
@@ -1846,10 +1831,9 @@ class V8_EXPORT ValueDeserializer {
 
   /*
    * Similar to TransferArrayBuffer, but for SharedArrayBuffer.
-   * The id is not necessarily in the same namespace as unshared ArrayBuffer
-   * objects.
+   * transfer_id exists in the same namespace as unshared ArrayBuffer objects.
    */
-  void TransferSharedArrayBuffer(uint32_t id,
+  void TransferSharedArrayBuffer(uint32_t transfer_id,
                                  Local<SharedArrayBuffer> shared_array_buffer);
 
   /*

src/api.cc

@@ -3033,15 +3033,6 @@ 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

@@ -677,8 +677,8 @@ class ErrorUtils : public AllStatic {
   T(DataCloneError, "% could not be cloned.")                                  \
   T(DataCloneErrorNeuteredArrayBuffer,                                         \
     "An ArrayBuffer is neutered and could not be cloned.")                     \
-  T(DataCloneErrorSharedArrayBufferTransferred,                                \
-    "A SharedArrayBuffer could not be cloned. SharedArrayBuffer must not be "  \
+  T(DataCloneErrorSharedArrayBufferNotTransferred,                             \
+    "A SharedArrayBuffer could not be cloned. SharedArrayBuffer must 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. transferID:uint32_t
-  kSharedArrayBuffer = 'u',
+  // Shared array buffer (transferred). transferID:uint32_t
+  kSharedArrayBufferTransfer = 'u',
   // Compiled WebAssembly module. encodingType:(one-byte tag).
   // If encodingType == 'y' (raw bytes):
   //  wasmWireByteLength:uint32_t, then raw data
@@ -269,7 +269,6 @@ 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);
 }
 
@@ -443,7 +442,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(Handle<JSArrayBuffer>::cast(receiver));
+      return WriteJSArrayBuffer(JSArrayBuffer::cast(*receiver));
     case JS_TYPED_ARRAY_TYPE:
     case JS_DATA_VIEW_TYPE:
       return WriteJSArrayBufferView(JSArrayBufferView::cast(*receiver));
@@ -725,37 +724,28 @@ Maybe<bool> ValueSerializer::WriteJSSet(Handle<JSSet> set) {
   return Just(true);
 }
 
-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);
-  }
-
+Maybe<bool> ValueSerializer::WriteJSArrayBuffer(JSArrayBuffer* array_buffer) {
   uint32_t* transfer_entry = array_buffer_transfer_map_.Find(array_buffer);
   if (transfer_entry) {
-    WriteTag(SerializationTag::kArrayBufferTransfer);
+    WriteTag(array_buffer->is_shared()
+                 ? SerializationTag::kSharedArrayBufferTransfer
+                 : 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, array_buffer);
+    ThrowDataCloneError(MessageTemplate::kDataCloneError, handle(array_buffer));
     return Nothing<bool>();
   }
   WriteTag(SerializationTag::kArrayBuffer);
@@ -1115,7 +1105,7 @@ MaybeHandle<Object> ValueDeserializer::ReadObjectInternal() {
       const bool is_shared = false;
       return ReadTransferredJSArrayBuffer(is_shared);
     }
-    case SerializationTag::kSharedArrayBuffer: {
+    case SerializationTag::kSharedArrayBufferTransfer: {
       const bool is_shared = true;
       return ReadTransferredJSArrayBuffer(is_shared);
     }

src/value-serializer.h

@@ -112,8 +112,7 @@ 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(Handle<JSArrayBuffer> array_buffer)
-      WARN_UNUSED_RESULT;
+  Maybe<bool> WriteJSArrayBuffer(JSArrayBuffer* array_buffer);
   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,7 +19,6 @@ namespace {
 
 using ::testing::_;
 using ::testing::Invoke;
-using ::testing::Return;
 
 class ValueSerializerTest : public TestWithIsolate {
  protected:
@@ -130,22 +129,15 @@ class ValueSerializerTest : public TestWithIsolate {
     encoded_data_functor(buffer);
   }
 
-  template <typename InputFunctor, typename MessageFunctor>
-  void InvalidEncodeTest(const InputFunctor& input_functor,
-                         const MessageFunctor& functor) {
+  template <typename MessageFunctor>
+  void InvalidEncodeTest(const char* source, const MessageFunctor& functor) {
     Context::Scope scope(serialization_context());
     TryCatch try_catch(isolate());
-    Local<Value> input_value = input_functor();
+    Local<Value> input_value = EvaluateScriptForInput(source);
     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>) {});
   }
@@ -2050,8 +2042,7 @@ class ValueSerializerTestWithSharedArrayBufferTransfer
  protected:
   static const size_t kTestByteLength = 4;
 
-  ValueSerializerTestWithSharedArrayBufferTransfer()
-      : serializer_delegate_(this) {
+  ValueSerializerTestWithSharedArrayBufferTransfer() {
     const uint8_t data[kTestByteLength] = {0x00, 0x01, 0x80, 0xff};
     memcpy(data_, data, kTestByteLength);
     {
@@ -2069,6 +2060,10 @@ 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_);
   }
@@ -2085,39 +2080,6 @@ 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];
@@ -2130,10 +2092,6 @@ 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());
@@ -2165,6 +2123,12 @@ 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() })");