[addaleax]

src: introduce BaseObject base FunctionTemplate

(shared with #33360)

worker: allow transferring/cloning generic BaseObjects

Extend support for transferring objects à la MessagePort to other
types of BaseObject subclasses, as well as implement cloning
support for cases in which destructive transferring is not needed
or optional.

(shared with #33360)

fixup! worker: allow transferring/cloning generic BaseObjects

(not shared, hence as a fixup commit)

src: add equality operators for BaseObjectPtr

worker: allow passing JS wrapper objects via postMessage

Enable JS wrapper objects to be used as transferable or cloneable
objects in postMessage() calls, by having them extend a C++-backed
class.

This requires a few internal changes:

• This commit adds the possibility for transferred objects to
  read/write JS values at the end of the serialization/deserialization
  phases.
• This commit adds the possibility for transferred objects to list
  sub-transferables, e.g. typically the public JS wrapper class
  would list its C++ handle in there.
• This commit adds usage of BaseObject in a few more places, because
  now during deserialization weakly held objects can also be involved,
  in addition to MessagePorts.

worker,fs: make FileHandle transferable

Allow passing FileHandle instances in the transfer list
of a .postMessage() call.

worker: emit 'messagerror' events for failed deserialization

This is much nicer than just treating exceptions as uncaught, and
enables reporting of exceptions from the internal C++ deserialization
machinery.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

@tniessen Just a heads up, this is where the conflicts I mentioned to you came from

@tniessen You will probably also want to include f172c2dc874a9f9575bd958e0d3eb828ced224ce in your PR. I missed the fact that returning a plain BaseObject* might not work in weird edge case situations, because objects like KeyObject or FileHandle are weak objects, and only holding a BaseObject* does not prevent them from being garbage-collected between returning them from Deserialize() and adding the corresponding JS object to the message data object.

@jasnell Happy to take suggestions on the API here. It’s a bit unfortunate that we can’t use the constructor and instead use this deserialization method to fill the object’s contents with the transferred data, but I don’t really know a way around this (See the comment in JSTransferable::Data::Deserialize() for details on why it seems necessary to me)

This pattern is actually pretty common in JS for "hydrating" objects in my experience - though it is admittedly less neat than doing everything in the constructor and always getting a fully initialized object.

I'm good with this API. The one thing that I would be concerned about is whether we'll always be certain that kDeserialize is always going to be called with a non-null/non-undefined value, otherwise the ({ handle }) deconstruction will fail.

It will always be called with the value in [kTransfer]().data, so yes, that should work out.

@tniessen I couldn’t figure out a good way to deal with this, sorry. I would suggest doing something similar in #33360, too, unless anybody has a good idea that doesn’t come with tons of extra complexity… on the plus side, this is not something that people are typically going to run into, and if they do, they are probably in a situation where they understand why the message can’t properly be transferred

Thank you for explaining these changes, it's very helpful :-) I'll try to do the same in my PR :)

@tniessen See the comment below – the last commit here changes this to actually emit an error, so you could also do that in your PR :)

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

Does this have an impact on the memory consumption or size of FileHandle objects?

According to the inspector, this adds 32 bytes in total, which seems to make sense to me (1 × vpointer + 3 × pointer-size fields in BaseObject).

For comparison, the close function on each FileHandle is 56 bytes in size.

For comparison, the close function on each FileHandle is 56 bytes in size.

That is actually pretty unfortunate to allocate a function per handle, but I guess maybe file handles are expected to be relatively expensive to hold?

@benjamingr I would love to see our objects become smaller in general, but I think it’s fine here. There’s also the native FileHandle class, which also comes with 56 bytes per instance.

And as far as FileHandles as a resource are concerned, we maybe create thousands of them in a single process, but not millions.

Should the module name be guarded with a whitelist here?

I don't think it should since worker threads don't provide a sandbox but I just want to make sure that this isn't exposing a new attack surface somehow.

@benjamingr Hm yeah … this isn’t extending capabilities of the calling code in any way, but I agree that it might be good to be careful here. I’ll think about this a bit more.

@benjamingr I’ve updated this PR so that this function now verifies that the target constructor is a subclass of JSTransferable, and added tests that verify that fact + the fact that this will only load internal modules (which was already the case before).

I think that’s as “locked down” as this can get. If user code makes other internal classes inherit from JSTransferable on the receiving side, and override the kTransfer method on the sending side – which is totally doable in most situations – then that means that you can already make arbitrary changes to Node.js internals and won’t even need to do this.

The one thing I could see happen would be side effects from require()ing the internal module itself, which the sending code can still trigger on the receiving side (and which would still typically result in an uncaught exception).

What do you think?

Better :)

The one thing I could see happen would be side effects from require()ing the internal module itself, which the sending code can still trigger on the receiving side (and which would still typically result in an uncaught exception).

Fwiw that's the part that initially bugged me, but like we both said it doesn't increase the attack surface at all from what I can tell.

In the future, we might want to consider marking transferable objects with a special static read-only symbol property so we don't actually have to maintain this list. For instance, something like:

const { transferableSymbol } = require('worker_threads');

MessagePort[transferableSymbol]         // True!
FileHandle[transferableSymbol]          // True!
MyOtherHandleThing[transferableSymbol   // False!

Then we wouldn't necessarily have to maintain a list of such objects in the docs, we would just say that value may be any object where [transferableSymbol] === true

@jasnell I think that’s a good idea, but I do think it’s important to list the built-in types that are transferable here.

Agreed, but I think we could put those in one place rather than having them listed in a couple different places (they're currently listed in the value and the transferList, etc)

So the actual changes LGTM (ran fine when I checked them, the feature itself is pretty cool). I'm not sure that the cost of an additional 32 bytes per handle is worth it - but that's something we can probably fix later.

@benjamingr @tniessen Fwiw, I’ve pushed one additional commit here – it really bugged me that exceptions from deserialization would always end up as uncaught exceptions, and that we had no way of informing users about errors in the case of e.g. a context mismatch. I’ve made the MessagePort emit messagerror events in that case (That’s the event name used by the Web MessagePorts as well). In particular, this means that the sending side can’t really trigger uncaught exceptions on the receiving side in most cases.

Could you take a look at that as well?

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

Would it make sense to emit error or uncaughtException if no one is listening to messageerror?

(or a warning)

@benjamingr I would prefer to drop the message in that case, rather than start emitting more events/raise it as an uncaught exception… I think if anything, a warning would be okay, but like you said below, this isn’t something that should really occur in normal flow (and I assume you meant to refer to that as not critical)?

So yeah, if you feel strongly about it, I’d add a warning, otherwise I’d probably say it’s not worth the extra complexity.

(Also, I don’t know if your LGTM counts as an approval of this PR?)

Could you take a look at that as well?

I think it makes sense although I think people shouldn't run into deserialization errors in a normal flow and it's probably critical.

Making my LGTM explicit

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

CI: https://ci.nodejs.org/job/node-test-pull-request/31837/ (:yellow_heart:)