From 712163067fd64983232ee168fbe75c04eee53c6b Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 30 Nov 2021 17:16:25 -0800 Subject: [PATCH] src,lib: replace DOMException with native impl Refs: https://github.com/nodejs/node/issues/41038 Signed-off-by: James M Snell --- lib/internal/per_context/domexception.js | 122 ------------------ lib/internal/util.js | 4 +- lib/internal/webstreams/readablestream.js | 2 +- lib/internal/webstreams/transfer.js | 80 +----------- lib/internal/webstreams/transformstream.js | 2 +- lib/internal/webstreams/writablestream.js | 2 +- lib/internal/worker/io.js | 6 +- src/api/environment.cc | 1 - src/node_errors.cc | 21 +++ src/node_errors.h | 5 + src/node_messaging.cc | 39 +----- .../test-errors-cloneabledomexception.js | 21 +++ .../parallel/test-worker-message-port-move.js | 5 +- test/wpt/README.md | 2 +- test/wpt/test-performance-timeline.js | 2 +- test/wpt/test-streams.js | 2 +- test/wpt/test-user-timing.js | 2 +- test/wpt/test-webcrypto.js | 2 +- 18 files changed, 72 insertions(+), 248 deletions(-) delete mode 100644 lib/internal/per_context/domexception.js create mode 100644 test/parallel/test-errors-cloneabledomexception.js diff --git a/lib/internal/per_context/domexception.js b/lib/internal/per_context/domexception.js deleted file mode 100644 index 06bb3fa3053cd5..00000000000000 --- a/lib/internal/per_context/domexception.js +++ /dev/null @@ -1,122 +0,0 @@ -'use strict'; - -const { - Error, - ObjectDefineProperties, - ObjectDefineProperty, - SafeWeakMap, - SafeMap, - SymbolToStringTag, - TypeError, -} = primordials; - -class ERR_INVALID_THIS extends TypeError { - constructor(type) { - super('Value of "this" must be of ' + type); - } - - get code() { return 'ERR_INVALID_THIS'; } -} - -let internalsMap; -let nameToCodeMap; -let isInitialized = false; - -// We need to instantiate the maps lazily because they render -// the snapshot non-rehashable. -// https://bugs.chromium.org/p/v8/issues/detail?id=6593 -function ensureInitialized() { - if (isInitialized) { - return; - } - internalsMap = new SafeWeakMap(); - nameToCodeMap = new SafeMap(); - forEachCode((name, codeName, value) => { - nameToCodeMap.set(name, value); - }); - isInitialized = true; -} - -class DOMException extends Error { - constructor(message = '', name = 'Error') { - ensureInitialized(); - super(); - internalsMap.set(this, { - message: `${message}`, - name: `${name}` - }); - } - - get name() { - ensureInitialized(); - const internals = internalsMap.get(this); - if (internals === undefined) { - throw new ERR_INVALID_THIS('DOMException'); - } - return internals.name; - } - - get message() { - ensureInitialized(); - const internals = internalsMap.get(this); - if (internals === undefined) { - throw new ERR_INVALID_THIS('DOMException'); - } - return internals.message; - } - - get code() { - ensureInitialized(); - 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; - } -} - -ObjectDefineProperties(DOMException.prototype, { - [SymbolToStringTag]: { configurable: true, value: 'DOMException' }, - name: { enumerable: true, configurable: true }, - message: { enumerable: true, configurable: true }, - code: { enumerable: true, configurable: true } -}); - -function forEachCode(fn) { - fn('IndexSizeError', 'INDEX_SIZE_ERR', 1); - fn('DOMStringSizeError', 'DOMSTRING_SIZE_ERR', 2); - fn('HierarchyRequestError', 'HIERARCHY_REQUEST_ERR', 3); - fn('WrongDocumentError', 'WRONG_DOCUMENT_ERR', 4); - fn('InvalidCharacterError', 'INVALID_CHARACTER_ERR', 5); - fn('NoDataAllowedError', 'NO_DATA_ALLOWED_ERR', 6); - fn('NoModificationAllowedError', 'NO_MODIFICATION_ALLOWED_ERR', 7); - fn('NotFoundError', 'NOT_FOUND_ERR', 8); - fn('NotSupportedError', 'NOT_SUPPORTED_ERR', 9); - fn('InUseAttributeError', 'INUSE_ATTRIBUTE_ERR', 10); - fn('InvalidStateError', 'INVALID_STATE_ERR', 11); - fn('SyntaxError', 'SYNTAX_ERR', 12); - fn('InvalidModificationError', 'INVALID_MODIFICATION_ERR', 13); - fn('NamespaceError', 'NAMESPACE_ERR', 14); - fn('InvalidAccessError', 'INVALID_ACCESS_ERR', 15); - fn('ValidationError', 'VALIDATION_ERR', 16); - fn('TypeMismatchError', 'TYPE_MISMATCH_ERR', 17); - fn('SecurityError', 'SECURITY_ERR', 18); - fn('NetworkError', 'NETWORK_ERR', 19); - fn('AbortError', 'ABORT_ERR', 20); - fn('URLMismatchError', 'URL_MISMATCH_ERR', 21); - fn('QuotaExceededError', 'QUOTA_EXCEEDED_ERR', 22); - fn('TimeoutError', 'TIMEOUT_ERR', 23); - fn('InvalidNodeTypeError', 'INVALID_NODE_TYPE_ERR', 24); - fn('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. -} - -forEachCode((name, codeName, value) => { - const desc = { enumerable: true, value }; - ObjectDefineProperty(DOMException, codeName, desc); - ObjectDefineProperty(DOMException.prototype, codeName, desc); -}); - -exports.DOMException = DOMException; diff --git a/lib/internal/util.js b/lib/internal/util.js index e0f26f35717219..cbc47ba5589d46 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -468,12 +468,12 @@ function createDeferredPromise() { let _DOMException; const lazyDOMExceptionClass = () => { - _DOMException ??= internalBinding('messaging').DOMException; + _DOMException ??= internalBinding('errors').DOMException; return _DOMException; }; const lazyDOMException = hideStackFrames((message, name) => { - _DOMException ??= internalBinding('messaging').DOMException; + _DOMException ??= internalBinding('errors').DOMException; return new _DOMException(message, name); }); diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index c9b988d6fee0f7..6ec02cef680476 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -40,7 +40,7 @@ const { const { DOMException, -} = internalBinding('messaging'); +} = internalBinding('errors'); const { isArrayBufferView, diff --git a/lib/internal/webstreams/transfer.js b/lib/internal/webstreams/transfer.js index 985d7e86738f35..4b1d2f9aa94621 100644 --- a/lib/internal/webstreams/transfer.js +++ b/lib/internal/webstreams/transfer.js @@ -13,7 +13,7 @@ const { const { DOMException, -} = internalBinding('messaging'); +} = internalBinding('errors'); const { ReadableStream, @@ -40,66 +40,6 @@ const { 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 +73,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 +96,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 +136,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 +165,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 +180,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 +222,4 @@ module.exports = { newCrossRealmWritableSink, CrossRealmTransformWritableSink, CrossRealmTransformReadableSource, - CloneableDOMException, - InternalCloneableDOMException, }; diff --git a/lib/internal/webstreams/transformstream.js b/lib/internal/webstreams/transformstream.js index b4e690daa98c4a..cacb989d4edf25 100644 --- a/lib/internal/webstreams/transformstream.js +++ b/lib/internal/webstreams/transformstream.js @@ -22,7 +22,7 @@ const { const { DOMException, -} = internalBinding('messaging'); +} = internalBinding('errors'); const { createDeferredPromise, diff --git a/lib/internal/webstreams/writablestream.js b/lib/internal/webstreams/writablestream.js index dba7560c549a9a..7d6f4662fdca35 100644 --- a/lib/internal/webstreams/writablestream.js +++ b/lib/internal/webstreams/writablestream.js @@ -28,7 +28,7 @@ const { const { DOMException, -} = internalBinding('messaging'); +} = internalBinding('errors'); const { createDeferredPromise, diff --git a/lib/internal/worker/io.js b/lib/internal/worker/io.js index 5d03f43ef6df62..c037c287c6e557 100644 --- a/lib/internal/worker/io.js +++ b/lib/internal/worker/io.js @@ -32,8 +32,12 @@ const { receiveMessageOnPort: receiveMessageOnPort_, stopMessagePort, checkMessagePort, - DOMException, } = internalBinding('messaging'); + +const { + DOMException, +} = internalBinding('errors'); + const { getEnvMessagePort } = internalBinding('worker'); diff --git a/src/api/environment.cc b/src/api/environment.cc index 1ec1e8bc8c1896..9acfac8f3cdd6e 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -669,7 +669,6 @@ Maybe InitializePrimordials(Local context) { } static const char* context_files[] = {"internal/per_context/primordials", - "internal/per_context/domexception", "internal/per_context/messageport", nullptr}; diff --git a/src/node_errors.cc b/src/node_errors.cc index b015ca5db8cd51..7125125bcd2f88 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -1175,6 +1175,27 @@ BaseObjectPtr DOMException::Create( return MakeBaseObject(env, obj, message, name); } +BaseObjectPtr DOMException::Create( + Environment* env, + Local message, + const std::string& name) { + HandleScope scope(env->isolate()); + + Local ctor; + if (!GetConstructorTemplate(env)->GetFunction(env->context()).ToLocal(&ctor)) + return BaseObjectPtr(); + + Local obj; + if (!ctor->NewInstance(env->context()).ToLocal(&obj)) + return BaseObjectPtr(); + + return MakeBaseObject( + env, + obj, + message, + String::NewFromUtf8(env->isolate(), name.c_str()).ToLocalChecked()); +} + void DOMException::New(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); diff --git a/src/node_errors.h b/src/node_errors.h index c1d0ac5d81fbb1..608ae88d5d6763 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -298,6 +298,11 @@ class DOMException : public BaseObject { const std::string& message, const std::string& name = "DOMException"); + static BaseObjectPtr Create( + Environment* env, + v8::Local message, + const std::string& name = "DOMException"); + static void New(const v8::FunctionCallbackInfo& args); static void GetMessage(const v8::FunctionCallbackInfo& args); static void GetName(const v8::FunctionCallbackInfo& args); diff --git a/src/node_messaging.cc b/src/node_messaging.cc index a1f28d4746d07f..05728e1f6cba56 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -246,33 +246,11 @@ MaybeLocal GetEmitMessageFunction(Local context) { return emit_message_val.As(); } -MaybeLocal GetDOMException(Local context) { - Isolate* isolate = context->GetIsolate(); - Local per_context_bindings; - Local domexception_ctor_val; - if (!GetPerContextExports(context).ToLocal(&per_context_bindings) || - !per_context_bindings->Get(context, - FIXED_ONE_BYTE_STRING(isolate, "DOMException")) - .ToLocal(&domexception_ctor_val)) { - return MaybeLocal(); - } - CHECK(domexception_ctor_val->IsFunction()); - Local domexception_ctor = domexception_ctor_val.As(); - return domexception_ctor; -} - void ThrowDataCloneException(Local context, Local message) { - Isolate* isolate = context->GetIsolate(); - Local argv[] = {message, - FIXED_ONE_BYTE_STRING(isolate, "DataCloneError")}; - Local exception; - Local domexception_ctor; - if (!GetDOMException(context).ToLocal(&domexception_ctor) || - !domexception_ctor->NewInstance(context, arraysize(argv), argv) - .ToLocal(&exception)) { - return; - } - isolate->ThrowException(exception); + Environment* env = Environment::GetCurrent(context); + BaseObjectPtr exception = + DOMException::Create(env, message, "DataCloneError"); + env->isolate()->ThrowException(exception->object()); } // This tells V8 how to serialize objects that it does not understand @@ -1483,15 +1461,6 @@ static void InitMessaging(Local target, env->SetMethod(target, "setDeserializerCreateObjectFunction", SetDeserializerCreateObjectFunction); env->SetMethod(target, "broadcastChannel", BroadcastChannel); - - { - Local domexception = GetDOMException(context).ToLocalChecked(); - target - ->Set(context, - FIXED_ONE_BYTE_STRING(env->isolate(), "DOMException"), - domexception) - .Check(); - } } static void RegisterExternalReferences(ExternalReferenceRegistry* registry) { diff --git a/test/parallel/test-errors-cloneabledomexception.js b/test/parallel/test-errors-cloneabledomexception.js new file mode 100644 index 00000000000000..0cb5038a204c2c --- /dev/null +++ b/test/parallel/test-errors-cloneabledomexception.js @@ -0,0 +1,21 @@ +'use strict'; + +const common = require('../common'); + +const { strictEqual, ok } = require('assert'); + +const exception = new DOMException('foo', 'AbortError'); +strictEqual(exception.name, 'AbortError'); +strictEqual(exception.message, 'foo'); +strictEqual(exception.code, 20); + +const mc = new MessageChannel; +mc.port1.onmessage = common.mustCall(({data}) => { + ok(data instanceof DOMException); + strictEqual(data.name, 'AbortError'); + strictEqual(data.message, 'foo'); + strictEqual(data.code, 20); + strictEqual(data.stack, exception.stack); + mc.port1.close(); +}); +mc.port2.postMessage(exception); diff --git a/test/parallel/test-worker-message-port-move.js b/test/parallel/test-worker-message-port-move.js index 44efd2e6a6b94f..75709fd263fbcf 100644 --- a/test/parallel/test-worker-message-port-move.js +++ b/test/parallel/test-worker-message-port-move.js @@ -15,7 +15,8 @@ Object.assign(context, { global: context, assert, MessagePort, - MessageChannel + MessageChannel, + Error, }); vm.runInContext('(' + function() { @@ -51,7 +52,7 @@ vm.runInContext('(' + function() { port.postMessage(global); } catch (e) { assert.strictEqual(e.constructor.name, 'DOMException'); - assert(e instanceof Object); + assert.strictEqual(typeof e, 'object'); assert(e instanceof Error); threw = true; } diff --git a/test/wpt/README.md b/test/wpt/README.md index b8ded27ef37d78..113707b66de3cc 100644 --- a/test/wpt/README.md +++ b/test/wpt/README.md @@ -57,7 +57,7 @@ runner.setFlags(['--expose-internals']); // Set a script that will be executed in the worker before running the tests. runner.setInitScript(` const { internalBinding } = require('internal/test/binding'); - const { DOMException } = internalBinding('messaging'); + const { DOMException } = internalBinding('errors'); global.DOMException = DOMException; `); diff --git a/test/wpt/test-performance-timeline.js b/test/wpt/test-performance-timeline.js index 12f1abd3a52a32..a9ad26a6fcd033 100644 --- a/test/wpt/test-performance-timeline.js +++ b/test/wpt/test-performance-timeline.js @@ -22,7 +22,7 @@ runner.setInitScript(` global.performance = performance; const { internalBinding } = require('internal/test/binding'); - const { DOMException } = internalBinding('messaging'); + const { DOMException } = internalBinding('errors'); global.DOMException = DOMException; `); diff --git a/test/wpt/test-streams.js b/test/wpt/test-streams.js index 987676d8c49125..216926f88916c9 100644 --- a/test/wpt/test-streams.js +++ b/test/wpt/test-streams.js @@ -27,7 +27,7 @@ runner.setInitScript(` } = require('stream/web'); const { internalBinding } = require('internal/test/binding'); - const { DOMException } = internalBinding('messaging'); + const { DOMException } = internalBinding('errors'); global.DOMException = DOMException; Object.defineProperties(global, { diff --git a/test/wpt/test-user-timing.js b/test/wpt/test-user-timing.js index 36d13297ba57cc..e9f5f2d4605d89 100644 --- a/test/wpt/test-user-timing.js +++ b/test/wpt/test-user-timing.js @@ -20,7 +20,7 @@ runner.setInitScript(` global.performance = performance; const { internalBinding } = require('internal/test/binding'); - const { DOMException } = internalBinding('messaging'); + const { DOMException } = internalBinding('errors'); global.DOMException = DOMException; `); diff --git a/test/wpt/test-webcrypto.js b/test/wpt/test-webcrypto.js index e8707a464f434c..42eb88e0faccce 100644 --- a/test/wpt/test-webcrypto.js +++ b/test/wpt/test-webcrypto.js @@ -19,7 +19,7 @@ runner.setInitScript(` crypto, } = require('internal/crypto/webcrypto'); const { internalBinding } = require('internal/test/binding'); - const { DOMException } = internalBinding('messaging'); + const { DOMException } = internalBinding('errors'); global.DOMException = DOMException; Object.defineProperties(global, {