src,lib: re-implement DOMException as a cloneable native object #41044

jasnell · 2021-12-01T01:38:54Z

The JavaScript-implemented DOMException is not structured cloneable without ugly hacks.

This re-implements it in C++ as a cloneable BaseObject.

Also makes AbortError structured cloneable.

Refs: #41038

Refs: nodejs#41038 Signed-off-by: James M Snell <jasnell@gmail.com>

nodejs-github-bot · 2021-12-02T21:00:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/41283/

RaisinTen · 2021-12-03T13:42:10Z

src/node_errors.h

@@ -270,6 +276,97 @@ void DecorateErrorStack(Environment* env,
                        const errors::TryCatchScope& try_catch);
 }  // namespace errors

+class DOMException : public BaseObject {


Why not inherit from JSTransferable?

node/src/node_messaging.h

Lines 318 to 321 in 3f51f05

// Provide a base class from which JS classes that should be transferable or

// cloneable by postMesssage() can inherit.

// See e.g. FileHandle in internal/fs/promises.js for an example.

class JSTransferable : public BaseObject {

Because we can't. See the referenced issue for detail: #41038

RaisinTen · 2021-12-03T13:53:43Z

src/node_errors.cc

+  }
+  Local<Object> err = Exception::Error(message_str).As<Object>();
+
+  if (!err->Set(env->context(), env->name_string(), name_str).FromJust()) {


Suggested change

if (!err->Set(env->context(), env->name_string(), name_str).FromJust()) {

if (err->Set(env->context(), env->name_string(), name_str).IsNothing()) {

Why crash the process in case of an ongoing exception?

RaisinTen · 2021-12-03T14:07:23Z

src/node_errors.cc

+      env,
+      obj,
+      message,
+      String::NewFromUtf8(env->isolate(), name.c_str()).ToLocalChecked());


Suggested change

String::NewFromUtf8(env->isolate(), name.c_str()).ToLocalChecked());

String::NewFromUtf8(env->isolate(), name.c_str())

.FromMaybe(Local<String>()));

Why crash the process in case of an ongoing exception?

RaisinTen · 2021-12-03T14:18:15Z

src/node_errors.cc

+  args.GetReturnValue().Set(ret);
+#undef V


Suggested change

args.GetReturnValue().Set(ret);

#undef V

#undef V

args.GetReturnValue().Set(ret);

Setting the return value does not need to be done in the scope of V, does it?

RaisinTen · 2021-12-03T14:38:12Z

src/node_errors.cc

+    const std::string& name)
+    : BaseObject(env, object) {
+  Local<Value> message_value =
+      String::NewFromUtf8(env->isolate(), message.c_str()).ToLocalChecked();


We can get rid of the calls to the *Check*() functions in the constructors and still signal to the caller if a failure occurs by using a factory function to construct these objects and return a nullish value when there is an exception. Such a thing is already implemented by the MessagePort class constructors:

node/src/node_messaging.h

Line 221 in 3f51f05

class MessagePort : public HandleWrap {

addaleax

This seems like a huge amount of unnecessary complexity, compared to making JSTransferable support transferables in the clone information … are you sure you want to do this?

addaleax · 2021-12-03T15:31:25Z

src/node_errors.cc

+    return Global<Value>();
+  }
+
+  return Global<Value>(env->isolate(), stack);


Why does this return a Global rather than a Local?

addaleax · 2021-12-03T15:31:59Z

src/node_errors.h

+ private:
+  v8::Global<v8::Value> message;
+  v8::Global<v8::Value> name;
+  v8::Global<v8::Value> stack;


Are these always strings?

jasnell · 2021-12-03T15:47:00Z

@addaleax:

This seems like a huge amount of unnecessary complexity, compared to making JSTransferable support transferables in the clone information … are you sure you want to do this?

That does not actually solve the problem here. The issue is that custom error objects like DOMException and AbortError are not cloneable without losing key bits of information. Making JSTransferable support transferables in clone is something we definitely want to do but that wouldn't solve the issue with custom error objects.

joyeecheung · 2021-12-06T09:52:47Z

src/node_errors.cc

+// This is annoying, but v8 does not give a nice way of getting a good
+// stack property so we have to create an error object and capture a
+// stack that way.
+Global<Value> MakeErrorStack(


It doesn't seem nice that we have to re-implement all these error properties in C++. If the only problem with implementing them in JS (which seems nicer in comparison since we can do multiple inheritance there) is getting hold of C++ stuff before internalBinding and require become available, I think we can simply put JSTransferable and the symbols to some object (primordials would do if we are being lazy and not want to invent a new argument) in InitializePrimordials and pass them into the per-context script for DOMException to use?

jasnell requested review from addaleax and joyeecheung December 1, 2021 01:38

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Dec 1, 2021

jasnell requested a review from benjamingr December 1, 2021 01:39

This comment has been minimized.

Sign in to view

jasnell force-pushed the cloneable-domexception branch from 7121630 to afdcc0e Compare December 1, 2021 15:01

This comment has been minimized.

Sign in to view

jasnell added 3 commits December 2, 2021 12:57

src: implement a structured cloneable DOMException

218f13d

Refs: nodejs#41038 Signed-off-by: James M Snell <jasnell@gmail.com>

src,lib: replace DOMException with native impl

65ecdd4

Refs: nodejs#41038 Signed-off-by: James M Snell <jasnell@gmail.com>

errors: make AbortError structured cloneable

ab28941

Refs: nodejs#41038 Signed-off-by: James M Snell <jasnell@gmail.com>

jasnell force-pushed the cloneable-domexception branch from afdcc0e to ab28941 Compare December 2, 2021 20:59

github-actions bot mentioned this pull request Dec 3, 2021

CI Reliability 2021-12-03 nodejs/reliability#133

Open

36 tasks

RaisinTen reviewed Dec 3, 2021

View reviewed changes

addaleax reviewed Dec 3, 2021

View reviewed changes

This was referenced Dec 4, 2021

CI Reliability 2021-12-04 nodejs/reliability#134

Open

CI Reliability 2021-12-05 nodejs/reliability#135

Open

CI Reliability 2021-12-06 nodejs/reliability#136

Open

joyeecheung reviewed Dec 6, 2021

View reviewed changes

github-actions bot mentioned this pull request Dec 7, 2021

CI Reliability 2021-12-07 nodejs/reliability#137

Open

46 tasks

jasnell closed this Dec 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src,lib: re-implement DOMException as a cloneable native object #41044

src,lib: re-implement DOMException as a cloneable native object #41044

jasnell commented Dec 1, 2021 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Dec 2, 2021

RaisinTen Dec 3, 2021

jasnell Dec 3, 2021

RaisinTen Dec 3, 2021

RaisinTen Dec 3, 2021

RaisinTen Dec 3, 2021

RaisinTen Dec 3, 2021

addaleax left a comment

addaleax Dec 3, 2021

addaleax Dec 3, 2021

jasnell commented Dec 3, 2021

joyeecheung Dec 6, 2021

	// Provide a base class from which JS classes that should be transferable or
	// cloneable by postMesssage() can inherit.
	// See e.g. FileHandle in internal/fs/promises.js for an example.
	class JSTransferable : public BaseObject {

	if (!err->Set(env->context(), env->name_string(), name_str).FromJust()) {
	if (err->Set(env->context(), env->name_string(), name_str).IsNothing()) {

	String::NewFromUtf8(env->isolate(), name.c_str()).ToLocalChecked());
	String::NewFromUtf8(env->isolate(), name.c_str())
	.FromMaybe(Local<String>()));

src,lib: re-implement DOMException as a cloneable native object #41044

src,lib: re-implement DOMException as a cloneable native object #41044

Conversation

jasnell commented Dec 1, 2021 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Dec 3, 2021

Choose a reason for hiding this comment

jasnell commented Dec 1, 2021 •

edited