diff options
40 files changed, 147 insertions, 244 deletions
diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 06dcbb4fb1..f7a6d4e68d 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -123,8 +123,8 @@ RetainedObjectInfo* WrapperInfo(uint16_t class_id, Local<Value> wrapper) { Local<Object> object = wrapper.As<Object>(); CHECK_GT(object->InternalFieldCount(), 0); - AsyncWrap* wrap = Unwrap<AsyncWrap>(object); - if (wrap == nullptr) return nullptr; // ClearWrap() already called. + AsyncWrap* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, object, nullptr); return new RetainedAsyncInfo(class_id, wrap); } @@ -231,7 +231,7 @@ class PromiseWrap : public AsyncWrap { public: PromiseWrap(Environment* env, Local<Object> object, bool silent) : AsyncWrap(env, object, PROVIDER_PROMISE, -1, silent) { - MakeWeak(this); + MakeWeak(); } size_t self_size() const override { return sizeof(*this); } diff --git a/src/base_object-inl.h b/src/base_object-inl.h index 5ff211f473..11ba1c88da 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -31,54 +31,90 @@ namespace node { -inline BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> handle) +BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> handle) : persistent_handle_(env->isolate(), handle), env_(env) { CHECK_EQ(false, handle.IsEmpty()); - // The zero field holds a pointer to the handle. Immediately set it to - // nullptr in case it's accessed by the user before construction is complete. - if (handle->InternalFieldCount() > 0) - handle->SetAlignedPointerInInternalField(0, nullptr); + CHECK_GT(handle->InternalFieldCount(), 0); + handle->SetAlignedPointerInInternalField(0, static_cast<void*>(this)); } -inline Persistent<v8::Object>& BaseObject::persistent() { +BaseObject::~BaseObject() { + if (persistent_handle_.IsEmpty()) { + // This most likely happened because the weak callback below cleared it. + return; + } + + { + v8::HandleScope handle_scope(env_->isolate()); + object()->SetAlignedPointerInInternalField(0, nullptr); + } +} + + +Persistent<v8::Object>& BaseObject::persistent() { return persistent_handle_; } -inline v8::Local<v8::Object> BaseObject::object() { +v8::Local<v8::Object> BaseObject::object() { return PersistentToLocal(env_->isolate(), persistent_handle_); } -inline Environment* BaseObject::env() const { +Environment* BaseObject::env() const { return env_; } -template <typename Type> -inline void BaseObject::WeakCallback( - const v8::WeakCallbackInfo<Type>& data) { - delete data.GetParameter(); +BaseObject* BaseObject::FromJSObject(v8::Local<v8::Object> obj) { + CHECK_GT(obj->InternalFieldCount(), 0); + return static_cast<BaseObject*>(obj->GetAlignedPointerFromInternalField(0)); } -template <typename Type> -inline void BaseObject::MakeWeak(Type* ptr) { - v8::HandleScope scope(env_->isolate()); - v8::Local<v8::Object> handle = object(); - CHECK_GT(handle->InternalFieldCount(), 0); - Wrap(handle, ptr); - persistent_handle_.SetWeak<Type>(ptr, WeakCallback<Type>, - v8::WeakCallbackType::kParameter); +template <typename T> +T* BaseObject::FromJSObject(v8::Local<v8::Object> object) { + return static_cast<T*>(FromJSObject(object)); } -inline void BaseObject::ClearWeak() { +void BaseObject::MakeWeak() { + persistent_handle_.SetWeak( + this, + [](const v8::WeakCallbackInfo<BaseObject>& data) { + BaseObject* obj = data.GetParameter(); + // Clear the persistent handle so that ~BaseObject() doesn't attempt + // to mess with internal fields, since the JS object may have + // transitioned into an invalid state. + // Refs: https://github.com/nodejs/node/issues/18897 + obj->persistent_handle_.Reset(); + delete obj; + }, v8::WeakCallbackType::kParameter); +} + + +void BaseObject::ClearWeak() { persistent_handle_.ClearWeak(); } + +v8::Local<v8::FunctionTemplate> +BaseObject::MakeLazilyInitializedJSTemplate(Environment* env) { + auto constructor = [](const v8::FunctionCallbackInfo<v8::Value>& args) { +#ifdef DEBUG + CHECK(args.IsConstructCall()); + CHECK_GT(args.This()->InternalFieldCount(), 0); +#endif + args.This()->SetAlignedPointerInInternalField(0, nullptr); + }; + + v8::Local<v8::FunctionTemplate> t = env->NewFunctionTemplate(constructor); + t->InstanceTemplate()->SetInternalFieldCount(1); + return t; +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/base_object.h b/src/base_object.h index 478499bbfe..7d8281238b 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -26,6 +26,7 @@ #include "node_persistent.h" #include "v8.h" +#include <type_traits> // std::remove_reference namespace node { @@ -33,8 +34,10 @@ class Environment; class BaseObject { public: + // Associates this object with `handle`. It uses the 0th internal field for + // that, and in particular aborts if there is no such field. inline BaseObject(Environment* env, v8::Local<v8::Object> handle); - virtual ~BaseObject() = default; + virtual inline ~BaseObject(); // Returns the wrapped object. Returns an empty handle when // persistent.IsEmpty() is true. @@ -44,23 +47,30 @@ class BaseObject { inline Environment* env() const; - // The handle_ must have an internal field count > 0, and the first - // index is reserved for a pointer to this class. This is an - // implicit requirement, but Node does not have a case where it's - // required that MakeWeak() be called and the internal field not - // be set. - template <typename Type> - inline void MakeWeak(Type* ptr); + // Get a BaseObject* pointer, or subclass pointer, for the JS object that + // was also passed to the `BaseObject()` constructor initially. + // This may return `nullptr` if the C++ object has not been constructed yet, + // e.g. when the JS object used `MakeLazilyInitializedJSTemplate`. + static inline BaseObject* FromJSObject(v8::Local<v8::Object> object); + template <typename T> + static inline T* FromJSObject(v8::Local<v8::Object> object); + // Make the `Persistent` a weak reference and, `delete` this object once + // the JS object has been garbage collected. + inline void MakeWeak(); + + // Undo `MakeWeak()`, i.e. turn this into a strong reference. inline void ClearWeak(); + // Utility to create a FunctionTemplate with one internal field (used for + // the `BaseObject*` pointer) and a constructor that initializes that field + // to `nullptr`. + static inline v8::Local<v8::FunctionTemplate> MakeLazilyInitializedJSTemplate( + Environment* env); + private: BaseObject(); - template <typename Type> - static inline void WeakCallback( - const v8::WeakCallbackInfo<Type>& data); - // persistent_handle_ needs to be at a fixed offset from the start of the // class because it is used by src/node_postmortem_metadata.cc to calculate // offsets and generate debug symbols for BaseObject, which assumes that the @@ -71,6 +81,22 @@ class BaseObject { Environment* env_; }; + +// Global alias for FromJSObject() to avoid churn. +template <typename T> +inline T* Unwrap(v8::Local<v8::Object> obj) { + return BaseObject::FromJSObject<T>(obj); +} + + +#define ASSIGN_OR_RETURN_UNWRAP(ptr, obj, ...) \ + do { \ + *ptr = static_cast<typename std::remove_reference<decltype(*ptr)>::type>( \ + BaseObject::FromJSObject(obj)); \ + if (*ptr == nullptr) \ + return __VA_ARGS__; \ + } while (0) + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 0cc7ed1464..4df47d75d4 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -187,7 +187,7 @@ ChannelWrap::ChannelWrap(Environment* env, is_servers_default_(true), library_inited_(false), active_query_count_(0) { - MakeWeak<ChannelWrap>(this); + MakeWeak(); Setup(); } @@ -205,7 +205,6 @@ class GetAddrInfoReqWrap : public ReqWrap<uv_getaddrinfo_t> { GetAddrInfoReqWrap(Environment* env, Local<Object> req_wrap_obj, bool verbatim); - ~GetAddrInfoReqWrap(); size_t self_size() const override { return sizeof(*this); } bool verbatim() const { return verbatim_; } @@ -219,18 +218,12 @@ GetAddrInfoReqWrap::GetAddrInfoReqWrap(Environment* env, bool verbatim) : ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_GETADDRINFOREQWRAP) , verbatim_(verbatim) { - Wrap(req_wrap_obj, this); -} - -GetAddrInfoReqWrap::~GetAddrInfoReqWrap() { - ClearWrap(object()); } class GetNameInfoReqWrap : public ReqWrap<uv_getnameinfo_t> { public: GetNameInfoReqWrap(Environment* env, Local<Object> req_wrap_obj); - ~GetNameInfoReqWrap(); size_t self_size() const override { return sizeof(*this); } }; @@ -238,11 +231,6 @@ class GetNameInfoReqWrap : public ReqWrap<uv_getnameinfo_t> { GetNameInfoReqWrap::GetNameInfoReqWrap(Environment* env, Local<Object> req_wrap_obj) : ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_GETNAMEINFOREQWRAP) { - Wrap(req_wrap_obj, this); -} - -GetNameInfoReqWrap::~GetNameInfoReqWrap() { - ClearWrap(object()); } @@ -587,8 +575,6 @@ class QueryWrap : public AsyncWrap { QueryWrap(ChannelWrap* channel, Local<Object> req_wrap_obj) : AsyncWrap(channel->env(), req_wrap_obj, AsyncWrap::PROVIDER_QUERYWRAP), channel_(channel) { - Wrap(req_wrap_obj, this); - // Make sure the channel object stays alive during the query lifetime. req_wrap_obj->Set(env()->context(), env()->channel_string(), @@ -597,7 +583,6 @@ class QueryWrap : public AsyncWrap { ~QueryWrap() override { CHECK_EQ(false, persistent().IsEmpty()); - ClearWrap(object()); } // Subclasses should implement the appropriate Send method. @@ -2143,14 +2128,8 @@ void Initialize(Local<Object> target, target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "AI_V4MAPPED"), Integer::New(env->isolate(), AI_V4MAPPED)); - auto is_construct_call_callback = - [](const FunctionCallbackInfo<Value>& args) { - CHECK(args.IsConstructCall()); - ClearWrap(args.This()); - }; Local<FunctionTemplate> aiw = - FunctionTemplate::New(env->isolate(), is_construct_call_callback); - aiw->InstanceTemplate()->SetInternalFieldCount(1); + BaseObject::MakeLazilyInitializedJSTemplate(env); AsyncWrap::AddWrapMethods(env, aiw); Local<String> addrInfoWrapString = FIXED_ONE_BYTE_STRING(env->isolate(), "GetAddrInfoReqWrap"); @@ -2158,8 +2137,7 @@ void Initialize(Local<Object> target, target->Set(addrInfoWrapString, aiw->GetFunction()); Local<FunctionTemplate> niw = - FunctionTemplate::New(env->isolate(), is_construct_call_callback); - niw->InstanceTemplate()->SetInternalFieldCount(1); + BaseObject::MakeLazilyInitializedJSTemplate(env); AsyncWrap::AddWrapMethods(env, niw); Local<String> nameInfoWrapString = FIXED_ONE_BYTE_STRING(env->isolate(), "GetNameInfoReqWrap"); @@ -2167,8 +2145,7 @@ void Initialize(Local<Object> target, target->Set(nameInfoWrapString, niw->GetFunction()); Local<FunctionTemplate> qrw = - FunctionTemplate::New(env->isolate(), is_construct_call_callback); - qrw->InstanceTemplate()->SetInternalFieldCount(1); + BaseObject::MakeLazilyInitializedJSTemplate(env); AsyncWrap::AddWrapMethods(env, qrw); Local<String> queryWrapString = FIXED_ONE_BYTE_STRING(env->isolate(), "QueryReqWrap"); diff --git a/src/connect_wrap.cc b/src/connect_wrap.cc index f9ea987e05..dacdf72da7 100644 --- a/src/connect_wrap.cc +++ b/src/connect_wrap.cc @@ -13,12 +13,6 @@ using v8::Object; ConnectWrap::ConnectWrap(Environment* env, Local<Object> req_wrap_obj, AsyncWrap::ProviderType provider) : ReqWrap(env, req_wrap_obj, provider) { - Wrap(req_wrap_obj, this); -} - - -ConnectWrap::~ConnectWrap() { - ClearWrap(object()); } } // namespace node diff --git a/src/connect_wrap.h b/src/connect_wrap.h index 6227542bcb..80eae7f9bb 100644 --- a/src/connect_wrap.h +++ b/src/connect_wrap.h @@ -15,7 +15,6 @@ class ConnectWrap : public ReqWrap<uv_connect_t> { ConnectWrap(Environment* env, v8::Local<v8::Object> req_wrap_obj, AsyncWrap::ProviderType provider); - ~ConnectWrap(); size_t self_size() const override { return sizeof(*this); } }; diff --git a/src/connection_wrap.h b/src/connection_wrap.h index 096672efdd..afb168c614 100644 --- a/src/connection_wrap.h +++ b/src/connection_wrap.h @@ -29,7 +29,6 @@ class ConnectionWrap : public LibuvStreamWrap { UVType handle_; }; - } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc index 1e7505d6d3..ed74f36719 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -131,7 +131,7 @@ void FSEventWrap::New(const FunctionCallbackInfo<Value>& args) { void FSEventWrap::Start(const FunctionCallbackInfo<Value>& args) { Environment* env = Environment::GetCurrent(args); - FSEventWrap* wrap = Unwrap<FSEventWrap>(args.Holder()); + FSEventWrap* wrap = Unwrap<FSEventWrap>(args.This()); CHECK_NE(wrap, nullptr); CHECK(!wrap->initialized_); diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index a3b0209eb3..49bf0c55be 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -93,7 +93,6 @@ HandleWrap::HandleWrap(Environment* env, handle_(handle) { handle_->data = this; HandleScope scope(env->isolate()); - Wrap(object, this); env->handle_wrap_queue()->PushBack(this); } @@ -114,7 +113,6 @@ void HandleWrap::OnClose(uv_handle_t* handle) { if (have_close_callback) wrap->MakeCallback(env->onclose_string(), 0, nullptr); - ClearWrap(wrap->object()); delete wrap; } diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 37cdcecd61..ae353defe8 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -64,7 +64,6 @@ class JSBindingsConnection : public AsyncWrap { Local<Function> callback) : AsyncWrap(env, wrap, PROVIDER_INSPECTORJSBINDING), callback_(env->isolate(), callback) { - Wrap(wrap, this); Agent* inspector = env->inspector_agent(); session_ = inspector->Connect(std::unique_ptr<JSBindingsSessionDelegate>( new JSBindingsSessionDelegate(env, this))); @@ -83,9 +82,6 @@ class JSBindingsConnection : public AsyncWrap { void Disconnect() { session_.reset(); - if (!persistent().IsEmpty()) { - ClearWrap(object()); - } delete this; } diff --git a/src/js_stream.cc b/src/js_stream.cc index ed6c6ee738..2293d8cf20 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -24,12 +24,7 @@ using v8::Value; JSStream::JSStream(Environment* env, Local<Object> obj) : AsyncWrap(env, obj, AsyncWrap::PROVIDER_JSSTREAM), StreamBase(env) { - node::Wrap(obj, this); - MakeWeak<JSStream>(this); -} - - -JSStream::~JSStream() { + MakeWeak(); } diff --git a/src/js_stream.h b/src/js_stream.h index 338cbe5456..b47a91a653 100644 --- a/src/js_stream.h +++ b/src/js_stream.h @@ -16,8 +16,6 @@ class JSStream : public AsyncWrap, public StreamBase { v8::Local<v8::Value> unused, v8::Local<v8::Context> context); - ~JSStream(); - bool IsAlive() override; bool IsClosing() override; int ReadStart() override; diff --git a/src/module_wrap.cc b/src/module_wrap.cc index f88c113ae0..71f1acdaf6 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -158,7 +158,6 @@ void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) { obj->context_.Reset(isolate, context); env->module_map.emplace(module->GetIdentityHash(), obj); - Wrap(that, obj); that->SetIntegrityLevel(context, IntegrityLevel::kFrozen); args.GetReturnValue().Set(that); diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 651e147617..4db82d4d7c 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -885,7 +885,7 @@ class ContextifyScript : public BaseObject { ContextifyScript(Environment* env, Local<Object> object) : BaseObject(env, object) { - MakeWeak<ContextifyScript>(this); + MakeWeak(); } }; diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 20a9e2ec4e..4b6eee5574 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4690,7 +4690,6 @@ class PBKDF2Request : public AsyncWrap { keylen_(keylen), key_(node::Malloc(keylen)), iter_(iter) { - Wrap(object, this); } ~PBKDF2Request() override { @@ -4705,8 +4704,6 @@ class PBKDF2Request : public AsyncWrap { free(key_); key_ = nullptr; keylen_ = 0; - - ClearWrap(object()); } uv_work_t* work_req() { @@ -4868,11 +4865,6 @@ class RandomBytesRequest : public AsyncWrap { size_(size), data_(data), free_mode_(free_mode) { - Wrap(object, this); - } - - ~RandomBytesRequest() override { - ClearWrap(object()); } uv_work_t* work_req() { diff --git a/src/node_crypto.h b/src/node_crypto.h index 3963f7050f..6f34eae359 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -175,7 +175,7 @@ class SecureContext : public BaseObject { ctx_(nullptr), cert_(nullptr), issuer_(nullptr) { - MakeWeak<SecureContext>(this); + MakeWeak(); env->isolate()->AdjustAmountOfExternalAllocatedMemory(kExternalSize); } @@ -403,7 +403,7 @@ class CipherBase : public BaseObject { auth_tag_set_(false), auth_tag_len_(kNoAuthTagLength), pending_auth_failed_(false) { - MakeWeak<CipherBase>(this); + MakeWeak(); } private: @@ -434,7 +434,7 @@ class Hmac : public BaseObject { Hmac(Environment* env, v8::Local<v8::Object> wrap) : BaseObject(env, wrap), ctx_(nullptr) { - MakeWeak<Hmac>(this); + MakeWeak(); } private: @@ -459,7 +459,7 @@ class Hash : public BaseObject { : BaseObject(env, wrap), mdctx_(nullptr), finalized_(false) { - MakeWeak<Hash>(this); + MakeWeak(); } private: @@ -514,7 +514,7 @@ class Sign : public SignBase { static void SignFinal(const v8::FunctionCallbackInfo<v8::Value>& args); Sign(Environment* env, v8::Local<v8::Object> wrap) : SignBase(env, wrap) { - MakeWeak<Sign>(this); + MakeWeak(); } }; @@ -537,7 +537,7 @@ class Verify : public SignBase { static void VerifyFinal(const v8::FunctionCallbackInfo<v8::Value>& args); Verify(Environment* env, v8::Local<v8::Object> wrap) : SignBase(env, wrap) { - MakeWeak<Verify>(this); + MakeWeak(); } }; @@ -605,7 +605,7 @@ class DiffieHellman : public BaseObject { initialised_(false), verifyError_(0), dh(nullptr) { - MakeWeak<DiffieHellman>(this); + MakeWeak(); } private: @@ -641,7 +641,7 @@ class ECDH : public BaseObject { : BaseObject(env, wrap), key_(key), group_(EC_KEY_get0_group(key_)) { - MakeWeak<ECDH>(this); + MakeWeak(); CHECK_NE(group_, nullptr); } diff --git a/src/node_file.cc b/src/node_file.cc index 89c53afc5b..97b957eed6 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -99,7 +99,7 @@ FileHandle::FileHandle(Environment* env, int fd, Local<Object> obj) AsyncWrap::PROVIDER_FILEHANDLE), StreamBase(env), fd_(fd) { - MakeWeak<FileHandle>(this); + MakeWeak(); v8::PropertyAttribute attr = static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete); object()->DefineOwnProperty(env->context(), diff --git a/src/node_file.h b/src/node_file.h index d6c8aa443c..03e41097d5 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -28,11 +28,6 @@ class FSReqBase : public ReqWrap<uv_fs_t> { FSReqBase(Environment* env, Local<Object> req, AsyncWrap::ProviderType type) : ReqWrap(env, req, type) { - Wrap(object(), this); - } - - virtual ~FSReqBase() { - ClearWrap(object()); } void Init(const char* syscall, @@ -249,10 +244,10 @@ class FileHandle : public AsyncWrap, public StreamBase { env->fdclose_constructor_template() ->NewInstance(env->context()).ToLocalChecked(), AsyncWrap::PROVIDER_FILEHANDLECLOSEREQ) { - Wrap(object(), this); promise_.Reset(env->isolate(), promise); ref_.Reset(env->isolate(), ref); } + ~CloseReq() { uv_fs_req_cleanup(req()); promise_.Reset(); diff --git a/src/node_http2.cc b/src/node_http2.cc index cb5c14a6fa..3563013da3 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -244,11 +244,6 @@ Http2Session::Http2Settings::Http2Settings( Init(); } -Http2Session::Http2Settings::~Http2Settings() { - if (!object().IsEmpty()) - ClearWrap(object()); -} - // Generates a Buffer that contains the serialized payload of a SETTINGS // frame. This can be used, for instance, to create the Base64-encoded // content of an Http2-Settings header field. @@ -458,7 +453,7 @@ Http2Session::Http2Session(Environment* env, nghttp2_session_type type) : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_HTTP2SESSION), session_type_(type) { - MakeWeak<Http2Session>(this); + MakeWeak(); statistics_.start_time = uv_hrtime(); // Capture the configuration options for this session @@ -1668,7 +1663,7 @@ Http2Stream::Http2Stream( session_(session), id_(id), current_headers_category_(category) { - MakeWeak<Http2Stream>(this); + MakeWeak(); statistics_.start_time = uv_hrtime(); // Limit the number of header pairs @@ -2615,11 +2610,6 @@ Http2Session::Http2Ping::Http2Ping( session_(session), startTime_(uv_hrtime()) { } -Http2Session::Http2Ping::~Http2Ping() { - if (!object().IsEmpty()) - ClearWrap(object()); -} - void Http2Session::Http2Ping::Send(uint8_t* payload) { uint8_t data[8]; if (payload == nullptr) { diff --git a/src/node_http2.h b/src/node_http2.h index 8ee4fd450e..f4ac926bb5 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -1148,7 +1148,6 @@ class Http2StreamPerformanceEntry : public PerformanceEntry { class Http2Session::Http2Ping : public AsyncWrap { public: explicit Http2Ping(Http2Session* session); - ~Http2Ping(); size_t self_size() const override { return sizeof(*this); } @@ -1169,7 +1168,6 @@ class Http2Session::Http2Settings : public AsyncWrap { public: explicit Http2Settings(Environment* env); explicit Http2Settings(Http2Session* session); - ~Http2Settings(); size_t self_size() const override { return sizeof(*this); } diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 085f4494e3..d6f9b110c3 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -151,16 +151,10 @@ class Parser : public AsyncWrap, public StreamListener { : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_HTTPPARSER), current_buffer_len_(0), current_buffer_data_(nullptr) { - Wrap(object(), this); Init(type); } - ~Parser() override { - ClearWrap(object()); - } - - size_t self_size() const override { return sizeof(*this); } diff --git a/src/node_i18n.cc b/src/node_i18n.cc index f491d2191d..5dd30a254a 100644 --- a/src/node_i18n.cc +++ b/src/node_i18n.cc @@ -259,7 +259,7 @@ class ConverterObject : public BaseObject, Converter { BaseObject(env, wrap), Converter(converter, sub), ignoreBOM_(ignoreBOM) { - MakeWeak<ConverterObject>(this); + MakeWeak(); switch (ucnv_getType(converter)) { case UCNV_UTF8: diff --git a/src/node_internals.h b/src/node_internals.h index b8acfa63c2..e5ea575ebc 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -244,9 +244,10 @@ v8::Local<v8::Object> AddressToJS( template <typename T, int (*F)(const typename T::HandleType*, sockaddr*, int*)> void GetSockOrPeerName(const v8::FunctionCallbackInfo<v8::Value>& args) { - T* const wrap = Unwrap<T>(args.Holder()); - if (wrap == nullptr) - return args.GetReturnValue().Set(UV_EBADF); + T* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, + args.Holder(), + args.GetReturnValue().Set(UV_EBADF)); CHECK(args[0]->IsObject()); sockaddr_storage storage; int addrlen = sizeof(storage); diff --git a/src/node_serdes.cc b/src/node_serdes.cc index 6ace942c29..520b350199 100644 --- a/src/node_serdes.cc +++ b/src/node_serdes.cc @@ -86,7 +86,7 @@ class DeserializerContext : public BaseObject, SerializerContext::SerializerContext(Environment* env, Local<Object> wrap) : BaseObject(env, wrap), serializer_(env->isolate(), this) { - MakeWeak<SerializerContext>(this); + MakeWeak(); } void SerializerContext::ThrowDataCloneError(Local<String> message) { @@ -274,7 +274,7 @@ DeserializerContext::DeserializerContext(Environment* env, deserializer_(env->isolate(), data_, length_, this) { object()->Set(env->context(), env->buffer_string(), buffer).FromJust(); - MakeWeak<DeserializerContext>(this); + MakeWeak(); } MaybeLocal<Object> DeserializerContext::ReadHostObject(Isolate* isolate) { diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index 41683a0dc1..a2cfb1088c 100644 --- a/src/node_stat_watcher.cc +++ b/src/node_stat_watcher.cc @@ -83,8 +83,7 @@ static void Delete(uv_handle_t* handle) { StatWatcher::StatWatcher(Environment* env, Local<Object> wrap) : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_STATWATCHER), watcher_(new uv_fs_poll_t) { - MakeWeak<StatWatcher>(this); - Wrap(wrap, this); + MakeWeak(); uv_fs_poll_init(env->event_loop(), watcher_); watcher_->data = static_cast<void*>(this); } @@ -191,7 +190,7 @@ void StatWatcher::Stop(const FunctionCallbackInfo<Value>& args) { void StatWatcher::Stop() { uv_fs_poll_stop(watcher_); - MakeWeak<StatWatcher>(this); + MakeWeak(); } diff --git a/src/node_trace_events.cc b/src/node_trace_events.cc index f37537d545..0c0699f7be 100644 --- a/src/node_trace_events.cc +++ b/src/node_trace_events.cc @@ -37,7 +37,7 @@ class NodeCategorySet : public BaseObject { Local<Object> wrap, std::set<std::string> categories) : BaseObject(env, wrap), categories_(categories) { - MakeWeak<NodeCategorySet>(this); + MakeWeak(); } bool enabled_ = false; diff --git a/src/node_wrap.h b/src/node_wrap.h index 843fa71517..67cea2e715 100644 --- a/src/node_wrap.h +++ b/src/node_wrap.h @@ -33,6 +33,8 @@ namespace node { +// TODO(addaleax): Use real inheritance for the JS object templates to avoid +// this unnecessary case switching. #define WITH_GENERIC_UV_STREAM(env, obj, BODY, ELSE) \ do { \ if (env->tcp_constructor_template().IsEmpty() == false && \ diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 67987baf8a..ec447638e2 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -89,8 +89,6 @@ class ZCtx : public AsyncWrap { refs_(0), gzip_id_bytes_read_(0), write_result_(nullptr) { - MakeWeak<ZCtx>(this); - Wrap(wrap, this); } @@ -662,7 +660,7 @@ class ZCtx : public AsyncWrap { void Unref() { CHECK_GT(refs_, 0); if (--refs_ == 0) { - MakeWeak<ZCtx>(this); + MakeWeak(); } } diff --git a/src/pipe_wrap.cc b/src/pipe_wrap.cc index 0116051b3b..da7cb9e3ab 100644 --- a/src/pipe_wrap.cc +++ b/src/pipe_wrap.cc @@ -102,12 +102,7 @@ void PipeWrap::Initialize(Local<Object> target, env->set_pipe_constructor_template(t); // Create FunctionTemplate for PipeConnectWrap. - auto constructor = [](const FunctionCallbackInfo<Value>& args) { - CHECK(args.IsConstructCall()); - ClearWrap(args.This()); - }; - auto cwt = FunctionTemplate::New(env->isolate(), constructor); - cwt->InstanceTemplate()->SetInternalFieldCount(1); + auto cwt = BaseObject::MakeLazilyInitializedJSTemplate(env); AsyncWrap::AddWrapMethods(env, cwt); Local<String> wrapString = FIXED_ONE_BYTE_STRING(env->isolate(), "PipeConnectWrap"); diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 4f4252073d..cfe0de0872 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -243,12 +243,6 @@ SimpleShutdownWrap<OtherBase>::SimpleShutdownWrap( OtherBase(stream->stream_env(), req_wrap_obj, AsyncWrap::PROVIDER_SHUTDOWNWRAP) { - Wrap(req_wrap_obj, static_cast<AsyncWrap*>(this)); -} - -template <typename OtherBase> -SimpleShutdownWrap<OtherBase>::~SimpleShutdownWrap() { - ClearWrap(static_cast<AsyncWrap*>(this)->object()); } inline ShutdownWrap* StreamBase::CreateShutdownWrap( @@ -264,12 +258,6 @@ SimpleWriteWrap<OtherBase>::SimpleWriteWrap( OtherBase(stream->stream_env(), req_wrap_obj, AsyncWrap::PROVIDER_WRITEWRAP) { - Wrap(req_wrap_obj, static_cast<AsyncWrap*>(this)); -} - -template <typename OtherBase> -SimpleWriteWrap<OtherBase>::~SimpleWriteWrap() { - ClearWrap(static_cast<AsyncWrap*>(this)->object()); } inline WriteWrap* StreamBase::CreateWriteWrap( @@ -460,7 +448,7 @@ inline void StreamReq::ResetObject(v8::Local<v8::Object> obj) { #ifdef DEBUG CHECK_GT(obj->InternalFieldCount(), StreamReq::kStreamReqField); #endif - ClearWrap(obj); + obj->SetAlignedPointerInInternalField(0, nullptr); // BaseObject field. obj->SetAlignedPointerInInternalField(StreamReq::kStreamReqField, nullptr); } diff --git a/src/stream_base.cc b/src/stream_base.cc index 801b7f4b2f..3708ffe7b6 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -286,12 +286,6 @@ int StreamBase::WriteString(const FunctionCallbackInfo<Value>& args) { uv_stream_t* send_handle = nullptr; if (IsIPCPipe() && !send_handle_obj.IsEmpty()) { - // TODO(addaleax): This relies on the fact that HandleWrap comes first - // as a superclass of each individual subclass. - // There are similar assumptions in other places in the code base. - // A better idea would be having all BaseObject's internal pointers - // refer to the BaseObject* itself; this would require refactoring - // throughout the code base but makes Node rely much less on C++ quirks. HandleWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, send_handle_obj, UV_EINVAL); send_handle = reinterpret_cast<uv_stream_t*>(wrap->GetHandle()); diff --git a/src/stream_base.h b/src/stream_base.h index 94a091988c..b91cf7df6c 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -350,7 +350,6 @@ class SimpleShutdownWrap : public ShutdownWrap, public OtherBase { public: SimpleShutdownWrap(StreamBase* stream, v8::Local<v8::Object> req_wrap_obj); - ~SimpleShutdownWrap(); AsyncWrap* GetAsyncWrap() override { return this; } size_t self_size() const override { return sizeof(*this); } @@ -361,7 +360,6 @@ class SimpleWriteWrap : public WriteWrap, public OtherBase { public: SimpleWriteWrap(StreamBase* stream, v8::Local<v8::Object> req_wrap_obj); - ~SimpleWriteWrap(); AsyncWrap* GetAsyncWrap() override { return this; } size_t self_size() const override { return sizeof(*this) + StorageSize(); } diff --git a/src/stream_pipe.cc b/src/stream_pipe.cc index 8f0263cd9a..617a0129cf 100644 --- a/src/stream_pipe.cc +++ b/src/stream_pipe.cc @@ -17,7 +17,7 @@ StreamPipe::StreamPipe(StreamBase* source, StreamBase* sink, Local<Object> obj) : AsyncWrap(source->stream_env(), obj, AsyncWrap::PROVIDER_STREAMPIPE) { - MakeWeak(this); + MakeWeak(); CHECK_NE(sink, nullptr); CHECK_NE(source, nullptr); diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index 6158a1c4a4..3ccd157159 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -26,7 +26,6 @@ #include "handle_wrap.h" #include "node_buffer.h" #include "node_internals.h" -#include "node_wrap.h" #include "connect_wrap.h" #include "stream_base-inl.h" #include "stream_wrap.h" @@ -117,12 +116,8 @@ void TCPWrap::Initialize(Local<Object> target, env->set_tcp_constructor_template(t); // Create FunctionTemplate for TCPConnectWrap. - auto constructor = [](const FunctionCallbackInfo<Value>& args) { - CHECK(args.IsConstructCall()); - ClearWrap(args.This()); - }; - auto cwt = FunctionTemplate::New(env->isolate(), constructor); - cwt->InstanceTemplate()->SetInternalFieldCount(1); + Local<FunctionTemplate> cwt = + BaseObject::MakeLazilyInitializedJSTemplate(env); AsyncWrap::AddWrapMethods(env, cwt); Local<String> wrapString = FIXED_ONE_BYTE_STRING(env->isolate(), "TCPConnectWrap"); diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc index 02c0b81669..88377a3e1b 100644 --- a/src/timer_wrap.cc +++ b/src/timer_wrap.cc @@ -115,7 +115,8 @@ class TimerWrap : public HandleWrap { } static void Start(const FunctionCallbackInfo<Value>& args) { - TimerWrap* wrap = Unwrap<TimerWrap>(args.Holder()); + TimerWrap* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); CHECK(HandleWrap::IsAlive(wrap)); @@ -125,7 +126,8 @@ class TimerWrap : public HandleWrap { } static void Stop(const FunctionCallbackInfo<Value>& args) { - TimerWrap* wrap = Unwrap<TimerWrap>(args.Holder()); + TimerWrap* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); CHECK(HandleWrap::IsAlive(wrap)); diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 71adbdfc52..3764e2f3c9 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -68,8 +68,7 @@ TLSWrap::TLSWrap(Environment* env, shutdown_(false), cycle_depth_(0), eof_(false) { - node::Wrap(object(), this); - MakeWeak(this); + MakeWeak(); // sc comes from an Unwrap. Make sure it was assigned. CHECK_NE(sc, nullptr); @@ -873,16 +872,9 @@ void TLSWrap::Initialize(Local<Object> target, env->SetMethod(target, "wrap", TLSWrap::Wrap); - auto constructor = [](const FunctionCallbackInfo<Value>& args) { - CHECK(args.IsConstructCall()); - args.This()->SetAlignedPointerInInternalField(0, nullptr); - }; - + Local<FunctionTemplate> t = BaseObject::MakeLazilyInitializedJSTemplate(env); Local<String> tlsWrapString = FIXED_ONE_BYTE_STRING(env->isolate(), "TLSWrap"); - - auto t = env->NewFunctionTemplate(constructor); - t->InstanceTemplate()->SetInternalFieldCount(1); t->SetClassName(tlsWrapString); Local<FunctionTemplate> get_write_queue_size = diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index e02220a878..414fe07eab 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -53,7 +53,6 @@ using AsyncHooks = Environment::AsyncHooks; class SendWrap : public ReqWrap<uv_udp_send_t> { public: SendWrap(Environment* env, Local<Object> req_wrap_obj, bool have_callback); - ~SendWrap(); inline bool have_callback() const; size_t msg_size; size_t self_size() const override { return sizeof(*this); } @@ -67,12 +66,6 @@ SendWrap::SendWrap(Environment* env, bool have_callback) : ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_UDPSENDWRAP), have_callback_(have_callback) { - Wrap(req_wrap_obj, this); -} - - -SendWrap::~SendWrap() { - ClearWrap(object()); } @@ -81,12 +74,6 @@ inline bool SendWrap::have_callback() const { } -static void NewSendWrap(const FunctionCallbackInfo<Value>& args) { - CHECK(args.IsConstructCall()); - ClearWrap(args.This()); -} - - UDPWrap::UDPWrap(Environment* env, Local<Object> object) : HandleWrap(env, object, @@ -153,8 +140,7 @@ void UDPWrap::Initialize(Local<Object> target, // Create FunctionTemplate for SendWrap Local<FunctionTemplate> swt = - FunctionTemplate::New(env->isolate(), NewSendWrap); - swt->InstanceTemplate()->SetInternalFieldCount(1); + BaseObject::MakeLazilyInitializedJSTemplate(env); AsyncWrap::AddWrapMethods(env, swt); Local<String> sendWrapString = FIXED_ONE_BYTE_STRING(env->isolate(), "SendWrap"); diff --git a/src/util-inl.h b/src/util-inl.h index d07cfea922..41a22c97ef 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -217,25 +217,6 @@ inline v8::Local<v8::String> OneByteString(v8::Isolate* isolate, length).ToLocalChecked(); } -template <typename TypeName> -void Wrap(v8::Local<v8::Object> object, TypeName* pointer) { - CHECK_EQ(false, object.IsEmpty()); - CHECK_GT(object->InternalFieldCount(), 0); - object->SetAlignedPointerInInternalField(0, pointer); -} - -void ClearWrap(v8::Local<v8::Object> object) { - Wrap<void>(object, nullptr); -} - -template <typename TypeName> -TypeName* Unwrap(v8::Local<v8::Object> object) { - CHECK_EQ(false, object.IsEmpty()); - CHECK_GT(object->InternalFieldCount(), 0); - void* pointer = object->GetAlignedPointerFromInternalField(0); - return static_cast<TypeName*>(pointer); -} - void SwapBytes16(char* data, size_t nbytes) { CHECK_EQ(nbytes % 2, 0); diff --git a/src/util.h b/src/util.h index c8bad8171e..133899008f 100644 --- a/src/util.h +++ b/src/util.h @@ -35,7 +35,6 @@ #include <string.h> #include <functional> // std::function -#include <type_traits> // std::remove_reference namespace node { @@ -84,8 +83,6 @@ NO_RETURN void Abort(); NO_RETURN void Assert(const char* const (*args)[4]); void DumpBacktrace(FILE* fp); -template <typename T> using remove_reference = std::remove_reference<T>; - #define FIXED_ONE_BYTE_STRING(isolate, string) \ (node::OneByteString((isolate), (string), sizeof(string) - 1)) @@ -135,14 +132,6 @@ template <typename T> using remove_reference = std::remove_reference<T>; #define UNREACHABLE() ABORT() -#define ASSIGN_OR_RETURN_UNWRAP(ptr, obj, ...) \ - do { \ - *ptr = \ - Unwrap<typename node::remove_reference<decltype(**ptr)>::type>(obj); \ - if (*ptr == nullptr) \ - return __VA_ARGS__; \ - } while (0) - // TAILQ-style intrusive list node. template <typename T> class ListNode; @@ -250,13 +239,6 @@ inline v8::Local<v8::String> OneByteString(v8::Isolate* isolate, const unsigned char* data, int length = -1); -inline void Wrap(v8::Local<v8::Object> object, void* pointer); - -inline void ClearWrap(v8::Local<v8::Object> object); - -template <typename TypeName> -inline TypeName* Unwrap(v8::Local<v8::Object> object); - // Swaps bytes in place. nbytes is the number of bytes to swap and must be a // multiple of the word size (checked by function). inline void SwapBytes16(char* data, size_t nbytes); diff --git a/test/cctest/test_node_postmortem_metadata.cc b/test/cctest/test_node_postmortem_metadata.cc index 335f3e8581..f69df3ed22 100644 --- a/test/cctest/test_node_postmortem_metadata.cc +++ b/test/cctest/test_node_postmortem_metadata.cc @@ -72,7 +72,11 @@ TEST_F(DebugSymbolsTest, BaseObjectPersistentHandle) { const Argv argv; Env env{handle_scope, argv}; - v8::Local<v8::Object> object = v8::Object::New(isolate_); + v8::Local<v8::ObjectTemplate> obj_templ = v8::ObjectTemplate::New(isolate_); + obj_templ->SetInternalFieldCount(1); + + v8::Local<v8::Object> object = + obj_templ->NewInstance(env.context()).ToLocalChecked(); node::BaseObject obj(*env, object); auto expected = reinterpret_cast<uintptr_t>(&obj.persistent()); |