src,lib: make DOMException cloneable

This attepts to make DOMException cloneable by making the JSTransferable constructor and the required symbols available to the per_context scripts. Refs: nodejs#41038 Signed-off-by: Darshan Sen <[email protected]>
RaisinTen · May 7, 2022 · da01057 · da01057
1 parent 3bc23f5
commit da01057
Show file tree

Hide file tree

Showing 12 changed files with 186 additions and 125 deletions.
diff --git a/lib/.eslintrc.yaml b/lib/.eslintrc.yaml
@@ -239,3 +239,4 @@ globals:
   module: false
   internalBinding: false
   primordials: false
+  JSTransferable: false
diff --git a/lib/internal/per_context/domexception.js b/lib/internal/per_context/domexception.js
@@ -3,16 +3,21 @@
 const {
   ErrorCaptureStackTrace,
   ErrorPrototype,
+  FunctionPrototypeCall,
   ObjectDefineProperties,
   ObjectDefineProperty,
   ObjectSetPrototypeOf,
-  SafeWeakMap,
   SafeMap,
   SafeSet,
+  SafeWeakMap,
+  SymbolFor,
   SymbolToStringTag,
   TypeError,
 } = primordials;
 
+const kClone = 'nodejs.worker_threads.clone';
+const kDeserialize = 'nodejs.worker_threads.deserialize';
+
 function throwInvalidThisError(Base, type) {
   const err = new Base();
   const key = 'ERR_INVALID_THIS';
@@ -46,13 +51,23 @@ const disusedNamesSet = new SafeSet()
   .add('NoDataAllowedError')
   .add('ValidationError');
 
-class DOMException {
+class DOMException extends JSTransferable {
   constructor(message = '', name = 'Error') {
-    ErrorCaptureStackTrace(this);
-    internalsMap.set(this, {
-      message: `${message}`,
-      name: `${name}`
-    });
+    super();
+
+    let code;
+    if (disusedNamesSet.has(name)) {
+      code = 0;
+    } else {
+      code = nameToCodeMap.get(name);
+      code = code === undefined ? 0 : code;
+    }
+
+    const o = { __proto__: null };
+    ErrorCaptureStackTrace(o);
+    FunctionPrototypeCall(this[kDeserialize],
+                          this,
+                          { name, message, code, stack: o.stack });
   }
 
   get name() {
@@ -76,13 +91,32 @@ class DOMException {
     if (internals === undefined) {
       throwInvalidThisError(TypeError, 'DOMException');
     }
+    return internals.code;
+  }
 
-    if (disusedNamesSet.has(internals.name)) {
-      return 0;
+  get stack() {
+    const internals = internalsMap.get(this);
+    if (internals === undefined) {
+      throwInvalidThisError(TypeError, 'DOMException');
     }
+    return internals.stack;
+  }
 
-    const code = nameToCodeMap.get(internals.name);
-    return code === undefined ? 0 : code;
+  [kClone]() {
+    const { name, message, code, stack } = this;
+    return {
+      data: { name, message, code, stack },
+      deserializeInfo: 'internal/per_context/domexception:DOMException',
+    };
+  }
+
+  [kDeserialize]({ name, message, code, stack }) {
+    internalsMap.set(this, {
+      name: `${name}`,
+      message: `${message}`,
+      code,
+      stack: `${stack}`,
+    });
   }
 }
 
@@ -91,7 +125,8 @@ ObjectDefineProperties(DOMException.prototype, {
   [SymbolToStringTag]: { configurable: true, value: 'DOMException' },
   name: { enumerable: true, configurable: true },
   message: { enumerable: true, configurable: true },
-  code: { enumerable: true, configurable: true }
+  code: { enumerable: true, configurable: true },
+  stack: { enumerable: true, configurable: true }
 });
 
 for (const { 0: name, 1: codeName, 2: value } of [

diff --git a/lib/internal/webstreams/transfer.js b/lib/internal/webstreams/transfer.js
@@ -1,9 +1,7 @@
 'use strict';
 
 const {
-  ObjectDefineProperties,
   PromiseResolve,
-  ReflectConstruct,
 } = primordials;
 
 const {
@@ -34,72 +32,6 @@ const {
 
 const assert = require('internal/assert');
 
-const {
-  makeTransferable,
-  kClone,
-  kDeserialize,
-} = require('internal/worker/js_transferable');
-
-// This class is a bit of a hack. The Node.js implementation of
-// DOMException is not transferable/cloneable. This provides us
-// with a variant that is. Unfortunately, it means playing around
-// a bit with the message, name, and code properties and the
-// prototype. We can revisit this if DOMException is ever made
-// properly cloneable.
-class CloneableDOMException extends DOMException {
-  constructor(message, name) {
-    super(message, name);
-    this[kDeserialize]({
-      message: this.message,
-      name: this.name,
-      code: this.code,
-    });
-    // eslint-disable-next-line no-constructor-return
-    return makeTransferable(this);
-  }
-
-  [kClone]() {
-    return {
-      data: {
-        message: this.message,
-        name: this.name,
-        code: this.code,
-      },
-      deserializeInfo:
-        'internal/webstreams/transfer:InternalCloneableDOMException'
-    };
-  }
-
-  [kDeserialize]({ message, name, code }) {
-    ObjectDefineProperties(this, {
-      message: {
-        configurable: true,
-        enumerable: true,
-        get() { return message; },
-      },
-      name: {
-        configurable: true,
-        enumerable: true,
-        get() { return name; },
-      },
-      code: {
-        configurable: true,
-        enumerable: true,
-        get() { return code; },
-      },
-    });
-  }
-}
-
-function InternalCloneableDOMException() {
-  return makeTransferable(
-    ReflectConstruct(
-      CloneableDOMException,
-      [],
-      DOMException));
-}
-InternalCloneableDOMException[kDeserialize] = () => {};
-
 class CrossRealmTransformReadableSource {
   constructor(port) {
     this[kState] = {
@@ -133,7 +65,7 @@ class CrossRealmTransformReadableSource {
     };
 
     port.onmessageerror = () => {
-      const error = new CloneableDOMException(
+      const error = new DOMException(
         'Internal transferred ReadableStream error',
         'DataCloneError');
       port.postMessage({ type: 'error', value: error });
@@ -156,10 +88,6 @@ class CrossRealmTransformReadableSource {
     try {
       this[kState].port.postMessage({ type: 'error', value: reason });
     } catch (error) {
-      if (error instanceof DOMException) {
-        // eslint-disable-next-line no-ex-assign
-        error = new CloneableDOMException(error.message, error.name);
-      }
       this[kState].port.postMessage({ type: 'error', value: error });
       throw error;
     } finally {
@@ -200,7 +128,7 @@ class CrossRealmTransformWritableSink {
       }
     };
     port.onmessageerror = () => {
-      const error = new CloneableDOMException(
+      const error = new DOMException(
         'Internal transferred ReadableStream error',
         'DataCloneError');
       port.postMessage({ type: 'error', value: error });
@@ -229,10 +157,6 @@ class CrossRealmTransformWritableSink {
     try {
       this[kState].port.postMessage({ type: 'chunk', value: chunk });
     } catch (error) {
-      if (error instanceof DOMException) {
-        // eslint-disable-next-line no-ex-assign
-        error = new CloneableDOMException(error.message, error.name);
-      }
       this[kState].port.postMessage({ type: 'error', value: error });
       this[kState].port.close();
       throw error;
@@ -248,10 +172,6 @@ class CrossRealmTransformWritableSink {
     try {
       this[kState].port.postMessage({ type: 'error', value: reason });
     } catch (error) {
-      if (error instanceof DOMException) {
-        // eslint-disable-next-line no-ex-assign
-        error = new CloneableDOMException(error.message, error.name);
-      }
       this[kState].port.postMessage({ type: 'error', value: error });
       throw error;
     } finally {
@@ -294,6 +214,4 @@ module.exports = {
   newCrossRealmWritableSink,
   CrossRealmTransformWritableSink,
   CrossRealmTransformReadableSource,
-  CloneableDOMException,
-  InternalCloneableDOMException,
 };
diff --git a/lib/internal/worker/js_transferable.js b/lib/internal/worker/js_transferable.js
@@ -9,16 +9,17 @@ const {
   StringPrototypeSplit,
 } = primordials;
 const {
-  messaging_deserialize_symbol,
   messaging_transfer_symbol,
-  messaging_clone_symbol,
   messaging_transfer_list_symbol
 } = internalBinding('symbols');
 const {
   JSTransferable,
   setDeserializerCreateObjectFunction
 } = internalBinding('messaging');
 
+const kClone = 'nodejs.worker_threads.clone';
+const kDeserialize = 'nodejs.worker_threads.deserialize';
+
 function setup() {
   // Register the handler that will be used when deserializing JS-based objects
   // from .postMessage() calls. The format of `deserializeInfo` is generally
@@ -27,7 +28,7 @@ function setup() {
     const { 0: module, 1: ctor } = StringPrototypeSplit(deserializeInfo, ':');
     const Ctor = require(module)[ctor];
     if (typeof Ctor !== 'function' ||
-        typeof Ctor.prototype[messaging_deserialize_symbol] !== 'function') {
+        typeof Ctor.prototype[kDeserialize] !== 'function') {
       // Not one of the official errors because one should not be able to get
       // here without messing with Node.js internals.
       // eslint-disable-next-line no-restricted-syntax
@@ -49,8 +50,8 @@ module.exports = {
   makeTransferable,
   setup,
   JSTransferable,
-  kClone: messaging_clone_symbol,
-  kDeserialize: messaging_deserialize_symbol,
+  kClone,
+  kDeserialize,
   kTransfer: messaging_transfer_symbol,
   kTransferList: messaging_transfer_list_symbol,
 };
diff --git a/src/api/environment.cc b/src/api/environment.cc
@@ -2,6 +2,7 @@
 #include "node_context_data.h"
 #include "node_errors.h"
 #include "node_internals.h"
+#include "node_messaging.h"
 #include "node_native_module_env.h"
 #include "node_options-inl.h"
 #include "node_platform.h"
@@ -681,6 +682,8 @@ Maybe<bool> InitializePrimordials(Local<Context> context) {
       FIXED_ONE_BYTE_STRING(isolate, "primordials");
   Local<String> global_string = FIXED_ONE_BYTE_STRING(isolate, "global");
   Local<String> exports_string = FIXED_ONE_BYTE_STRING(isolate, "exports");
+  Local<String> js_transferable_string =
+      FIXED_ONE_BYTE_STRING(isolate, "JSTransferable");
 
   // Create primordials first and make it available to per-context scripts.
   Local<Object> primordials = Object::New(isolate);
@@ -696,9 +699,15 @@ Maybe<bool> InitializePrimordials(Local<Context> context) {
                                         nullptr};
 
   for (const char** module = context_files; *module != nullptr; module++) {
-    std::vector<Local<String>> parameters = {
-        global_string, exports_string, primordials_string};
-    Local<Value> arguments[] = {context->Global(), exports, primordials};
+    std::vector<Local<String>> parameters = {global_string,
+                                             exports_string,
+                                             primordials_string,
+                                             js_transferable_string};
+    Local<Value> arguments[] = {
+        context->Global(),
+        exports,
+        primordials,
+        worker::JSTransferable::GetConstructorFunction(isolate)};
     MaybeLocal<Function> maybe_fn =
         native_module::NativeModuleEnv::LookupAndCompile(
             context, *module, &parameters, nullptr);

diff --git a/src/base_object.h b/src/base_object.h
@@ -112,6 +112,9 @@ class BaseObject : public MemoryRetainer {
   static v8::Local<v8::FunctionTemplate> GetConstructorTemplate(
       Environment* env);
 
+  static v8::Local<v8::FunctionTemplate> GetConstructorTemplate(
+      v8::Isolate* isolate);
+
   // Interface for transferring BaseObject instances using the .postMessage()
   // method of MessagePorts (and, by extension, Workers).
   // GetTransferMode() returns a transfer mode that indicates how to deal with

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -1289,8 +1289,18 @@ void Environment::set_process_exit_handler(
   ENVIRONMENT_STRONG_PERSISTENT_VALUES(V)
 #undef V
 
-v8::Local<v8::Context> Environment::context() const {
-  return PersistentToLocal::Strong(context_);
+#define V(PropertyName, TypeName)                                              \
+  inline v8::Local<TypeName> Environment::PropertyName() const {               \
+    return get_##PropertyName(isolate());                                      \
+  }                                                                            \
+  inline void Environment::set_##PropertyName(v8::Local<TypeName> value) {     \
+    set_##PropertyName(isolate(), value);                                      \
+  }
+  PER_ISOLATE_ETERNALS(V)
+#undef V
+
+  v8::Local<v8::Context> Environment::context() const {
+    return PersistentToLocal::Strong(context_);
 }
 
 }  // namespace node