src,lib: make DOMException cloneable

[RaisinTen]

This attepts to make DOMException cloneable by making the JSTransferable
constructor and the required symbols available to the per_context scripts.

Fixes: #41038
Signed-off-by: Darshan Sen raisinten@gmail.com

[RaisinTen]

Currently, calling postMessage() makes V8 throws a DataCloneError through:

node/src/node_messaging.cc

Line 286 in 12e3c74

void ThrowDataCloneError(Local<String> message) override {

Before recompiling V8, I would appreciate some review to make sure if the current approach is okay or if there's something obvious I'm missing here.

[jasnell]

Suggested change

	tmpl->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "BaseObject"));
	tmpl->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "JSTransferable"));

??

[RaisinTen]

@jasnell this is creating the BaseObject constructor template, so that JSTransferable can inherit from it. It has been taken from here:

node/src/env.cc

Line 1716 in ced5fc3

tmpl->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "BaseObject"));

[jasnell]

This is creating a new separate FunctionTemplate for BaseObject that is not the same as BaseObject. That seems like a breaking difference here. @addaleax ... what do you think here?

[jasnell]

If this is correct, comments here explaining what is happening and why would be good

[RaisinTen]

🤦 missed this. I too think this is the issue here, see - #41148 (comment). I noticed this yesterday too when I tried to locally move the same DOMException code to a context-aware module and it stopped throwing a DataCloneError. Given that the JSTransferable constructor is the only new thing coming from C++ land, I think this indeed is the root cause.

[RaisinTen]

I tried to make the BaseObject constructor available before the Environment is created in 2e7f18e, so that JSTransferable can inherit from the same object.
Good news - postMessage() doesn't throw a DataCloneError anymore and calls kClone
Bad news - the kDeserialize method doesn't get called and the 'message' event doesn't get fired on mc.port1 which keeps the event loop alive
However, still when I move the implementation to a context aware module, the DOMException object is sent over to port1. Could it be that the deserializeInfo string path doesn't work for per_context module objects? 🤔

[RaisinTen]

@addaleax @joyeecheung any clue what's going wrong here?

[joyeecheung]

I am guessing this might be problematic - the builtins aren't really context aware and the code usually assume that there is only one set of these in each environment (which comes from the main context). It may not be enough to simply share the function templates.

[RaisinTen]

@joyeecheung

I am guessing this might be problematic - the builtins aren't really context aware

Why isn't a persistent pointing to the same object enough to share them across contexts if the BaseObject is not bound to a single V8 context?

the code usually assume that there is only one set of these in each environment (which comes from the main context)

This function does create just one BaseObject ctor template for each environment this is called from, like a singleton class, right?

It may not be enough to simply share the function templates.

Do you know what else we might need to share here?

[joyeecheung]

I was referring to the JSTransferrable constructor template, not sure why I added the comment here. On the other hand,

Bad news - the kDeserialize method doesn't get called and the 'message' event doesn't get fired on mc.port1 which keeps the event loop alive

I think it is caused by not sharing the symbols across worker threads, it is okay to call Symbol.for in a vm context from the same Environment since they share the same isolate, but if it is called from a worker then the symbol belongs to the symbol table of the worker and won't be recognized by the main instance - you can debug into where env()->messaging_deserialize_symbol() is used to confirm. If that's the case some work needs to be done to ensure that when creating the context for a worker, the symbol from the main context is used instead of being created, maybe with an optional parameter passed into NewContext()

[RaisinTen]

@joyeecheung for debugging purposes, I tried to replace the symbols with strings because those would not require more work to make make the object cloneable but I'm still seeing the same behavior where the message callback doesn't get called when I send the DOMException object using postMessage(). It probably means that we are not doing something correctly while exposing the JSTransferable ctor to the per_context scripts. Not sure what I'm doing wrong, any clue?

[RaisinTen]

@joyeecheung is there anything else we could do in order to share the function templates?

[RaisinTen]

@joyeecheung could you PTAL?

[joyeecheung]

Is the bug going away? If it is, I assume the templates are shared alright.

[RaisinTen]

@joyeecheung unfortunately, nope. :/
The bug still remains, even if we turn the symbol keys into string keys. So that would mean that there's something wrong with the way in which we are sharing the templates.

[DerekNonGeneric]

@RaisinTen, is there anything I can do to help move this forward?

Seems we need this for structuredClone feature-completeness (hopefully for v18.x) as we can't currently pass DOMException as an argument to structuredClone w/o throwing a new error in v17.x today (as you are likely aware).

[RaisinTen]

I just wanted to take a break from this PR and work on some other things for a while. If you want, you may cherry-pick the commits in this PR and create a new PR and add your changes on top of it.

[aduh95]

What's the status here? Do we still want to do this?

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[RaisinTen]

I think we can close this