src: fix ObjectWrap inheritance

- fix wrap/unwrap of objects inheriting from non-ObjectWrap PR-URL: #732 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Nicola Del Gobbo <[email protected]>
nodejs · Jun 8, 2020 · ba7ad37 · ba7ad37
1 parent 31504c8
commit ba7ad37
Show file tree

Hide file tree

Showing 6 changed files with 53 additions and 3 deletions.
diff --git a/napi-inl.h b/napi-inl.h
@@ -3146,10 +3146,11 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
   napi_value wrapper = callbackInfo.This();
   napi_status status;
   napi_ref ref;
-  status = napi_wrap(env, wrapper, this, FinalizeCallback, nullptr, &ref);
+  T* instance = static_cast<T*>(this);
+  status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref);
   NAPI_THROW_IF_FAILED_VOID(env, status);
 
-  Reference<Object>* instanceRef = this;
+  Reference<Object>* instanceRef = instance;
   *instanceRef = Reference<Object>(env, ref);
 }
 
@@ -3872,7 +3873,7 @@ inline napi_value ObjectWrap<T>::InstanceSetterCallbackWrapper(
 
 template <typename T>
 inline void ObjectWrap<T>::FinalizeCallback(napi_env env, void* data, void* /*hint*/) {
-  ObjectWrap<T>* instance = static_cast<ObjectWrap<T>*>(data);
+  T* instance = static_cast<T*>(data);
   instance->Finalize(Napi::Env(env));
   delete instance;
 }

diff --git a/test/binding.cc b/test/binding.cc
@@ -53,6 +53,7 @@ Object InitTypedArray(Env env);
 Object InitObjectWrap(Env env);
 Object InitObjectWrapConstructorException(Env env);
 Object InitObjectWrapRemoveWrap(Env env);
+Object InitObjectWrapMultipleInheritance(Env env);
 Object InitObjectReference(Env env);
 Object InitVersionManagement(Env env);
 Object InitThunkingManual(Env env);
@@ -111,6 +112,7 @@ Object Init(Env env, Object exports) {
   exports.Set("objectwrapConstructorException",
       InitObjectWrapConstructorException(env));
   exports.Set("objectwrap_removewrap", InitObjectWrapRemoveWrap(env));
+  exports.Set("objectwrap_multiple_inheritance", InitObjectWrapMultipleInheritance(env));
   exports.Set("objectreference", InitObjectReference(env));
   exports.Set("version_management", InitVersionManagement(env));
   exports.Set("thunking_manual", InitThunkingManual(env));

diff --git a/test/binding.gyp b/test/binding.gyp
@@ -45,6 +45,7 @@
         'objectwrap.cc',
         'objectwrap_constructor_exception.cc',
         'objectwrap-removewrap.cc',
+        'objectwrap_multiple_inheritance.cc',
         'objectreference.cc',
         'version_management.cc',
         'thunking_manual.cc',

diff --git a/test/index.js b/test/index.js
@@ -53,6 +53,7 @@ let testModules = [
   'objectwrap',
   'objectwrap_constructor_exception',
   'objectwrap-removewrap',
+  'objectwrap_multiple_inheritance',
   'objectreference',
   'version_management'
 ];

diff --git a/test/objectwrap_multiple_inheritance.cc b/test/objectwrap_multiple_inheritance.cc
@@ -0,0 +1,30 @@
+#include <napi.h>
+
+class TestMIBase {
+public:
+  TestMIBase() : test(0) {}
+  virtual void dummy() {}
+  uint32_t test;
+};
+
+class TestMI : public TestMIBase, public Napi::ObjectWrap<TestMI> {
+public:
+  TestMI(const Napi::CallbackInfo& info) :
+    Napi::ObjectWrap<TestMI>(info) {}
+
+  Napi::Value GetTest(const Napi::CallbackInfo& info) {
+    return Napi::Number::New(info.Env(), test);
+  }
+
+  static void Initialize(Napi::Env env, Napi::Object exports) {
+    exports.Set("TestMI", DefineClass(env, "TestMI", {
+      InstanceAccessor<&TestMI::GetTest>("test")
+    }));
+  }
+};
+
+Napi::Object InitObjectWrapMultipleInheritance(Napi::Env env) {
+  Napi::Object exports = Napi::Object::New(env);
+  TestMI::Initialize(env, exports);
+  return exports;
+}
diff --git a/test/objectwrap_multiple_inheritance.js b/test/objectwrap_multiple_inheritance.js
@@ -0,0 +1,15 @@
+'use strict';
+
+const buildType = process.config.target_defaults.default_configuration;
+const assert = require('assert');
+
+const test = bindingName => {
+  const binding = require(bindingName);
+  const TestMI = binding.objectwrap_multiple_inheritance.TestMI;
+  const testmi = new TestMI();
+
+  assert.strictEqual(testmi.test, 0);
+}
+
+test(`./build/${buildType}/binding.node`);
+test(`./build/${buildType}/binding_noexcept.node`);