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
  ddc19be6de1ba263d9c175b2760696e7b9918b25) 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 ddc19be6de1ba263d9c175b2760696e7b9918b25, I do not
think that this is going to have any impact on diagnostic tooling.

Fixes: https://github.com/nodejs/node/issues/18897
PR-URL: https://github.com/nodejs/node/pull/20455
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

1 files changed, 4 insertions, 27 deletions

diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc
index 0cc7ed1464f..4df47d75d43 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");