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, 2 insertions, 12 deletions

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) {