src: refactor BaseObject internal field management

- Instead of storing a pointer whose type refers to the specific
  subclass of `BaseObject`, just store a `BaseObject*` directly.
  This means in particular that one can cast to classes along
  the way of the inheritance chain without issues, and that
  `BaseObject*` no longer needs to be the first superclass
  in the case of multiple inheritance.

  In particular, this renders hack-y solutions to this problem (like
  ddc19be) obsolete and addresses
  a `TODO` comment of mine.

- Move wrapping/unwrapping methods to the `BaseObject` class.
  We use these almost exclusively for `BaseObject`s, and I hope
  that this gives a better idea of how (and for what) these are used
  in our code.

- Perform initialization/deinitialization of the internal field
  in the `BaseObject*` constructor/destructor. This makes the code
  a bit more obviously correct, avoids explicit calls for this
  in subclass constructors, and in particular allows us to avoid
  crash situations when we previously called `ClearWrap()`
  during GC.

  This also means that we enforce that the object passed to the
  `BaseObject` constructor needs to have an internal field.
  This is the only reason for the test change.

- Change the signature of `MakeWeak()` to not require a pointer
  argument. Previously, this would always have been the same
  as `this`, and no other value made sense. Also, the parameter
  was something that I personally found somewhat confusing
  when becoming familiar with Node’s code.

- Add a `TODO` comment that motivates switching to real inheritance
  for the JS types we expose from the native side. This patch
  brings us a lot closer to being able to do that.

- Some less significant drive-by cleanup.

Since we *effectively* already store the `BaseObject*` pointer
anyway since ddc19be, I do not
think that this is going to have any impact on diagnostic tooling.

Fixes: #18897
PR-URL: #20455
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Author: addaleax

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); }
 

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

src/base_object.h

@@ -26,15 +26,18 @@
 
 #include "node_persistent.h"
 #include "v8.h"
+#include <type_traits>  // std::remove_reference
 
 namespace node {
 
 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

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,30 +218,19 @@ 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); }
 };
 
 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,32 +2128,24 @@ 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");
   aiw->SetClassName(addrInfoWrapString);
   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");
   niw->SetClassName(nameInfoWrapString);
   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");

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

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); }
 };

src/connection_wrap.h

@@ -29,7 +29,6 @@ class ConnectionWrap : public LibuvStreamWrap {
   UVType handle_;
 };
 
-
 }  // namespace node
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

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_);
 

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;
 }
 

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;
   }