Several bug fixes related to MessageChannel/MessagePort

lib/internal/bootstrap/node.js

@@ -125,6 +125,10 @@
       setupGlobalURL();
     }
 
+    if (process.binding('config').experimentalWorker) {
+      setupDOMException();
+    }
+
     // On OpenBSD process.execPath will be relative unless we
     // get the full path before process.execPath is used.
     if (process.platform === 'openbsd') {
@@ -405,6 +409,11 @@
     });
   }
 
+  function setupDOMException() {
+    // Registers the constructor with C++.
+    NativeModule.require('internal/domexception');
+  }
+
   function setupInspector(originalConsole, wrappedConsole, CJSModule) {
     if (!process.config.variables.v8_enable_inspector) {
       return;

lib/internal/domexception.js

@@ -0,0 +1,83 @@
+'use strict';
+
+const { internalBinding } = require('internal/bootstrap/loaders');
+const { registerDOMException } = internalBinding('messaging');
+const { ERR_INVALID_THIS } = require('internal/errors').codes;
+
+const internalsMap = new WeakMap();
+
+const nameToCodeMap = new Map();
+
+class DOMException extends Error {
+  constructor(message = '', name = 'Error') {
+    super();
+    internalsMap.set(this, {
+      message: `${message}`,
+      name: `${name}`
+    });
+  }
+
+  get name() {
+    const internals = internalsMap.get(this);
+    if (internals === undefined) {
+      throw new ERR_INVALID_THIS('DOMException');
+    }
+    return internals.name;
+  }
+
+  get message() {
+    const internals = internalsMap.get(this);
+    if (internals === undefined) {
+      throw new ERR_INVALID_THIS('DOMException');
+    }
+    return internals.message;
+  }
+
+  get code() {
+    const internals = internalsMap.get(this);
+    if (internals === undefined) {
+      throw new ERR_INVALID_THIS('DOMException');
+    }
+    const code = nameToCodeMap.get(internals.name);
+    return code === undefined ? 0 : code;
+  }
+}
+
+for (const [name, codeName, value] of [
+  ['IndexSizeError', 'INDEX_SIZE_ERR', 1],
+  ['DOMStringSizeError', 'DOMSTRING_SIZE_ERR', 2],
+  ['HierarchyRequestError', 'HIERARCHY_REQUEST_ERR', 3],
+  ['WrongDocumentError', 'WRONG_DOCUMENT_ERR', 4],
+  ['InvalidCharacterError', 'INVALID_CHARACTER_ERR', 5],
+  ['NoDataAllowedError', 'NO_DATA_ALLOWED_ERR', 6],
+  ['NoModificationAllowedError', 'NO_MODIFICATION_ALLOWED_ERR', 7],
+  ['NotFoundError', 'NOT_FOUND_ERR', 8],
+  ['NotSupportedError', 'NOT_SUPPORTED_ERR', 9],
+  ['InUseAttributeError', 'INUSE_ATTRIBUTE_ERR', 10],
+  ['InvalidStateError', 'INVALID_STATE_ERR', 11],
+  ['SyntaxError', 'SYNTAX_ERR', 12],
+  ['InvalidModificationError', 'INVALID_MODIFICATION_ERR', 13],
+  ['NamespaceError', 'NAMESPACE_ERR', 14],
+  ['InvalidAccessError', 'INVALID_ACCESS_ERR', 15],
+  ['ValidationError', 'VALIDATION_ERR', 16],
+  ['TypeMismatchError', 'TYPE_MISMATCH_ERR', 17],
+  ['SecurityError', 'SECURITY_ERR', 18],
+  ['NetworkError', 'NETWORK_ERR', 19],
+  ['AbortError', 'ABORT_ERR', 20],
+  ['URLMismatchError', 'URL_MISMATCH_ERR', 21],
+  ['QuotaExceededError', 'QUOTA_EXCEEDED_ERR', 22],
+  ['TimeoutError', 'TIMEOUT_ERR', 23],
+  ['InvalidNodeTypeError', 'INVALID_NODE_TYPE_ERR', 24],
+  ['DataCloneError', 'DATA_CLONE_ERR', 25]
+  // There are some more error names, but since they don't have codes assigned,
+  // we don't need to care about them.
+]) {
+  const desc = { enumerable: true, value };
+  Object.defineProperty(DOMException, codeName, desc);
+  Object.defineProperty(DOMException.prototype, codeName, desc);
+  nameToCodeMap.set(name, value);
+}
+
+module.exports = DOMException;
+
+registerDOMException(DOMException);

node.gyp

@@ -106,6 +106,7 @@
       'lib/internal/constants.js',
       'lib/internal/dns/promises.js',
       'lib/internal/dns/utils.js',
+      'lib/internal/domexception.js',
       'lib/internal/encoding.js',
       'lib/internal/errors.js',
       'lib/internal/error-serdes.js',

src/env.h

@@ -322,6 +322,7 @@ struct PackageConfig {
   V(buffer_prototype_object, v8::Object)                                      \
   V(context, v8::Context)                                                     \
   V(domain_callback, v8::Function)                                            \
+  V(domexception_function, v8::Function)                                      \
   V(fdclose_constructor_template, v8::ObjectTemplate)                         \
   V(fd_constructor_template, v8::ObjectTemplate)                              \
   V(filehandlereadwrap_template, v8::ObjectTemplate)                          \

src/node_messaging.cc

@@ -144,6 +144,21 @@ void Message::AddMessagePort(std::unique_ptr<MessagePortData>&& data) {
 
 namespace {
 
+void ThrowDataCloneException(Environment* env, Local<String> message) {
+  Local<Value> argv[] = {
+    message,
+    FIXED_ONE_BYTE_STRING(env->isolate(), "DataCloneError")
+  };
+  Local<Value> exception;
+  Local<Function> domexception_ctor = env->domexception_function();
+  CHECK(!domexception_ctor.IsEmpty());
+  if (!domexception_ctor->NewInstance(env->context(), arraysize(argv), argv)
+          .ToLocal(&exception)) {
+    return;
+  }
+  env->isolate()->ThrowException(exception);
+}
+
 // This tells V8 how to serialize objects that it does not understand
 // (e.g. C++ objects) into the output buffer, in a way that our own
 // DeserializerDelegate understands how to unpack.
@@ -153,7 +168,7 @@ class SerializerDelegate : public ValueSerializer::Delegate {
       : env_(env), context_(context), msg_(m) {}
 
   void ThrowDataCloneError(Local<String> message) override {
-    env_->isolate()->ThrowException(Exception::Error(message));
+    ThrowDataCloneException(env_, message);
   }
 
   Maybe<bool> WriteHostObject(Isolate* isolate, Local<Object> object) override {
@@ -224,7 +239,8 @@ class SerializerDelegate : public ValueSerializer::Delegate {
 Maybe<bool> Message::Serialize(Environment* env,
                                Local<Context> context,
                                Local<Value> input,
-                               Local<Value> transfer_list_v) {
+                               Local<Value> transfer_list_v,
+                               Local<Object> source_port) {
   HandleScope handle_scope(env->isolate());
   Context::Scope context_scope(context);
 
@@ -258,8 +274,23 @@ Maybe<bool> Message::Serialize(Environment* env,
         continue;
       } else if (env->message_port_constructor_template()
                     ->HasInstance(entry)) {
+        // Check if the source MessagePort is being transferred.
+        if (!source_port.IsEmpty() && entry == source_port) {
+          ThrowDataCloneException(
+              env,
+              FIXED_ONE_BYTE_STRING(env->isolate(),
+                                    "Transfer list contains source port"));
+          return Nothing<bool>();
+        }
         MessagePort* port = Unwrap<MessagePort>(entry.As<Object>());
-        CHECK_NE(port, nullptr);
+        if (port == nullptr || port->IsDetached()) {
+          ThrowDataCloneException(
+              env,
+              FIXED_ONE_BYTE_STRING(
+                  env->isolate(),
+                  "MessagePort in transfer list is already detached"));
+          return Nothing<bool>();
+        }
         delegate.ports_.push_back(port);
         continue;
       }
@@ -395,6 +426,10 @@ uv_async_t* MessagePort::async() {
   return reinterpret_cast<uv_async_t*>(GetHandle());
 }
 
+bool MessagePort::IsDetached() const {
+  return data_ == nullptr || IsHandleClosing();
+}
+
 void MessagePort::TriggerAsync() {
   if (IsHandleClosing()) return;
   CHECK_EQ(uv_async_send(async()), 0);
@@ -537,36 +572,69 @@ std::unique_ptr<MessagePortData> MessagePort::Detach() {
 }
 
 
-void MessagePort::Send(Message&& message) {
-  Mutex::ScopedLock lock(*data_->sibling_mutex_);
-  if (data_->sibling_ == nullptr)
-    return;
-  data_->sibling_->AddToIncomingQueue(std::move(message));
-}
+Maybe<bool> MessagePort::PostMessage(Environment* env,
+                                     Local<Value> message_v,
+                                     Local<Value> transfer_v) {
+  Isolate* isolate = env->isolate();
+  Local<Object> obj = object(isolate);
+  Local<Context> context = obj->CreationContext();
 
-void MessagePort::Send(const FunctionCallbackInfo<Value>& args) {
-  Environment* env = Environment::GetCurrent(args);
-  Local<Context> context = object(env->isolate())->CreationContext();
   Message msg;
-  if (msg.Serialize(env, context, args[0], args[1])
-          .IsNothing()) {
-    return;
+
+  // Per spec, we need to both check if transfer list has the source port, and
+  // serialize the input message, even if the MessagePort is closed or detached.
+
+  Maybe<bool> serialization_maybe =
+      msg.Serialize(env, context, message_v, transfer_v, obj);
+  if (data_ == nullptr) {
+    return serialization_maybe;
   }
-  Send(std::move(msg));
+  if (serialization_maybe.IsNothing()) {
+    return Nothing<bool>();
+  }
+
+  Mutex::ScopedLock lock(*data_->sibling_mutex_);
+  bool doomed = false;
+
+  // Check if the target port is posted to itself.
+  if (data_->sibling_ != nullptr) {
+    for (const auto& port_data : msg.message_ports()) {
+      if (data_->sibling_ == port_data.get()) {
+        doomed = true;
+        ProcessEmitWarning(env, "The target port was posted to itself, and "
+                                "the communication channel was lost");
+        break;
+      }
+    }
+  }
+
+  if (data_->sibling_ == nullptr || doomed)
+    return Just(true);
+
+  data_->sibling_->AddToIncomingQueue(std::move(msg));
+  return Just(true);
 }
 
 void MessagePort::PostMessage(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-  MessagePort* port;
-  ASSIGN_OR_RETURN_UNWRAP(&port, args.This());
-  if (!port->data_) {
-    return THROW_ERR_CLOSED_MESSAGE_PORT(env);
-  }
   if (args.Length() == 0) {
     return THROW_ERR_MISSING_ARGS(env, "Not enough arguments to "
                                        "MessagePort.postMessage");
   }
-  port->Send(args);
+
+  MessagePort* port = Unwrap<MessagePort>(args.This());
+  // Even if the backing MessagePort object has already been deleted, we still
+  // want to serialize the message to ensure spec-compliant behavior w.r.t.
+  // transfers.
+  if (port == nullptr) {
+    Message msg;
+    Local<Object> obj = args.This();
+    Local<Context> context = obj->CreationContext();
+    USE(msg.Serialize(env, context, args[0], args[1], obj));
+    return;
+  }
+
+  port->PostMessage(env, args[0], args[1]);
 }
 
 void MessagePort::Start() {
@@ -688,6 +756,13 @@ static void MessageChannel(const FunctionCallbackInfo<Value>& args) {
       .FromJust();
 }
 
+static void RegisterDOMException(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  CHECK_EQ(args.Length(), 1);
+  CHECK(args[0]->IsFunction());
+  env->set_domexception_function(args[0].As<Function>());
+}
+
 static void InitMessaging(Local<Object> target,
                           Local<Value> unused,
                           Local<Context> context,
@@ -708,6 +783,8 @@ static void InitMessaging(Local<Object> target,
               env->message_port_constructor_string(),
               GetMessagePortConstructor(env, context).ToLocalChecked())
                   .FromJust();
+
+  env->SetMethod(target, "registerDOMException", RegisterDOMException);
 }
 
 }  // anonymous namespace

src/node_messaging.h

@@ -32,10 +32,14 @@ class Message {
   // Serialize a JS value, and optionally transfer objects, into this message.
   // The Message object retains ownership of all transferred objects until
   // deserialization.
+  // The source_port parameter, if provided, will make Serialize() throw a
+  // "DataCloneError" DOMException if source_port is found in transfer_list.
   v8::Maybe<bool> Serialize(Environment* env,
                             v8::Local<v8::Context> context,
                             v8::Local<v8::Value> input,
-                            v8::Local<v8::Value> transfer_list);
+                            v8::Local<v8::Value> transfer_list,
+                            v8::Local<v8::Object> source_port =
+                                v8::Local<v8::Object>());
 
   // Internal method of Message that is called when a new SharedArrayBuffer
   // object is encountered in the incoming value's structure.
@@ -44,6 +48,13 @@ class Message {
   // and that transfers ownership of `data` to this message.
   void AddMessagePort(std::unique_ptr<MessagePortData>&& data);
 
+  // The MessagePorts that will be transferred, as recorded by Serialize().
+  // Used for warning user about posting the target MessagePort to itself,
+  // which will as a side effect destroy the communication channel.
+  const std::vector<std::unique_ptr<MessagePortData>>& message_ports() const {
+    return message_ports_;
+  }
+
  private:
   MallocedBuffer<char> main_message_buf_;
   std::vector<MallocedBuffer<char>> array_buffer_contents_;
@@ -122,10 +133,11 @@ class MessagePort : public HandleWrap {
                           std::unique_ptr<MessagePortData> data = nullptr);
 
   // Send a message, i.e. deliver it into the sibling's incoming queue.
-  // If there is no sibling, i.e. this port is closed,
-  // this message is silently discarded.
-  void Send(Message&& message);
-  void Send(const v8::FunctionCallbackInfo<v8::Value>& args);
+  // If this port is closed, or if there is no sibling, this message is
+  // serialized with transfers, then silently discarded.
+  v8::Maybe<bool> PostMessage(Environment* env,
+                              v8::Local<v8::Value> message,
+                              v8::Local<v8::Value> transfer);
   // Deliver a single message into this port's incoming queue.
   void AddToIncomingQueue(Message&& message);
 
@@ -157,6 +169,15 @@ class MessagePort : public HandleWrap {
   void Close(
       v8::Local<v8::Value> close_callback = v8::Local<v8::Value>()) override;
 
+  // Returns true if either data_ has been freed, or if the handle is being
+  // closed. Equivalent to the [[Detached]] internal slot in the HTML Standard.
+  //
+  // If checking if a JavaScript MessagePort object is detached, this method
+  // alone is often not enough, since the backing C++ MessagePort object may
+  // have been deleted already. For all intents and purposes, an object with a
+  // NULL pointer to the C++ MessagePort object is also detached.
+  inline bool IsDetached() const;
+
   size_t self_size() const override;
 
  private: