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

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