From cbc35c3a0d4d4a0da937471c3efbde515b9682fb Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 8 Apr 2020 17:14:00 +0200 Subject: [PATCH 01/11] src: introduce BaseObject base FunctionTemplate --- src/async_wrap.cc | 1 + src/base_object-inl.h | 1 + src/base_object.h | 3 +++ src/env.cc | 11 +++++++++++ src/env.h | 1 + src/module_wrap.cc | 1 + src/node_crypto.cc | 9 +++++++++ src/node_i18n.cc | 1 + src/node_perf.cc | 1 + src/node_process_methods.cc | 5 ++++- src/node_serdes.cc | 2 ++ src/node_trace_events.cc | 1 + src/node_util.cc | 1 + src/node_wasi.cc | 1 + 14 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 13c2c3ca5a5ec3..80342b78284892 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -557,6 +557,7 @@ Local AsyncWrap::GetConstructorTemplate(Environment* env) { if (tmpl.IsEmpty()) { tmpl = env->NewFunctionTemplate(nullptr); tmpl->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "AsyncWrap")); + tmpl->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(tmpl, "getAsyncId", AsyncWrap::GetAsyncId); env->SetProtoMethod(tmpl, "asyncReset", AsyncWrap::AsyncReset); env->SetProtoMethod(tmpl, "getProviderType", AsyncWrap::GetProviderType); diff --git a/src/base_object-inl.h b/src/base_object-inl.h index f6e980e8749f61..b3fb1562707b64 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -156,6 +156,7 @@ BaseObject::MakeLazilyInitializedJSTemplate(Environment* env) { }; v8::Local t = env->NewFunctionTemplate(constructor); + t->Inherit(BaseObject::GetConstructorTemplate(env)); t->InstanceTemplate()->SetInternalFieldCount( BaseObject::kInternalFieldCount); return t; diff --git a/src/base_object.h b/src/base_object.h index 674eae1f0f7766..e02fd8d209f30a 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -98,6 +98,9 @@ class BaseObject : public MemoryRetainer { // a BaseObjectPtr to this object. inline void Detach(); + static v8::Local GetConstructorTemplate( + Environment* env); + protected: virtual inline void OnGCCollect(); diff --git a/src/env.cc b/src/env.cc index 677faf093a8c1f..4030df9ec90f57 100644 --- a/src/env.cc +++ b/src/env.cc @@ -269,6 +269,7 @@ void Environment::CreateProperties() { Local templ = FunctionTemplate::New(isolate()); templ->InstanceTemplate()->SetInternalFieldCount( BaseObject::kInternalFieldCount); + templ->Inherit(BaseObject::GetConstructorTemplate(this)); set_binding_data_ctor_template(templ); } @@ -1112,4 +1113,14 @@ bool BaseObject::IsRootNode() const { return !persistent_handle_.IsWeak(); } +Local BaseObject::GetConstructorTemplate(Environment* env) { + Local tmpl = env->base_object_ctor_template(); + if (tmpl.IsEmpty()) { + tmpl = env->NewFunctionTemplate(nullptr); + tmpl->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "BaseObject")); + env->set_base_object_ctor_template(tmpl); + } + return tmpl; +} + } // namespace node diff --git a/src/env.h b/src/env.h index 4b8670de46a8c2..a8c797635cbb8d 100644 --- a/src/env.h +++ b/src/env.h @@ -395,6 +395,7 @@ constexpr size_t kFsStatsBufferLength = #define ENVIRONMENT_STRONG_PERSISTENT_TEMPLATES(V) \ V(async_wrap_ctor_template, v8::FunctionTemplate) \ V(async_wrap_object_ctor_template, v8::FunctionTemplate) \ + V(base_object_ctor_template, v8::FunctionTemplate) \ V(binding_data_ctor_template, v8::FunctionTemplate) \ V(compiled_fn_entry_template, v8::ObjectTemplate) \ V(dir_instance_template, v8::ObjectTemplate) \ diff --git a/src/module_wrap.cc b/src/module_wrap.cc index ab8dbc9cbf7fa5..05f8b8ad762135 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -699,6 +699,7 @@ void ModuleWrap::Initialize(Local target, tpl->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "ModuleWrap")); tpl->InstanceTemplate()->SetInternalFieldCount( ModuleWrap::kInternalFieldCount); + tpl->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(tpl, "link", Link); env->SetProtoMethod(tpl, "instantiate", Instantiate); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 360732e37fd514..ca394f2e24e4bb 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -455,6 +455,7 @@ void SecureContext::Initialize(Environment* env, Local target) { Local t = env->NewFunctionTemplate(New); t->InstanceTemplate()->SetInternalFieldCount( SecureContext::kInternalFieldCount); + t->Inherit(BaseObject::GetConstructorTemplate(env)); Local secureContextString = FIXED_ONE_BYTE_STRING(env->isolate(), "SecureContext"); t->SetClassName(secureContextString); @@ -3246,6 +3247,7 @@ Local KeyObject::Initialize(Environment* env, Local target) { Local t = env->NewFunctionTemplate(New); t->InstanceTemplate()->SetInternalFieldCount( KeyObject::kInternalFieldCount); + t->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(t, "init", Init); env->SetProtoMethodNoSideEffect(t, "getSymmetricKeySize", @@ -3480,6 +3482,7 @@ void CipherBase::Initialize(Environment* env, Local target) { t->InstanceTemplate()->SetInternalFieldCount( CipherBase::kInternalFieldCount); + t->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(t, "init", Init); env->SetProtoMethod(t, "initiv", InitIv); @@ -4095,6 +4098,7 @@ void Hmac::Initialize(Environment* env, Local target) { t->InstanceTemplate()->SetInternalFieldCount( Hmac::kInternalFieldCount); + t->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(t, "init", HmacInit); env->SetProtoMethod(t, "update", HmacUpdate); @@ -4207,6 +4211,7 @@ void Hash::Initialize(Environment* env, Local target) { t->InstanceTemplate()->SetInternalFieldCount( Hash::kInternalFieldCount); + t->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(t, "update", HashUpdate); env->SetProtoMethod(t, "digest", HashDigest); @@ -4463,6 +4468,7 @@ void Sign::Initialize(Environment* env, Local target) { t->InstanceTemplate()->SetInternalFieldCount( SignBase::kInternalFieldCount); + t->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(t, "init", SignInit); env->SetProtoMethod(t, "update", SignUpdate); @@ -4785,6 +4791,7 @@ void Verify::Initialize(Environment* env, Local target) { t->InstanceTemplate()->SetInternalFieldCount( SignBase::kInternalFieldCount); + t->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(t, "init", VerifyInit); env->SetProtoMethod(t, "update", VerifyUpdate); @@ -5095,6 +5102,7 @@ void DiffieHellman::Initialize(Environment* env, Local target) { t->InstanceTemplate()->SetInternalFieldCount( DiffieHellman::kInternalFieldCount); + t->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(t, "generateKeys", GenerateKeys); env->SetProtoMethod(t, "computeSecret", ComputeSecret); @@ -5454,6 +5462,7 @@ void ECDH::Initialize(Environment* env, Local target) { HandleScope scope(env->isolate()); Local t = env->NewFunctionTemplate(New); + t->Inherit(BaseObject::GetConstructorTemplate(env)); t->InstanceTemplate()->SetInternalFieldCount(ECDH::kInternalFieldCount); diff --git a/src/node_i18n.cc b/src/node_i18n.cc index 169374aa5de441..5382e469a4087c 100644 --- a/src/node_i18n.cc +++ b/src/node_i18n.cc @@ -811,6 +811,7 @@ void Initialize(Local target, // ConverterObject { Local t = FunctionTemplate::New(env->isolate()); + t->Inherit(BaseObject::GetConstructorTemplate(env)); t->InstanceTemplate()->SetInternalFieldCount( ConverterObject::kInternalFieldCount); Local converter_string = diff --git a/src/node_perf.cc b/src/node_perf.cc index 4ed1c956c9e52e..916a9974d5d6f9 100644 --- a/src/node_perf.cc +++ b/src/node_perf.cc @@ -644,6 +644,7 @@ void Initialize(Local target, eldh->SetClassName(eldh_classname); eldh->InstanceTemplate()->SetInternalFieldCount( ELDHistogram::kInternalFieldCount); + eldh->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(eldh, "exceeds", ELDHistogramExceeds); env->SetProtoMethod(eldh, "min", ELDHistogramMin); env->SetProtoMethod(eldh, "max", ELDHistogramMax); diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc index d580f74478cbf7..800ce0313647d4 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -426,7 +426,10 @@ static void ReallyExit(const FunctionCallbackInfo& args) { class FastHrtime : public BaseObject { public: static Local New(Environment* env) { - Local otmpl = v8::ObjectTemplate::New(env->isolate()); + Local ctor = + v8::FunctionTemplate::New(env->isolate()); + ctor->Inherit(BaseObject::GetConstructorTemplate(env)); + Local otmpl = ctor->InstanceTemplate(); otmpl->SetInternalFieldCount(FastHrtime::kInternalFieldCount); auto create_func = [env](auto fast_func, auto slow_func) { diff --git a/src/node_serdes.cc b/src/node_serdes.cc index c7877215911f8e..28844f1858ff3d 100644 --- a/src/node_serdes.cc +++ b/src/node_serdes.cc @@ -451,6 +451,7 @@ void Initialize(Local target, ser->InstanceTemplate()->SetInternalFieldCount( SerializerContext::kInternalFieldCount); + ser->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(ser, "writeHeader", SerializerContext::WriteHeader); env->SetProtoMethod(ser, "writeValue", SerializerContext::WriteValue); @@ -478,6 +479,7 @@ void Initialize(Local target, des->InstanceTemplate()->SetInternalFieldCount( DeserializerContext::kInternalFieldCount); + des->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(des, "readHeader", DeserializerContext::ReadHeader); env->SetProtoMethod(des, "readValue", DeserializerContext::ReadValue); diff --git a/src/node_trace_events.cc b/src/node_trace_events.cc index 9adee9e458ccc0..58813a9083a560 100644 --- a/src/node_trace_events.cc +++ b/src/node_trace_events.cc @@ -131,6 +131,7 @@ void NodeCategorySet::Initialize(Local target, env->NewFunctionTemplate(NodeCategorySet::New); category_set->InstanceTemplate()->SetInternalFieldCount( NodeCategorySet::kInternalFieldCount); + category_set->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(category_set, "enable", NodeCategorySet::Enable); env->SetProtoMethod(category_set, "disable", NodeCategorySet::Disable); diff --git a/src/node_util.cc b/src/node_util.cc index ec3f8e1fe7deaf..199b04c95b44d4 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -325,6 +325,7 @@ void Initialize(Local target, weak_ref->InstanceTemplate()->SetInternalFieldCount( WeakReference::kInternalFieldCount); weak_ref->SetClassName(weak_ref_string); + weak_ref->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(weak_ref, "get", WeakReference::Get); env->SetProtoMethod(weak_ref, "incRef", WeakReference::IncRef); env->SetProtoMethod(weak_ref, "decRef", WeakReference::DecRef); diff --git a/src/node_wasi.cc b/src/node_wasi.cc index 4a78f99365bbe6..48ef82bd088d6a 100644 --- a/src/node_wasi.cc +++ b/src/node_wasi.cc @@ -1679,6 +1679,7 @@ static void Initialize(Local target, auto wasi_wrap_string = FIXED_ONE_BYTE_STRING(env->isolate(), "WASI"); tmpl->InstanceTemplate()->SetInternalFieldCount(WASI::kInternalFieldCount); tmpl->SetClassName(wasi_wrap_string); + tmpl->Inherit(BaseObject::GetConstructorTemplate(env)); env->SetProtoMethod(tmpl, "args_get", WASI::ArgsGet); env->SetProtoMethod(tmpl, "args_sizes_get", WASI::ArgsSizesGet); From 915b90e486495a6581ea37c3e2ab68f715832efe Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 9 Apr 2020 18:45:15 +0200 Subject: [PATCH 02/11] worker: allow transferring/cloning generic BaseObjects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- doc/api/errors.md | 5 +- src/base_object.h | 38 +++++++++- src/node_messaging.cc | 164 ++++++++++++++++++++++++++++++------------ src/node_messaging.h | 37 ++++++++-- src/util.h | 6 ++ 5 files changed, 194 insertions(+), 56 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index a98571edd61942..43c0b1dedaf5b8 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1589,8 +1589,9 @@ is thrown if a required option is missing. ### `ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST` -A `MessagePort` was found in the object passed to a `postMessage()` call, -but not provided in the `transferList` for that call. +An object that needs to be explicitly listed in the `transferList` argument +was found in the object passed to a `postMessage()` call, but not provided in +the `transferList` for that call. Usually, this is a `MessagePort`. ### `ERR_MISSING_PASSPHRASE` diff --git a/src/base_object.h b/src/base_object.h index e02fd8d209f30a..39450540323a6c 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -34,6 +34,10 @@ class Environment; template class BaseObjectPtrImpl; +namespace worker { +class TransferData; +} + class BaseObject : public MemoryRetainer { public: enum InternalFields { kSlot, kInternalFieldCount }; @@ -101,7 +105,39 @@ class BaseObject : public MemoryRetainer { static v8::Local GetConstructorTemplate( Environment* env); - protected: + // 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 + // the current object: + // - kUntransferable: + // No transfer is possible, either because this type of BaseObject does + // not know how to be transfered, or because it is not in a state in + // which it is possible to do so (e.g. because it has already been + // transfered). + // - kTransferable: + // This object can be transfered in a destructive fashion, i.e. will be + // rendered unusable on the sending side of the channel in the process + // of being transfered. (In C++ this would be referred to as movable but + // not copyable.) Objects of this type need to be listed in the + // `transferList` argument of the relevant postMessage() call in order to + // make sure that they are not accidentally destroyed on the sending side. + // TransferForMessaging() will be called to get a representation of the + // object that is used for subsequent deserialization. + // - kCloneable: + // This object can be cloned without being modified. + // CloneForMessaging() will be called to get a representation of the + // object that is used for subsequent deserialization, unless the + // object is listed in transferList, in which case TransferForMessaging() + // is attempted first. + enum class TransferMode { + kUntransferable, + kTransferable, + kCloneable + }; + virtual TransferMode GetTransferMode() const; + virtual std::unique_ptr TransferForMessaging(); + virtual std::unique_ptr CloneForMessaging() const; + virtual inline void OnGCCollect(); private: diff --git a/src/node_messaging.cc b/src/node_messaging.cc index ffe29a5dea8c8a..f67ec25e8a7d47 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -37,6 +37,19 @@ using v8::ValueSerializer; using v8::WasmModuleObject; namespace node { + +BaseObject::TransferMode BaseObject::GetTransferMode() const { + return BaseObject::TransferMode::kUntransferable; +} + +std::unique_ptr BaseObject::TransferForMessaging() { + return CloneForMessaging(); +} + +std::unique_ptr BaseObject::CloneForMessaging() const { + return {}; +} + namespace worker { Message::Message(MallocedBuffer&& buffer) @@ -55,21 +68,20 @@ class DeserializerDelegate : public ValueDeserializer::Delegate { DeserializerDelegate( Message* m, Environment* env, - const std::vector& message_ports, + const std::vector& host_objects, const std::vector>& shared_array_buffers, const std::vector& wasm_modules) - : message_ports_(message_ports), + : host_objects_(host_objects), shared_array_buffers_(shared_array_buffers), wasm_modules_(wasm_modules) {} MaybeLocal ReadHostObject(Isolate* isolate) override { - // Currently, only MessagePort hosts objects are supported, so identifying - // by the index in the message's MessagePort array is sufficient. + // Identifying the index in the message's BaseObject array is sufficient. uint32_t id; if (!deserializer->ReadUint32(&id)) return MaybeLocal(); - CHECK_LE(id, message_ports_.size()); - return message_ports_[id]->object(isolate); + CHECK_LE(id, host_objects_.size()); + return host_objects_[id]->object(isolate); } MaybeLocal GetSharedArrayBufferFromId( @@ -88,7 +100,7 @@ class DeserializerDelegate : public ValueDeserializer::Delegate { ValueDeserializer* deserializer = nullptr; private: - const std::vector& message_ports_; + const std::vector& host_objects_; const std::vector>& shared_array_buffers_; const std::vector& wasm_modules_; }; @@ -102,22 +114,25 @@ MaybeLocal Message::Deserialize(Environment* env, EscapableHandleScope handle_scope(env->isolate()); Context::Scope context_scope(context); - // Create all necessary MessagePort handles. - std::vector ports(message_ports_.size()); - for (uint32_t i = 0; i < message_ports_.size(); ++i) { - ports[i] = MessagePort::New(env, - context, - std::move(message_ports_[i])); - if (ports[i] == nullptr) { - for (MessagePort* port : ports) { - // This will eventually release the MessagePort object itself. - if (port != nullptr) - port->Close(); + // Create all necessary objects for transferables, e.g. MessagePort handles. + std::vector host_objects(transferables_.size()); + for (uint32_t i = 0; i < transferables_.size(); ++i) { + TransferData* data = transferables_[i].get(); + host_objects[i] = data->Deserialize( + env, context, std::move(transferables_[i])); + if (host_objects[i] == nullptr) { + for (BaseObject* object : host_objects) { + CHECK_NOT_NULL(object); + + // Since creating one of the objects failed, we don't want to have the + // other objects lying around in memory. We act as if the object has + // been garbage-collected. + object->OnGCCollect(); } return MaybeLocal(); } } - message_ports_.clear(); + transferables_.clear(); std::vector> shared_array_buffers; // Attach all transferred SharedArrayBuffers to their new Isolate. @@ -130,7 +145,7 @@ MaybeLocal Message::Deserialize(Environment* env, shared_array_buffers_.clear(); DeserializerDelegate delegate( - this, env, ports, shared_array_buffers, wasm_modules_); + this, env, host_objects, shared_array_buffers, wasm_modules_); ValueDeserializer deserializer( env->isolate(), reinterpret_cast(main_message_buf_.data), @@ -157,8 +172,8 @@ void Message::AddSharedArrayBuffer( shared_array_buffers_.emplace_back(std::move(backing_store)); } -void Message::AddMessagePort(std::unique_ptr&& data) { - message_ports_.emplace_back(std::move(data)); +void Message::AddTransferable(std::unique_ptr&& data) { + transferables_.emplace_back(std::move(data)); } uint32_t Message::AddWASMModule(CompiledWasmModule&& mod) { @@ -224,8 +239,8 @@ class SerializerDelegate : public ValueSerializer::Delegate { } Maybe WriteHostObject(Isolate* isolate, Local object) override { - if (env_->message_port_constructor_template()->HasInstance(object)) { - return WriteMessagePort(Unwrap(object)); + if (env_->base_object_ctor_template()->HasInstance(object)) { + return WriteHostObject(Unwrap(object)); } ThrowDataCloneError(env_->clone_unsupported_type_str()); @@ -257,32 +272,61 @@ class SerializerDelegate : public ValueSerializer::Delegate { void Finish() { // Only close the MessagePort handles and actually transfer them // once we know that serialization succeeded. - for (MessagePort* port : ports_) { - port->Close(); - msg_->AddMessagePort(port->Detach()); + for (uint32_t i = 0; i < host_objects_.size(); i++) { + BaseObject* host_object = host_objects_[i]; + std::unique_ptr data; + if (i < first_cloned_object_index_) + data = host_object->TransferForMessaging(); + if (!data) + data = host_object->CloneForMessaging(); + CHECK(data); + msg_->AddTransferable(std::move(data)); } } + inline void AddHostObject(BaseObject* host_object) { + // Make sure we have not started serializing the value itself yet. + CHECK_EQ(first_cloned_object_index_, SIZE_MAX); + host_objects_.push_back(host_object); + } + ValueSerializer* serializer = nullptr; private: - Maybe WriteMessagePort(MessagePort* port) { - for (uint32_t i = 0; i < ports_.size(); i++) { - if (ports_[i] == port) { + Maybe WriteHostObject(BaseObject* host_object) { + for (uint32_t i = 0; i < host_objects_.size(); i++) { + if (host_objects_[i] == host_object) { serializer->WriteUint32(i); return Just(true); } } - THROW_ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST(env_); - return Nothing(); + BaseObject::TransferMode mode = host_object->GetTransferMode(); + if (mode == BaseObject::TransferMode::kUntransferable) { + ThrowDataCloneError(env_->clone_unsupported_type_str()); + return Nothing(); + } else if (mode == BaseObject::TransferMode::kTransferable) { + // TODO(addaleax): This message code is too specific. Fix that in a + // semver-major follow-up. + THROW_ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST(env_); + return Nothing(); + } + + CHECK_EQ(mode, BaseObject::TransferMode::kCloneable); + uint32_t index = host_objects_.size(); + if (first_cloned_object_index_ == SIZE_MAX) + first_cloned_object_index_ = index; + serializer->WriteUint32(index); + host_objects_.push_back(host_object); + return Just(true); } Environment* env_; Local context_; Message* msg_; std::vector> seen_shared_array_buffers_; - std::vector ports_; + std::vector host_objects_; + size_t first_cloned_object_index_ = SIZE_MAX; friend class worker::Message; }; @@ -344,8 +388,7 @@ Maybe Message::Serialize(Environment* env, array_buffers.push_back(ab); serializer.TransferArrayBuffer(id, ab); continue; - } else if (env->message_port_constructor_template() - ->HasInstance(entry)) { + } else if (env->base_object_ctor_template()->HasInstance(entry)) { // Check if the source MessagePort is being transferred. if (!source_port.IsEmpty() && entry == source_port) { ThrowDataCloneException( @@ -354,8 +397,10 @@ Maybe Message::Serialize(Environment* env, "Transfer list contains source port")); return Nothing(); } - MessagePort* port = Unwrap(entry.As()); - if (port == nullptr || port->IsDetached()) { + BaseObject* host_object = Unwrap(entry.As()); + if (env->message_port_constructor_template()->HasInstance(entry) && + (host_object == nullptr || + static_cast(host_object)->IsDetached())) { ThrowDataCloneException( context, FIXED_ONE_BYTE_STRING( @@ -363,17 +408,23 @@ Maybe Message::Serialize(Environment* env, "MessagePort in transfer list is already detached")); return Nothing(); } - if (std::find(delegate.ports_.begin(), delegate.ports_.end(), port) != - delegate.ports_.end()) { + if (std::find(delegate.host_objects_.begin(), + delegate.host_objects_.end(), + host_object) != delegate.host_objects_.end()) { ThrowDataCloneException( context, - FIXED_ONE_BYTE_STRING( - env->isolate(), - "Transfer list contains duplicate MessagePort")); + String::Concat(env->isolate(), + FIXED_ONE_BYTE_STRING( + env->isolate(), + "Transfer list contains duplicate "), + entry.As()->GetConstructorName())); return Nothing(); } - delegate.ports_.push_back(port); - continue; + if (host_object != nullptr && host_object->GetTransferMode() != + BaseObject::TransferMode::kUntransferable) { + delegate.AddHostObject(host_object); + continue; + } } THROW_ERR_INVALID_TRANSFER_OBJECT(env); @@ -406,7 +457,7 @@ Maybe Message::Serialize(Environment* env, void Message::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("array_buffers_", array_buffers_); tracker->TrackField("shared_array_buffers", shared_array_buffers_); - tracker->TrackField("message_ports", message_ports_); + tracker->TrackField("transferables", transferables_); } MessagePortData::MessagePortData(MessagePort* owner) : owner_(owner) { } @@ -672,6 +723,25 @@ std::unique_ptr MessagePort::Detach() { return std::move(data_); } +BaseObject::TransferMode MessagePort::GetTransferMode() const { + if (IsDetached()) + return BaseObject::TransferMode::kUntransferable; + return BaseObject::TransferMode::kTransferable; +} + +std::unique_ptr MessagePort::TransferForMessaging() { + Close(); + return Detach(); +} + +BaseObject* MessagePortData::Deserialize( + Environment* env, + Local context, + std::unique_ptr self) { + return MessagePort::New( + env, context, + static_unique_pointer_cast(std::move(self))); +} Maybe MessagePort::PostMessage(Environment* env, Local message_v, @@ -699,8 +769,8 @@ Maybe MessagePort::PostMessage(Environment* env, // Check if the target port is posted to itself. if (data_->sibling_ != nullptr) { - for (const auto& port_data : msg.message_ports()) { - if (data_->sibling_ == port_data.get()) { + for (const auto& transferable : msg.transferables()) { + if (data_->sibling_ == transferable.get()) { doomed = true; ProcessEmitWarning(env, "The target port was posted to itself, and " "the communication channel was lost"); diff --git a/src/node_messaging.h b/src/node_messaging.h index d687e7549d51e9..6a4bddd8106953 100644 --- a/src/node_messaging.h +++ b/src/node_messaging.h @@ -15,6 +15,23 @@ class MessagePort; typedef MaybeStackBuffer, 8> TransferList; +// Used to represent the in-flight structure of an object that is being +// transfered or cloned using postMessage(). +class TransferData : public MemoryRetainer { + public: + // Deserialize this object on the receiving end after a .postMessage() call. + // - `context` may not be the same as `env->context()`. This method should + // not produce JS objects coming from Contexts other than `context`. + // - `self` is a unique_ptr for the object that this is being called on. + // - The return value is treated like a `Maybe`, i.e. if `nullptr` is + // returned, any further deserialization of the message is stopped and + // control is returned to the event loop or JS as soon as possible. + virtual BaseObject* Deserialize( + Environment* env, + v8::Local context, + std::unique_ptr self) = 0; +}; + // Represents a single communication message. class Message : public MemoryRetainer { public: @@ -54,16 +71,17 @@ class Message : public MemoryRetainer { void AddSharedArrayBuffer(std::shared_ptr backing_store); // Internal method of Message that is called once serialization finishes // and that transfers ownership of `data` to this message. - void AddMessagePort(std::unique_ptr&& data); + void AddTransferable(std::unique_ptr&& data); // Internal method of Message that is called when a new WebAssembly.Module // object is encountered in the incoming value's structure. uint32_t AddWASMModule(v8::CompiledWasmModule&& mod); - // The MessagePorts that will be transferred, as recorded by Serialize(). + // The host objects that will be transferred, as recorded by Serialize() + // (e.g. MessagePorts). // Used for warning user about posting the target MessagePort to itself, // which will as a side effect destroy the communication channel. - const std::vector>& message_ports() const { - return message_ports_; + const std::vector>& transferables() const { + return transferables_; } void MemoryInfo(MemoryTracker* tracker) const override; @@ -75,7 +93,7 @@ class Message : public MemoryRetainer { MallocedBuffer main_message_buf_; std::vector> array_buffers_; std::vector> shared_array_buffers_; - std::vector> message_ports_; + std::vector> transferables_; std::vector wasm_modules_; friend class MessagePort; @@ -83,7 +101,7 @@ class Message : public MemoryRetainer { // This contains all data for a `MessagePort` instance that is not tied to // a specific Environment/Isolate/event loop, for easier transfer between those. -class MessagePortData : public MemoryRetainer { +class MessagePortData : public TransferData { public: explicit MessagePortData(MessagePort* owner); ~MessagePortData() override; @@ -108,6 +126,10 @@ class MessagePortData : public MemoryRetainer { void Disentangle(); void MemoryInfo(MemoryTracker* tracker) const override; + BaseObject* Deserialize( + Environment* env, + v8::Local context, + std::unique_ptr self) override; SET_MEMORY_INFO_NAME(MessagePortData) SET_SELF_SIZE(MessagePortData) @@ -195,6 +217,9 @@ class MessagePort : public HandleWrap { // NULL pointer to the C++ MessagePort object is also detached. inline bool IsDetached() const; + TransferMode GetTransferMode() const override; + std::unique_ptr TransferForMessaging() override; + void MemoryInfo(MemoryTracker* tracker) const override; SET_MEMORY_INFO_NAME(MessagePort) SET_SELF_SIZE(MessagePort) diff --git a/src/util.h b/src/util.h index 62229e3c448637..8bdeb35184e99d 100644 --- a/src/util.h +++ b/src/util.h @@ -784,6 +784,12 @@ class FastStringKey { size_t cached_hash_; }; +// Like std::static_pointer_cast but for unique_ptr with the default deleter. +template +std::unique_ptr static_unique_pointer_cast(std::unique_ptr&& ptr) { + return std::unique_ptr(static_cast(ptr.release())); +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS From 5f25ff9653fcb8b80cb1a200bff0d962fa913b6f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 6 Jun 2020 14:02:59 +0200 Subject: [PATCH 03/11] fixup! worker: allow transferring/cloning generic BaseObjects --- src/node_messaging.cc | 20 ++++++++++---------- src/node_messaging.h | 4 ++-- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/node_messaging.cc b/src/node_messaging.cc index f67ec25e8a7d47..ae7a0fc1750d04 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -68,7 +68,7 @@ class DeserializerDelegate : public ValueDeserializer::Delegate { DeserializerDelegate( Message* m, Environment* env, - const std::vector& host_objects, + const std::vector>& host_objects, const std::vector>& shared_array_buffers, const std::vector& wasm_modules) : host_objects_(host_objects), @@ -100,7 +100,7 @@ class DeserializerDelegate : public ValueDeserializer::Delegate { ValueDeserializer* deserializer = nullptr; private: - const std::vector& host_objects_; + const std::vector>& host_objects_; const std::vector>& shared_array_buffers_; const std::vector& wasm_modules_; }; @@ -115,19 +115,19 @@ MaybeLocal Message::Deserialize(Environment* env, Context::Scope context_scope(context); // Create all necessary objects for transferables, e.g. MessagePort handles. - std::vector host_objects(transferables_.size()); + std::vector> host_objects(transferables_.size()); for (uint32_t i = 0; i < transferables_.size(); ++i) { TransferData* data = transferables_[i].get(); host_objects[i] = data->Deserialize( env, context, std::move(transferables_[i])); - if (host_objects[i] == nullptr) { - for (BaseObject* object : host_objects) { - CHECK_NOT_NULL(object); + if (!host_objects[i]) { + for (BaseObjectPtr object : host_objects) { + if (!object) continue; // Since creating one of the objects failed, we don't want to have the // other objects lying around in memory. We act as if the object has // been garbage-collected. - object->OnGCCollect(); + object->Detach(); } return MaybeLocal(); } @@ -734,13 +734,13 @@ std::unique_ptr MessagePort::TransferForMessaging() { return Detach(); } -BaseObject* MessagePortData::Deserialize( +BaseObjectPtr MessagePortData::Deserialize( Environment* env, Local context, std::unique_ptr self) { - return MessagePort::New( + return BaseObjectPtr { MessagePort::New( env, context, - static_unique_pointer_cast(std::move(self))); + static_unique_pointer_cast(std::move(self))) }; } Maybe MessagePort::PostMessage(Environment* env, diff --git a/src/node_messaging.h b/src/node_messaging.h index 6a4bddd8106953..649ee201045428 100644 --- a/src/node_messaging.h +++ b/src/node_messaging.h @@ -26,7 +26,7 @@ class TransferData : public MemoryRetainer { // - The return value is treated like a `Maybe`, i.e. if `nullptr` is // returned, any further deserialization of the message is stopped and // control is returned to the event loop or JS as soon as possible. - virtual BaseObject* Deserialize( + virtual BaseObjectPtr Deserialize( Environment* env, v8::Local context, std::unique_ptr self) = 0; @@ -126,7 +126,7 @@ class MessagePortData : public TransferData { void Disentangle(); void MemoryInfo(MemoryTracker* tracker) const override; - BaseObject* Deserialize( + BaseObjectPtr Deserialize( Environment* env, v8::Local context, std::unique_ptr self) override; From b1aad92c98d8c909c93971b47c2b12c32f2a0a6f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 6 Jun 2020 15:57:12 +0200 Subject: [PATCH 04/11] src: add equality operators for BaseObjectPtr --- src/base_object-inl.h | 14 ++++++++++++++ src/base_object.h | 5 +++++ 2 files changed, 19 insertions(+) diff --git a/src/base_object-inl.h b/src/base_object-inl.h index b3fb1562707b64..555063f4e23e67 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -338,6 +338,20 @@ BaseObjectPtrImpl::operator bool() const { return get() != nullptr; } +template +template +bool BaseObjectPtrImpl::operator ==( + const BaseObjectPtrImpl& other) const { + return get() == other.get(); +} + +template +template +bool BaseObjectPtrImpl::operator !=( + const BaseObjectPtrImpl& other) const { + return get() != other.get(); +} + template BaseObjectPtr MakeBaseObject(Args&&... args) { return BaseObjectPtr(new T(std::forward(args)...)); diff --git a/src/base_object.h b/src/base_object.h index 39450540323a6c..79ef76b236f549 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -236,6 +236,11 @@ class BaseObjectPtrImpl final { inline T* operator->() const; inline operator bool() const; + template + inline bool operator ==(const BaseObjectPtrImpl& other) const; + template + inline bool operator !=(const BaseObjectPtrImpl& other) const; + private: union { BaseObject* target; // Used for strong pointers. From 877d631c56e6b99d9af176a2287edab850ab6e55 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 6 Jun 2020 16:11:31 +0200 Subject: [PATCH 05/11] 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 `MessagePort`s. --- lib/internal/bootstrap/node.js | 1 + lib/internal/worker/js_transferable.js | 31 ++ node.gyp | 1 + src/base_object.h | 9 + src/env.h | 6 + src/node_errors.h | 3 +- src/node_messaging.cc | 309 ++++++++++++++++-- src/node_messaging.h | 52 +++ .../test-worker-workerdata-messageport.js | 3 +- 9 files changed, 382 insertions(+), 33 deletions(-) create mode 100644 lib/internal/worker/js_transferable.js diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 522faf779f446b..92dc2a6cc7d2d6 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -58,6 +58,7 @@ process._exiting = false; // process.config is serialized config.gypi process.config = JSONParse(internalBinding('native_module').config); +require('internal/worker/js_transferable').setup(); // Bootstrappers for all threads, including worker threads and main thread const perThreadSetup = require('internal/process/per_thread'); diff --git a/lib/internal/worker/js_transferable.js b/lib/internal/worker/js_transferable.js new file mode 100644 index 00000000000000..41f1e6ff72ae06 --- /dev/null +++ b/lib/internal/worker/js_transferable.js @@ -0,0 +1,31 @@ +'use strict'; +const { + messaging_deserialize_symbol, + messaging_transfer_symbol, + messaging_clone_symbol, + messaging_transfer_list_symbol +} = internalBinding('symbols'); +const { + JSTransferable, + setDeserializerCreateObjectFunction +} = internalBinding('messaging'); + +function setup() { + // Register the handler that will be used when deserializing JS-based objects + // from .postMessage() calls. The format of `deserializeInfo` is generally + // 'module:Constructor', e.g. 'internal/fs/promises:FileHandle'. + setDeserializerCreateObjectFunction((deserializeInfo) => { + const [ module, ctor ] = deserializeInfo.split(':'); + const Ctor = require(module)[ctor]; + return new Ctor(); + }); +} + +module.exports = { + setup, + JSTransferable, + kClone: messaging_clone_symbol, + kDeserialize: messaging_deserialize_symbol, + kTransfer: messaging_transfer_symbol, + kTransferList: messaging_transfer_list_symbol +}; diff --git a/node.gyp b/node.gyp index 1ebb8c2cc978e6..37af98ba424cf3 100644 --- a/node.gyp +++ b/node.gyp @@ -220,6 +220,7 @@ 'lib/internal/vm/module.js', 'lib/internal/worker.js', 'lib/internal/worker/io.js', + 'lib/internal/worker/js_transferable.js', 'lib/internal/watchdog.js', 'lib/internal/streams/lazy_transform.js', 'lib/internal/streams/async_iterator.js', diff --git a/src/base_object.h b/src/base_object.h index 79ef76b236f549..61e5d0cff97174 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -123,12 +123,17 @@ class BaseObject : public MemoryRetainer { // make sure that they are not accidentally destroyed on the sending side. // TransferForMessaging() will be called to get a representation of the // object that is used for subsequent deserialization. + // The NestedTransferables() method can be used to transfer other objects + // along with this one, if a situation requires it. // - kCloneable: // This object can be cloned without being modified. // CloneForMessaging() will be called to get a representation of the // object that is used for subsequent deserialization, unless the // object is listed in transferList, in which case TransferForMessaging() // is attempted first. + // After a successful clone, FinalizeTransferRead() is called on the receiving + // end, and can read deserialize JS data possibly serialized by a previous + // FinalizeTransferWrite() call. enum class TransferMode { kUntransferable, kTransferable, @@ -137,6 +142,10 @@ class BaseObject : public MemoryRetainer { virtual TransferMode GetTransferMode() const; virtual std::unique_ptr TransferForMessaging(); virtual std::unique_ptr CloneForMessaging() const; + virtual v8::Maybe>> + NestedTransferables() const; + virtual v8::Maybe FinalizeTransferRead( + v8::Local context, v8::ValueDeserializer* deserializer); virtual inline void OnGCCollect(); diff --git a/src/env.h b/src/env.h index a8c797635cbb8d..db7698b62bc07a 100644 --- a/src/env.h +++ b/src/env.h @@ -159,6 +159,10 @@ constexpr size_t kFsStatsBufferLength = V(async_id_symbol, "async_id_symbol") \ V(handle_onclose_symbol, "handle_onclose") \ V(no_message_symbol, "no_message_symbol") \ + V(messaging_deserialize_symbol, "messaging_deserialize_symbol") \ + V(messaging_transfer_symbol, "messaging_transfer_symbol") \ + V(messaging_clone_symbol, "messaging_clone_symbol") \ + V(messaging_transfer_list_symbol, "messaging_transfer_list_symbol") \ V(oninit_symbol, "oninit") \ V(owner_symbol, "owner_symbol") \ V(onpskexchange_symbol, "onpskexchange") \ @@ -201,6 +205,7 @@ constexpr size_t kFsStatsBufferLength = V(crypto_rsa_pss_string, "rsa-pss") \ V(cwd_string, "cwd") \ V(data_string, "data") \ + V(deserialize_info_string, "deserializeInfo") \ V(dest_string, "dest") \ V(destroyed_string, "destroyed") \ V(detached_string, "detached") \ @@ -453,6 +458,7 @@ constexpr size_t kFsStatsBufferLength = V(internal_binding_loader, v8::Function) \ V(immediate_callback_function, v8::Function) \ V(inspector_console_extension_installer, v8::Function) \ + V(messaging_deserialize_create_object, v8::Function) \ V(message_port, v8::Object) \ V(native_module_require, v8::Function) \ V(performance_entry_callback, v8::Function) \ diff --git a/src/node_errors.h b/src/node_errors.h index 01a8d8e75ac99a..d61c268dea9cf2 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -93,7 +93,8 @@ void OnFatalError(const char* location, const char* message); V(ERR_MEMORY_ALLOCATION_FAILED, "Failed to allocate memory") \ V(ERR_OSSL_EVP_INVALID_DIGEST, "Invalid digest used") \ V(ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST, \ - "MessagePort was found in message but not listed in transferList") \ + "Object that needs transfer was found in message but not listed " \ + "in transferList") \ V(ERR_MISSING_PLATFORM_FOR_WORKER, \ "The V8 platform used by this instance of Node does not support " \ "creating Workers") \ diff --git a/src/node_messaging.cc b/src/node_messaging.cc index ae7a0fc1750d04..8a7d6bd474cfa7 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -10,6 +10,7 @@ #include "util-inl.h" using node::contextify::ContextifyContext; +using node::errors::TryCatchScope; using v8::Array; using v8::ArrayBuffer; using v8::BackingStore; @@ -38,6 +39,8 @@ using v8::WasmModuleObject; namespace node { +using BaseObjectList = std::vector>; + BaseObject::TransferMode BaseObject::GetTransferMode() const { return BaseObject::TransferMode::kUntransferable; } @@ -50,8 +53,22 @@ std::unique_ptr BaseObject::CloneForMessaging() const { return {}; } +Maybe BaseObject::NestedTransferables() const { + return Just(BaseObjectList {}); +} + +Maybe BaseObject::FinalizeTransferRead( + Local context, ValueDeserializer* deserializer) { + return Just(true); +} + namespace worker { +Maybe TransferData::FinalizeTransferWrite( + Local context, ValueSerializer* serializer) { + return Just(true); +} + Message::Message(MallocedBuffer&& buffer) : main_message_buf_(std::move(buffer)) {} @@ -116,21 +133,22 @@ MaybeLocal Message::Deserialize(Environment* env, // Create all necessary objects for transferables, e.g. MessagePort handles. std::vector> host_objects(transferables_.size()); + auto cleanup = OnScopeLeave([&]() { + for (BaseObjectPtr object : host_objects) { + if (!object) continue; + + // If the function did not finish successfully, host_objects will contain + // a list of objects that will never be passed to JS. Therefore, we + // destroy them here. + object->Detach(); + } + }); + for (uint32_t i = 0; i < transferables_.size(); ++i) { TransferData* data = transferables_[i].get(); host_objects[i] = data->Deserialize( env, context, std::move(transferables_[i])); - if (!host_objects[i]) { - for (BaseObjectPtr object : host_objects) { - if (!object) continue; - - // Since creating one of the objects failed, we don't want to have the - // other objects lying around in memory. We act as if the object has - // been garbage-collected. - object->Detach(); - } - return MaybeLocal(); - } + if (!host_objects[i]) return {}; } transferables_.clear(); @@ -162,9 +180,18 @@ MaybeLocal Message::Deserialize(Environment* env, array_buffers_.clear(); if (deserializer.ReadHeader(context).IsNothing()) - return MaybeLocal(); - return handle_scope.Escape( - deserializer.ReadValue(context).FromMaybe(Local())); + return {}; + Local return_value; + if (!deserializer.ReadValue(context).ToLocal(&return_value)) + return {}; + + for (BaseObjectPtr base_object : host_objects) { + if (base_object->FinalizeTransferRead(context, &deserializer).IsNothing()) + return {}; + } + + host_objects.clear(); + return handle_scope.Escape(return_value); } void Message::AddSharedArrayBuffer( @@ -240,7 +267,8 @@ class SerializerDelegate : public ValueSerializer::Delegate { Maybe WriteHostObject(Isolate* isolate, Local object) override { if (env_->base_object_ctor_template()->HasInstance(object)) { - return WriteHostObject(Unwrap(object)); + return WriteHostObject( + BaseObjectPtr { Unwrap(object) }); } ThrowDataCloneError(env_->clone_unsupported_type_str()); @@ -269,31 +297,51 @@ class SerializerDelegate : public ValueSerializer::Delegate { return Just(msg_->AddWASMModule(module->GetCompiledModule())); } - void Finish() { - // Only close the MessagePort handles and actually transfer them - // once we know that serialization succeeded. + Maybe Finish(Local context) { for (uint32_t i = 0; i < host_objects_.size(); i++) { - BaseObject* host_object = host_objects_[i]; + BaseObjectPtr host_object = std::move(host_objects_[i]); std::unique_ptr data; if (i < first_cloned_object_index_) data = host_object->TransferForMessaging(); if (!data) data = host_object->CloneForMessaging(); - CHECK(data); + if (!data) return Nothing(); + if (data->FinalizeTransferWrite(context, serializer).IsNothing()) + return Nothing(); msg_->AddTransferable(std::move(data)); } + return Just(true); } - inline void AddHostObject(BaseObject* host_object) { + inline void AddHostObject(BaseObjectPtr host_object) { // Make sure we have not started serializing the value itself yet. CHECK_EQ(first_cloned_object_index_, SIZE_MAX); - host_objects_.push_back(host_object); + host_objects_.emplace_back(std::move(host_object)); + } + + // Some objects in the transfer list may register sub-objects that can be + // transferred. This could e.g. be a public JS wrapper object, such as a + // FileHandle, that is registering its C++ handle for transfer. + inline Maybe AddNestedHostObjects() { + for (size_t i = 0; i < host_objects_.size(); i++) { + std::vector> nested_transferables; + if (!host_objects_[i]->NestedTransferables().To(&nested_transferables)) + return Nothing(); + for (auto nested_transferable : nested_transferables) { + if (std::find(host_objects_.begin(), + host_objects_.end(), + nested_transferable) == host_objects_.end()) { + AddHostObject(nested_transferable); + } + } + } + return Just(true); } ValueSerializer* serializer = nullptr; private: - Maybe WriteHostObject(BaseObject* host_object) { + Maybe WriteHostObject(BaseObjectPtr host_object) { for (uint32_t i = 0; i < host_objects_.size(); i++) { if (host_objects_[i] == host_object) { serializer->WriteUint32(i); @@ -325,7 +373,7 @@ class SerializerDelegate : public ValueSerializer::Delegate { Local context_; Message* msg_; std::vector> seen_shared_array_buffers_; - std::vector host_objects_; + std::vector> host_objects_; size_t first_cloned_object_index_ = SIZE_MAX; friend class worker::Message; @@ -397,10 +445,11 @@ Maybe Message::Serialize(Environment* env, "Transfer list contains source port")); return Nothing(); } - BaseObject* host_object = Unwrap(entry.As()); + BaseObjectPtr host_object { + Unwrap(entry.As()) }; if (env->message_port_constructor_template()->HasInstance(entry) && - (host_object == nullptr || - static_cast(host_object)->IsDetached())) { + (!host_object || + static_cast(host_object.get())->IsDetached())) { ThrowDataCloneException( context, FIXED_ONE_BYTE_STRING( @@ -420,7 +469,7 @@ Maybe Message::Serialize(Environment* env, entry.As()->GetConstructorName())); return Nothing(); } - if (host_object != nullptr && host_object->GetTransferMode() != + if (host_object && host_object->GetTransferMode() != BaseObject::TransferMode::kUntransferable) { delegate.AddHostObject(host_object); continue; @@ -430,6 +479,8 @@ Maybe Message::Serialize(Environment* env, THROW_ERR_INVALID_TRANSFER_OBJECT(env); return Nothing(); } + if (delegate.AddNestedHostObjects().IsNothing()) + return Nothing(); serializer.WriteHeader(); if (serializer.WriteValue(context, input).IsNothing()) { @@ -444,7 +495,8 @@ Maybe Message::Serialize(Environment* env, array_buffers_.emplace_back(std::move(backing_store)); } - delegate.Finish(); + if (delegate.Finish(context).IsNothing()) + return Nothing(); // The serializer gave us a buffer allocated using `malloc()`. std::pair data = serializer.Release(); @@ -687,9 +739,10 @@ void MessagePort::OnMessage() { HandleScope handle_scope(env()->isolate()); Context::Scope context_scope(context); + Local emit_message = PersistentToLocal::Strong(emit_message_fn_); Local payload; - if (!ReceiveMessage(context, true).ToLocal(&payload)) break; + if (!ReceiveMessage(context, true).ToLocal(&payload)) goto reschedule; if (payload == env()->no_message_symbol()) break; if (!env()->can_call_into_js()) { @@ -698,8 +751,8 @@ void MessagePort::OnMessage() { continue; } - Local emit_message = PersistentToLocal::Strong(emit_message_fn_); if (MakeCallback(emit_message, 1, &payload).IsEmpty()) { + reschedule: // Re-schedule OnMessage() execution in case of failure. if (data_) TriggerAsync(); @@ -1017,8 +1070,187 @@ Local GetMessagePortConstructorTemplate(Environment* env) { return GetMessagePortConstructorTemplate(env); } +JSTransferable::JSTransferable(Environment* env, Local obj) + : BaseObject(env, obj) { + MakeWeak(); +} + +void JSTransferable::New(const FunctionCallbackInfo& args) { + CHECK(args.IsConstructCall()); + new JSTransferable(Environment::GetCurrent(args), args.This()); +} + +JSTransferable::TransferMode JSTransferable::GetTransferMode() const { + // Implement `kClone in this ? kCloneable : kTransferable`. + HandleScope handle_scope(env()->isolate()); + errors::TryCatchScope ignore_exceptions(env()); + + bool has_clone; + if (!object()->Has(env()->context(), + env()->messaging_clone_symbol()).To(&has_clone)) { + return TransferMode::kUntransferable; + } + + return has_clone ? TransferMode::kCloneable : TransferMode::kTransferable; +} + +std::unique_ptr JSTransferable::TransferForMessaging() { + return TransferOrClone(TransferMode::kTransferable); +} + +std::unique_ptr JSTransferable::CloneForMessaging() const { + return TransferOrClone(TransferMode::kCloneable); +} + +std::unique_ptr JSTransferable::TransferOrClone( + TransferMode mode) const { + // Call `this[symbol]()` where `symbol` is `kClone` or `kTransfer`, + // which should return an object with `data` and `deserializeInfo` properties; + // `data` is written to the serializer later, and `deserializeInfo` is stored + // on the `TransferData` instance as a string. + HandleScope handle_scope(env()->isolate()); + Local context = env()->isolate()->GetCurrentContext(); + Local method_name = mode == TransferMode::kCloneable ? + env()->messaging_clone_symbol() : env()->messaging_transfer_symbol(); + + Local method; + if (!object()->Get(context, method_name).ToLocal(&method)) { + return {}; + } + if (method->IsFunction()) { + Local result_v; + if (!method.As()->Call( + context, object(), 0, nullptr).ToLocal(&result_v)) { + return {}; + } + + if (result_v->IsObject()) { + Local result = result_v.As(); + Local data; + Local deserialize_info; + if (!result->Get(context, env()->data_string()).ToLocal(&data) || + !result->Get(context, env()->deserialize_info_string()) + .ToLocal(&deserialize_info)) { + return {}; + } + Utf8Value deserialize_info_str(env()->isolate(), deserialize_info); + if (*deserialize_info_str == nullptr) return {}; + return std::make_unique( + *deserialize_info_str, Global(env()->isolate(), data)); + } + } + + if (mode == TransferMode::kTransferable) + return TransferOrClone(TransferMode::kCloneable); + else + return {}; +} + +Maybe +JSTransferable::NestedTransferables() const { + // Call `this[kTransferList]()` and return the resulting list of BaseObjects. + HandleScope handle_scope(env()->isolate()); + Local context = env()->isolate()->GetCurrentContext(); + Local method_name = env()->messaging_transfer_list_symbol(); + + Local method; + if (!object()->Get(context, method_name).ToLocal(&method)) { + return Nothing(); + } + if (!method->IsFunction()) return Just(BaseObjectList {}); + + Local list_v; + if (!method.As()->Call( + context, object(), 0, nullptr).ToLocal(&list_v)) { + return Nothing(); + } + if (!list_v->IsArray()) return Just(BaseObjectList {}); + Local list = list_v.As(); + + BaseObjectList ret; + for (size_t i = 0; i < list->Length(); i++) { + Local value; + if (!list->Get(context, i).ToLocal(&value)) + return Nothing(); + if (env()->base_object_ctor_template()->HasInstance(value)) + ret.emplace_back(Unwrap(value)); + } + return Just(ret); +} + +Maybe JSTransferable::FinalizeTransferRead( + Local context, ValueDeserializer* deserializer) { + // Call `this[kDeserialize](data)` where `data` comes from the return value + // of `this[kTransfer]()` or `this[kClone]()`. + HandleScope handle_scope(env()->isolate()); + Local data; + if (!deserializer->ReadValue(context).ToLocal(&data)) return Nothing(); + + Local method_name = env()->messaging_deserialize_symbol(); + Local method; + if (!object()->Get(context, method_name).ToLocal(&method)) { + return Nothing(); + } + if (!method->IsFunction()) return Just(true); + + if (method.As()->Call(context, object(), 1, &data).IsEmpty()) { + return Nothing(); + } + return Just(true); +} + +JSTransferable::Data::Data(std::string&& deserialize_info, + v8::Global&& data) + : deserialize_info_(std::move(deserialize_info)), + data_(std::move(data)) {} + +BaseObjectPtr JSTransferable::Data::Deserialize( + Environment* env, + Local context, + std::unique_ptr self) { + // Create the JS wrapper object that will later be filled with data passed to + // the `[kDeserialize]()` method on it. This split is necessary, because here + // we need to create an object with the right prototype and internal fields, + // but the actual JS data stored in the serialized data can only be read at + // the end of the stream, after the main message has been read. + + if (context != env->context()) { + // It would be nice to throw some kind of exception here, but how do we + // pass that to end users? For now, just drop the message silently. + return {}; + } + HandleScope handle_scope(env->isolate()); + Local info; + if (!ToV8Value(context, deserialize_info_).ToLocal(&info)) return {}; + + Local ret; + CHECK(!env->messaging_deserialize_create_object().IsEmpty()); + if (!env->messaging_deserialize_create_object()->Call( + context, Null(env->isolate()), 1, &info).ToLocal(&ret) || + !env->base_object_ctor_template()->HasInstance(ret)) { + return {}; + } + + return BaseObjectPtr { Unwrap(ret) }; +} + +Maybe JSTransferable::Data::FinalizeTransferWrite( + Local context, ValueSerializer* serializer) { + HandleScope handle_scope(context->GetIsolate()); + auto ret = serializer->WriteValue(context, PersistentToLocal::Strong(data_)); + data_.Reset(); + return ret; +} + namespace { +static void SetDeserializerCreateObjectFunction( + const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + CHECK(args[0]->IsFunction()); + env->set_messaging_deserialize_create_object(args[0].As()); +} + static void MessageChannel(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); if (!args.IsConstructCall()) { @@ -1061,6 +1293,19 @@ static void InitMessaging(Local target, templ->GetFunction(context).ToLocalChecked()).Check(); } + { + Local js_transferable_string = + FIXED_ONE_BYTE_STRING(env->isolate(), "JSTransferable"); + Local t = env->NewFunctionTemplate(JSTransferable::New); + t->Inherit(BaseObject::GetConstructorTemplate(env)); + t->SetClassName(js_transferable_string); + t->InstanceTemplate()->SetInternalFieldCount( + JSTransferable::kInternalFieldCount); + target->Set(context, + js_transferable_string, + t->GetFunction(context).ToLocalChecked()).Check(); + } + target->Set(context, env->message_port_constructor_string(), GetMessagePortConstructorTemplate(env) @@ -1073,6 +1318,8 @@ static void InitMessaging(Local target, env->SetMethod(target, "receiveMessageOnPort", MessagePort::ReceiveMessage); env->SetMethod(target, "moveMessagePortToContext", MessagePort::MoveToContext); + env->SetMethod(target, "setDeserializerCreateObjectFunction", + SetDeserializerCreateObjectFunction); { Local domexception = GetDOMException(context).ToLocalChecked(); diff --git a/src/node_messaging.h b/src/node_messaging.h index 649ee201045428..378468b6f44465 100644 --- a/src/node_messaging.h +++ b/src/node_messaging.h @@ -30,6 +30,12 @@ class TransferData : public MemoryRetainer { Environment* env, v8::Local context, std::unique_ptr self) = 0; + // FinalizeTransferWrite() is the counterpart to + // BaseObject::FinalizeTransferRead(). It is called right after the transfer + // data was created, and defaults to doing nothing. After this function, + // this object should not hold any more Isolate-specific data. + virtual v8::Maybe FinalizeTransferWrite( + v8::Local context, v8::ValueSerializer* serializer); }; // Represents a single communication message. @@ -239,6 +245,52 @@ class MessagePort : public HandleWrap { friend class MessagePortData; }; +// 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 { + public: + JSTransferable(Environment* env, v8::Local obj); + static void New(const v8::FunctionCallbackInfo& args); + + TransferMode GetTransferMode() const override; + std::unique_ptr TransferForMessaging() override; + std::unique_ptr CloneForMessaging() const override; + v8::Maybe>> + NestedTransferables() const override; + v8::Maybe FinalizeTransferRead( + v8::Local context, + v8::ValueDeserializer* deserializer) override; + + SET_NO_MEMORY_INFO() + SET_MEMORY_INFO_NAME(JSTransferable) + SET_SELF_SIZE(JSTransferable) + + private: + std::unique_ptr TransferOrClone(TransferMode mode) const; + + class Data : public TransferData { + public: + Data(std::string&& deserialize_info, v8::Global&& data); + + BaseObjectPtr Deserialize( + Environment* env, + v8::Local context, + std::unique_ptr self) override; + v8::Maybe FinalizeTransferWrite( + v8::Local context, + v8::ValueSerializer* serializer) override; + + SET_NO_MEMORY_INFO() + SET_MEMORY_INFO_NAME(JSTransferableTransferData) + SET_SELF_SIZE(Data) + + private: + std::string deserialize_info_; + v8::Global data_; + }; +}; + v8::Local GetMessagePortConstructorTemplate( Environment* env); diff --git a/test/parallel/test-worker-workerdata-messageport.js b/test/parallel/test-worker-workerdata-messageport.js index 352d0729412ddb..9bf3422337e963 100644 --- a/test/parallel/test-worker-workerdata-messageport.js +++ b/test/parallel/test-worker-workerdata-messageport.js @@ -55,6 +55,7 @@ const meowScript = () => 'meow'; transferList: [] }), { code: 'ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST', - message: 'MessagePort was found in message but not listed in transferList' + message: 'Object that needs transfer was found in message but not ' + + 'listed in transferList' }); } From 758fe1a8edb6c854fd63fdf939a0618c39ac9b28 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 6 Jun 2020 16:27:30 +0200 Subject: [PATCH 06/11] worker,fs: make FileHandle transferable Allow passing `FileHandle` instances in the transfer list of a `.postMessage()` call. --- doc/api/worker_threads.md | 12 +++- lib/internal/fs/promises.js | 28 +++++++- src/node_file.cc | 34 ++++++++++ src/node_file.h | 22 +++++++ ...worker-message-port-transfer-filehandle.js | 65 +++++++++++++++++++ 5 files changed, 157 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-worker-message-port-transfer-filehandle.js diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md index d4fd2f5179a3c1..f6b3b47ef9827c 100644 --- a/doc/api/worker_threads.md +++ b/doc/api/worker_threads.md @@ -318,6 +318,10 @@ are part of the channel. ### `port.postMessage(value[, transferList])` * `value` {any} @@ -335,7 +339,8 @@ In particular, the significant differences to `JSON` are: * `value` may contain typed arrays, both using `ArrayBuffer`s and `SharedArrayBuffer`s. * `value` may contain [`WebAssembly.Module`][] instances. -* `value` may not contain native (C++-backed) objects other than `MessagePort`s. +* `value` may not contain native (C++-backed) objects other than `MessagePort`s + and [`FileHandle`][]s. ```js const { MessageChannel } = require('worker_threads'); @@ -349,7 +354,8 @@ circularData.foo = circularData; port2.postMessage(circularData); ``` -`transferList` may be a list of `ArrayBuffer` and `MessagePort` objects. +`transferList` may be a list of [`ArrayBuffer`][], [`MessagePort`][] and +[`FileHandle`][] objects. After transferring, they will not be usable on the sending side of the channel anymore (even if they are not contained in `value`). Unlike with [child processes][], transferring handles such as network sockets is currently @@ -816,6 +822,7 @@ active handle in the event system. If the worker is already `unref()`ed calling [`'close'` event]: #worker_threads_event_close [`'exit'` event]: #worker_threads_event_exit +[`ArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer [`AsyncResource`]: async_hooks.html#async_hooks_class_asyncresource [`Buffer`]: buffer.html [`Buffer.allocUnsafe()`]: buffer.html#buffer_class_method_buffer_allocunsafe_size @@ -823,6 +830,7 @@ active handle in the event system. If the worker is already `unref()`ed calling [`ERR_WORKER_NOT_RUNNING`]: errors.html#ERR_WORKER_NOT_RUNNING [`EventEmitter`]: events.html [`EventTarget`]: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget +[`FileHandle`]: fs.html#fs_class_filehandle [`MessagePort`]: #worker_threads_class_messageport [`SharedArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer [`Uint8Array`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 05d82991552bad..717b26e59fa3b4 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -65,13 +65,17 @@ const { promisify } = require('internal/util'); const kHandle = Symbol('kHandle'); const kFd = Symbol('kFd'); const { kUsePromises } = binding; +const { + JSTransferable, kDeserialize, kTransfer, kTransferList +} = require('internal/worker/js_transferable'); const getDirectoryEntriesPromise = promisify(getDirents); -class FileHandle { +class FileHandle extends JSTransferable { constructor(filehandle) { + super(); this[kHandle] = filehandle; - this[kFd] = filehandle.fd; + this[kFd] = filehandle ? filehandle.fd : -1; } getAsyncId() { @@ -142,6 +146,26 @@ class FileHandle { this[kFd] = -1; return this[kHandle].close(); } + + [kTransfer]() { + const handle = this[kHandle]; + this[kFd] = -1; + this[kHandle] = null; + + return { + data: { handle }, + deserializeInfo: 'internal/fs/promises:FileHandle' + }; + } + + [kTransferList]() { + return [ this[kHandle] ]; + } + + [kDeserialize]({ handle }) { + this[kHandle] = handle; + this[kFd] = handle.fd; + } } function validateFileHandle(handle) { diff --git a/src/node_file.cc b/src/node_file.cc index c2ed56f43fb0ae..9ed2d2960cf95e 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -190,6 +190,40 @@ void FileHandle::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("current_read", current_read_); } +FileHandle::TransferMode FileHandle::GetTransferMode() const { + return reading_ || closing_ || closed_ ? + TransferMode::kUntransferable : TransferMode::kTransferable; +} + +std::unique_ptr FileHandle::TransferForMessaging() { + CHECK_NE(GetTransferMode(), TransferMode::kUntransferable); + auto ret = std::make_unique(fd_); + closed_ = true; + return ret; +} + +FileHandle::TransferData::TransferData(int fd) : fd_(fd) {} + +FileHandle::TransferData::~TransferData() { + if (fd_ > 0) { + uv_fs_t close_req; + CHECK_EQ(0, uv_fs_close(nullptr, &close_req, fd_, nullptr)); + uv_fs_req_cleanup(&close_req); + } +} + +BaseObjectPtr FileHandle::TransferData::Deserialize( + Environment* env, + v8::Local context, + std::unique_ptr self) { + BindingData* bd = Environment::GetBindingData(context); + if (bd == nullptr) return {}; + + int fd = fd_; + fd_ = -1; + return BaseObjectPtr { FileHandle::New(bd, fd) }; +} + // Close the file descriptor if it hasn't already been closed. A process // warning will be emitted using a SetImmediate to avoid calling back to // JS during GC. If closing the fd fails at this point, a fatal exception diff --git a/src/node_file.h b/src/node_file.h index ad493bd944c20c..fd17fc99d52aba 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -5,6 +5,7 @@ #include "node.h" #include "aliased_buffer.h" +#include "node_messaging.h" #include "stream_base.h" #include @@ -273,7 +274,28 @@ class FileHandle final : public AsyncWrap, public StreamBase { FileHandle(const FileHandle&&) = delete; FileHandle& operator=(const FileHandle&&) = delete; + TransferMode GetTransferMode() const override; + std::unique_ptr TransferForMessaging() override; + private: + class TransferData : public worker::TransferData { + public: + explicit TransferData(int fd); + ~TransferData(); + + BaseObjectPtr Deserialize( + Environment* env, + v8::Local context, + std::unique_ptr self) override; + + SET_NO_MEMORY_INFO() + SET_MEMORY_INFO_NAME(FileHandleTransferData) + SET_SELF_SIZE(TransferData) + + private: + int fd_; + }; + FileHandle(BindingData* binding_data, v8::Local obj, int fd); // Synchronous close that emits a warning diff --git a/test/parallel/test-worker-message-port-transfer-filehandle.js b/test/parallel/test-worker-message-port-transfer-filehandle.js new file mode 100644 index 00000000000000..3765fca865e18e --- /dev/null +++ b/test/parallel/test-worker-message-port-transfer-filehandle.js @@ -0,0 +1,65 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const fs = require('fs').promises; +const vm = require('vm'); +const { MessageChannel, moveMessagePortToContext } = require('worker_threads'); +const { once } = require('events'); + +(async function() { + const fh = await fs.open(__filename); + + const { port1, port2 } = new MessageChannel(); + + assert.throws(() => { + port1.postMessage(fh); + }, { + // See the TODO about error code in node_messaging.cc. + code: 'ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST' + }); + + // Check that transferring FileHandle instances works. + assert.notStrictEqual(fh.fd, -1); + port1.postMessage(fh, [ fh ]); + assert.strictEqual(fh.fd, -1); + + const [ fh2 ] = await once(port2, 'message'); + assert.strictEqual(Object.getPrototypeOf(fh2), Object.getPrototypeOf(fh)); + + assert.deepStrictEqual(await fh2.readFile(), await fs.readFile(__filename)); + await fh2.close(); + + assert.rejects(() => fh.readFile(), { code: 'EBADF' }); +})().then(common.mustCall()); + +(async function() { + // Check that there is no crash if the message is never read. + const fh = await fs.open(__filename); + + const { port1 } = new MessageChannel(); + + assert.notStrictEqual(fh.fd, -1); + port1.postMessage(fh, [ fh ]); + assert.strictEqual(fh.fd, -1); +})().then(common.mustCall()); + +(async function() { + // Check that in the case of a context mismatch the message is discarded. + const fh = await fs.open(__filename); + + const { port1, port2 } = new MessageChannel(); + + const ctx = vm.createContext(); + const port2moved = moveMessagePortToContext(port2, ctx); + port2moved.onmessage = common.mustCall((msgEvent) => { + assert.strictEqual(msgEvent.data, 'second message'); + port1.close(); + }); + port2moved.start(); + + assert.notStrictEqual(fh.fd, -1); + port1.postMessage(fh, [ fh ]); + assert.strictEqual(fh.fd, -1); + + port1.postMessage('second message'); +})().then(common.mustCall()); From 82a22208b2aae6dfc2432dbf0ee89741f0ae6cb6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 6 Jun 2020 16:48:35 +0200 Subject: [PATCH 07/11] fixup! worker: allow passing JS wrapper objects via postMessage --- test/parallel/test-bootstrap-modules.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index b2506499e7d1c7..4dfe63c1e3d303 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -17,6 +17,7 @@ const expectedModules = new Set([ 'Internal Binding credentials', 'Internal Binding fs', 'Internal Binding fs_dir', + 'Internal Binding messaging', 'Internal Binding module_wrap', 'Internal Binding native_module', 'Internal Binding options', @@ -77,6 +78,7 @@ const expectedModules = new Set([ 'NativeModule internal/process/warning', 'NativeModule internal/querystring', 'NativeModule internal/source_map/source_map_cache', + 'NativeModule internal/source_map/prepare_stack_trace', 'NativeModule internal/timers', 'NativeModule internal/url', 'NativeModule internal/util', @@ -85,6 +87,7 @@ const expectedModules = new Set([ 'NativeModule internal/util/types', 'NativeModule internal/validators', 'NativeModule internal/vm/module', + 'NativeModule internal/worker/js_transferable', 'NativeModule path', 'NativeModule perf_hooks', 'NativeModule timers', From 22e70e2dc1cd503750e7adb65717c471b1a90392 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 6 Jun 2020 17:09:22 +0200 Subject: [PATCH 08/11] fixup! worker,fs: make FileHandle transferable --- doc/api/worker_threads.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md index f6b3b47ef9827c..9a893a40972349 100644 --- a/doc/api/worker_threads.md +++ b/doc/api/worker_threads.md @@ -320,7 +320,7 @@ are part of the channel. added: v10.5.0 changes: - version: REPLACEME - pr-url: https://github.com/nodejs/node/pull/????? + pr-url: https://github.com/nodejs/node/pull/33772 description: Added `FileHandle` to the list of transferable types. --> From c74eeb21cd3c409c77092761489b6b0c38dc9c9f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 6 Jun 2020 17:48:29 +0200 Subject: [PATCH 09/11] fixup! fixup! worker: allow passing JS wrapper objects via postMessage --- test/parallel/test-bootstrap-modules.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 4dfe63c1e3d303..d4aa7bdd3142c5 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -78,7 +78,6 @@ const expectedModules = new Set([ 'NativeModule internal/process/warning', 'NativeModule internal/querystring', 'NativeModule internal/source_map/source_map_cache', - 'NativeModule internal/source_map/prepare_stack_trace', 'NativeModule internal/timers', 'NativeModule internal/url', 'NativeModule internal/util', From a94759ac2550f25fdc79694cdffe0bde9d030c50 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 7 Jun 2020 19:13:06 +0200 Subject: [PATCH 10/11] fixup! worker,fs: make FileHandle transferable --- lib/internal/worker/js_transferable.js | 8 ++++ ...-transfer-fake-js-transferable-internal.js | 33 +++++++++++++++++ ...sage-port-transfer-fake-js-transferable.js | 37 +++++++++++++++++++ 3 files changed, 78 insertions(+) create mode 100644 test/parallel/test-worker-message-port-transfer-fake-js-transferable-internal.js create mode 100644 test/parallel/test-worker-message-port-transfer-fake-js-transferable.js diff --git a/lib/internal/worker/js_transferable.js b/lib/internal/worker/js_transferable.js index 41f1e6ff72ae06..707fd03f2f6d0e 100644 --- a/lib/internal/worker/js_transferable.js +++ b/lib/internal/worker/js_transferable.js @@ -1,4 +1,5 @@ 'use strict'; +const { Error } = primordials; const { messaging_deserialize_symbol, messaging_transfer_symbol, @@ -17,6 +18,13 @@ function setup() { setDeserializerCreateObjectFunction((deserializeInfo) => { const [ module, ctor ] = deserializeInfo.split(':'); const Ctor = require(module)[ctor]; + if (typeof Ctor !== 'function' || + !(Ctor.prototype instanceof JSTransferable)) { + // 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 + throw new Error(`Unknown deserialize spec ${deserializeInfo}`); + } return new Ctor(); }); } diff --git a/test/parallel/test-worker-message-port-transfer-fake-js-transferable-internal.js b/test/parallel/test-worker-message-port-transfer-fake-js-transferable-internal.js new file mode 100644 index 00000000000000..10a504943389d8 --- /dev/null +++ b/test/parallel/test-worker-message-port-transfer-fake-js-transferable-internal.js @@ -0,0 +1,33 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const fs = require('fs').promises; +const { MessageChannel } = require('worker_threads'); +const { once } = require('events'); + +// Test that overriding the internal kTransfer method of a JSTransferable does +// not enable loading arbitrary code from internal Node.js core modules. + +(async function() { + const fh = await fs.open(__filename); + assert.strictEqual(fh.constructor.name, 'FileHandle'); + + const kTransfer = Object.getOwnPropertySymbols(Object.getPrototypeOf(fh)) + .filter((symbol) => symbol.description === 'messaging_transfer_symbol')[0]; + assert.strictEqual(typeof kTransfer, 'symbol'); + fh[kTransfer] = () => { + return { + data: '✨', + deserializeInfo: 'net:Socket' + }; + }; + + const { port1, port2 } = new MessageChannel(); + port1.postMessage(fh, [ fh ]); + port2.on('message', common.mustNotCall()); + + const [ exception ] = await once(process, 'uncaughtException'); + + assert.strictEqual(exception.message, 'Unknown deserialize spec net:Socket'); + port2.close(); +})().then(common.mustCall()); diff --git a/test/parallel/test-worker-message-port-transfer-fake-js-transferable.js b/test/parallel/test-worker-message-port-transfer-fake-js-transferable.js new file mode 100644 index 00000000000000..924d850a5d182c --- /dev/null +++ b/test/parallel/test-worker-message-port-transfer-fake-js-transferable.js @@ -0,0 +1,37 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const fs = require('fs').promises; +const { MessageChannel } = require('worker_threads'); +const { once } = require('events'); + +// Test that overriding the internal kTransfer method of a JSTransferable does +// not enable loading arbitrary code from the disk. + +module.exports = { + NotARealClass: common.mustNotCall() +}; + +(async function() { + const fh = await fs.open(__filename); + assert.strictEqual(fh.constructor.name, 'FileHandle'); + + const kTransfer = Object.getOwnPropertySymbols(Object.getPrototypeOf(fh)) + .filter((symbol) => symbol.description === 'messaging_transfer_symbol')[0]; + assert.strictEqual(typeof kTransfer, 'symbol'); + fh[kTransfer] = () => { + return { + data: '✨', + deserializeInfo: `${__filename}:NotARealClass` + }; + }; + + const { port1, port2 } = new MessageChannel(); + port1.postMessage(fh, [ fh ]); + port2.on('message', common.mustNotCall()); + + const [ exception ] = await once(process, 'uncaughtException'); + + assert.match(exception.message, /Missing internal module/); + port2.close(); +})().then(common.mustCall()); From 9f487ab3545b9ff3d4abed23a7c9709864fa185c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 8 Jun 2020 17:42:45 +0200 Subject: [PATCH 11/11] 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. --- doc/api/errors.md | 12 +++++++++ doc/api/worker_threads.md | 18 +++++++++++++ lib/internal/worker.js | 4 ++- src/env.h | 2 ++ src/node_errors.h | 4 +++ src/node_messaging.cc | 25 ++++++++++++++++--- ...-transfer-fake-js-transferable-internal.js | 2 +- ...sage-port-transfer-fake-js-transferable.js | 2 +- ...worker-message-port-transfer-filehandle.js | 6 +++++ 9 files changed, 69 insertions(+), 6 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 43c0b1dedaf5b8..3079e2031d7a9f 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1566,6 +1566,17 @@ behavior. See the documentation for [policy][] manifests for more information. An attempt was made to allocate memory (usually in the C++ layer) but it failed. + +### `ERR_MESSAGE_TARGET_CONTEXT_UNAVAILABLE` + + +A message posted to a [`MessagePort`][] could not be deserialized in the target +[vm][] `Context`. Not all Node.js objects can be successfully instantiated in +any context at this time, and attempting to transfer them using `postMessage()` +can fail on the receiving side in that case. + ### `ERR_METHOD_NOT_IMPLEMENTED` @@ -2564,6 +2575,7 @@ such as `process.stdout.on('data')`. [`Class: assert.AssertionError`]: assert.html#assert_class_assert_assertionerror [`ERR_INVALID_ARG_TYPE`]: #ERR_INVALID_ARG_TYPE [`EventEmitter`]: events.html#events_class_eventemitter +[`MessagePort`]: worker_threads.html#worker_threads_class_messageport [`Object.getPrototypeOf`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getPrototypeOf [`Object.setPrototypeOf`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/setPrototypeOf [`REPL`]: repl.html diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md index 9a893a40972349..702abca0a01dd9 100644 --- a/doc/api/worker_threads.md +++ b/doc/api/worker_threads.md @@ -303,6 +303,15 @@ input of [`port.postMessage()`][]. Listeners on this event will receive a clone of the `value` parameter as passed to `postMessage()` and no further arguments. +### Event: `'messageerror'` + + +* `error` {Error} An Error object + +The `'messageerror'` event is emitted when deserializing a message failed. + ### `port.close()` + +* `error` {Error} An Error object + +The `'messageerror'` event is emitted when deserializing a message failed. + ### Event: `'online'`