From 698700d67a8c12cf753806f6127a92c1c45dfcd6 Mon Sep 17 00:00:00 2001 From: Roy Wright Date: Wed, 26 Sep 2018 09:29:22 -0400 Subject: [PATCH] src: remove TODOs by fixing memory leaks PR-URL: https://github.com/nodejs/node-addon-api/pull/343 Fixes: https://github.com/nodejs/node-addon-api/issues/333 Reviewed-By: Michael Dawson --- README.md | 6 + doc/property_descriptor.md | 114 ++++++++++-- doc/setup.md | 3 + napi-inl.deprecated.h | 192 ++++++++++++++++++++ napi-inl.h | 298 +++++++++++++++++++++---------- napi.h | 74 ++++++++ test/binding.cc | 8 + test/binding.gyp | 15 +- test/index.js | 1 + test/object/object.cc | 35 ++-- test/object/object_deprecated.cc | 66 +++++++ test/object/object_deprecated.js | 48 +++++ test/objectwrap.js | 2 +- test/thunking_manual.cc | 140 +++++++++++++++ test/thunking_manual.js | 18 ++ 15 files changed, 895 insertions(+), 125 deletions(-) create mode 100644 napi-inl.deprecated.h create mode 100644 test/object/object_deprecated.cc create mode 100644 test/object/object_deprecated.js create mode 100644 test/thunking_manual.cc create mode 100644 test/thunking_manual.js diff --git a/README.md b/README.md index 4f5be71..c27000a 100644 --- a/README.md +++ b/README.md @@ -133,6 +133,12 @@ npm install npm test ``` +To avoid testing the deprecated portions of the API run +``` +npm install +npm test --disable-deprecated +``` + Take a look and get inspired by our **[test suite](https://github.com/nodejs/node-addon-api/tree/master/test)** diff --git a/doc/property_descriptor.md b/doc/property_descriptor.md index 2c0fa6f..82a8719 100644 --- a/doc/property_descriptor.md +++ b/doc/property_descriptor.md @@ -22,19 +22,31 @@ Value TestFunction(const CallbackInfo& info) { } Void Init(Env env) { - // Accessor - PropertyDescriptor pd1 = PropertyDescriptor::Accessor("pd1", TestGetter); - PropertyDescriptor pd2 = PropertyDescriptor::Accessor("pd2", TestGetter, TestSetter); - - // Function - PropertyDescriptor pd3 = PropertyDescriptor::Function("function", TestFunction); - - // Value - Boolean true_bool = Boolean::New(env, true); - PropertyDescriptor pd4 = PropertyDescriptor::Value("boolean value", TestFunction, napi_writable); - - // Assign to an Object + // Create an object. Object obj = Object::New(env); + + // Accessor + PropertyDescriptor pd1 = PropertyDescriptor::Accessor(env, + obj, + "pd1", + TestGetter); + PropertyDescriptor pd2 = PropertyDescriptor::Accessor(env, + obj, + "pd2", + TestGetter, + TestSetter); + // Function + PropertyDescriptor pd3 = PropertyDescriptor::Function(env, + "function", + TestFunction); + // Value + Boolean true_bool = Boolean::New(env, true); + PropertyDescriptor pd4 = + PropertyDescriptor::Value("boolean value", + Napi::Boolean::New(env, true), + napi_writable); + + // Assign properties to the object. obj.DefineProperties({pd1, pd2, pd3, pd4}); } ``` @@ -71,6 +83,32 @@ The name of the property can be any of the following types: - `napi_value value` - `Napi::Name` +**This signature is deprecated. It will result in a memory leak if used.** + +```cpp +static Napi::PropertyDescriptor Napi::PropertyDescriptor::Accessor ( + Napi::Env env, + Napi::Object object, + ___ name, + Getter getter, + napi_property_attributes attributes = napi_default, + void *data = nullptr); +``` + +* `[in] env`: The environemnt in which to create this accessor. +* `[in] object`: The object on which the accessor will be defined. +* `[in] name`: The name used for the getter function. +* `[in] getter`: A getter function. +* `[in] attributes`: Potential attributes for the getter function. +* `[in] data`: A pointer to data of any type, default is a null pointer. + +Returns a `Napi::PropertyDescriptor` that contains a `Getter` accessor. + +The name of the property can be any of the following types: +- `const char*` +- `const std::string &` +- `Napi::Name` + ```cpp static Napi::PropertyDescriptor Napi::PropertyDescriptor::Accessor (___ name, Getter getter, @@ -93,6 +131,34 @@ The name of the property can be any of the following types: - `napi_value value` - `Napi::Name` +**This signature is deprecated. It will result in a memory leak if used.** + +```cpp +static Napi::PropertyDescriptor Napi::PropertyDescriptor::Accessor ( + Napi::Env env, + Napi::Object object, + ___ name, + Getter getter, + Setter setter, + napi_property_attributes attributes = napi_default, + void *data = nullptr); +``` + +* `[in] env`: The environemnt in which to create this accessor. +* `[in] object`: The object on which the accessor will be defined. +* `[in] name`: The name of the getter and setter function. +* `[in] getter`: The getter function. +* `[in] setter`: The setter function. +* `[in] attributes`: Potential attributes for the getter function. +* `[in] data`: A pointer to data of any type, default is a null pointer. + +Returns a `Napi::PropertyDescriptor` that contains a `Getter` and `Setter` function. + +The name of the property can be any of the following types: +- `const char*` +- `const std::string &` +- `Napi::Name` + ### Function ```cpp @@ -115,6 +181,30 @@ The name of the property can be any of the following types: - `napi_value value` - `Napi::Name` +**This signature is deprecated. It will result in a memory leak if used.** + +```cpp +static Napi::PropertyDescriptor Napi::PropertyDescriptor::Function ( + Napi::Env env, + ___ name, + Callable cb, + napi_property_attributes attributes = napi_default, + void *data = nullptr); +``` + +* `[in] env`: The environemnt in which to create this accessor. +* `[in] name`: The name of the Callable function. +* `[in] cb`: The function +* `[in] attributes`: Potential attributes for the getter function. +* `[in] data`: A pointer to data of any type, default is a null pointer. + +Returns a `Napi::PropertyDescriptor` that contains a callable `Napi::Function`. + +The name of the property can be any of the following types: +- `const char*` +- `const std::string &` +- `Napi::Name` + ### Value ```cpp diff --git a/doc/setup.md b/doc/setup.md index 542729a..36e6fc9 100644 --- a/doc/setup.md +++ b/doc/setup.md @@ -66,3 +66,6 @@ To use **N-API** in a native module: At build time, the N-API back-compat library code will be used only when the targeted node version *does not* have N-API built-in. + +The preprocessor directive `NODE_ADDON_API_DISABLE_DEPRECATED` can be defined at +compile time before including `napi.h` to skip the definition of deprecated APIs. diff --git a/napi-inl.deprecated.h b/napi-inl.deprecated.h new file mode 100644 index 0000000..d001743 --- /dev/null +++ b/napi-inl.deprecated.h @@ -0,0 +1,192 @@ +#ifndef SRC_NAPI_INL_DEPRECATED_H_ +#define SRC_NAPI_INL_DEPRECATED_H_ + +//////////////////////////////////////////////////////////////////////////////// +// PropertyDescriptor class +//////////////////////////////////////////////////////////////////////////////// + +template +inline PropertyDescriptor +PropertyDescriptor::Accessor(const char* utf8name, + Getter getter, + napi_property_attributes attributes, + void* /*data*/) { + typedef details::CallbackData CbData; + // TODO: Delete when the function is destroyed + auto callbackData = new CbData({ getter, nullptr }); + + return PropertyDescriptor({ + utf8name, + nullptr, + nullptr, + CbData::Wrapper, + nullptr, + nullptr, + attributes, + callbackData + }); +} + +template +inline PropertyDescriptor PropertyDescriptor::Accessor(const std::string& utf8name, + Getter getter, + napi_property_attributes attributes, + void* data) { + return Accessor(utf8name.c_str(), getter, attributes, data); +} + +template +inline PropertyDescriptor PropertyDescriptor::Accessor(napi_value name, + Getter getter, + napi_property_attributes attributes, + void* /*data*/) { + typedef details::CallbackData CbData; + // TODO: Delete when the function is destroyed + auto callbackData = new CbData({ getter, nullptr }); + + return PropertyDescriptor({ + nullptr, + name, + nullptr, + CbData::Wrapper, + nullptr, + nullptr, + attributes, + callbackData + }); +} + +template +inline PropertyDescriptor PropertyDescriptor::Accessor(Name name, + Getter getter, + napi_property_attributes attributes, + void* data) { + napi_value nameValue = name; + return PropertyDescriptor::Accessor(nameValue, getter, attributes, data); +} + +template +inline PropertyDescriptor PropertyDescriptor::Accessor(const char* utf8name, + Getter getter, + Setter setter, + napi_property_attributes attributes, + void* /*data*/) { + typedef details::AccessorCallbackData CbData; + // TODO: Delete when the function is destroyed + auto callbackData = new CbData({ getter, setter }); + + return PropertyDescriptor({ + utf8name, + nullptr, + nullptr, + CbData::GetterWrapper, + CbData::SetterWrapper, + nullptr, + attributes, + callbackData + }); +} + +template +inline PropertyDescriptor PropertyDescriptor::Accessor(const std::string& utf8name, + Getter getter, + Setter setter, + napi_property_attributes attributes, + void* data) { + return Accessor(utf8name.c_str(), getter, setter, attributes, data); +} + +template +inline PropertyDescriptor PropertyDescriptor::Accessor(napi_value name, + Getter getter, + Setter setter, + napi_property_attributes attributes, + void* /*data*/) { + typedef details::AccessorCallbackData CbData; + // TODO: Delete when the function is destroyed + auto callbackData = new CbData({ getter, setter }); + + return PropertyDescriptor({ + nullptr, + name, + nullptr, + CbData::GetterWrapper, + CbData::SetterWrapper, + nullptr, + attributes, + callbackData + }); +} + +template +inline PropertyDescriptor PropertyDescriptor::Accessor(Name name, + Getter getter, + Setter setter, + napi_property_attributes attributes, + void* data) { + napi_value nameValue = name; + return PropertyDescriptor::Accessor(nameValue, getter, setter, attributes, data); +} + +template +inline PropertyDescriptor PropertyDescriptor::Function(const char* utf8name, + Callable cb, + napi_property_attributes attributes, + void* /*data*/) { + typedef decltype(cb(CallbackInfo(nullptr, nullptr))) ReturnType; + typedef details::CallbackData CbData; + // TODO: Delete when the function is destroyed + auto callbackData = new CbData({ cb, nullptr }); + + return PropertyDescriptor({ + utf8name, + nullptr, + CbData::Wrapper, + nullptr, + nullptr, + nullptr, + attributes, + callbackData + }); +} + +template +inline PropertyDescriptor PropertyDescriptor::Function(const std::string& utf8name, + Callable cb, + napi_property_attributes attributes, + void* data) { + return Function(utf8name.c_str(), cb, attributes, data); +} + +template +inline PropertyDescriptor PropertyDescriptor::Function(napi_value name, + Callable cb, + napi_property_attributes attributes, + void* /*data*/) { + typedef decltype(cb(CallbackInfo(nullptr, nullptr))) ReturnType; + typedef details::CallbackData CbData; + // TODO: Delete when the function is destroyed + auto callbackData = new CbData({ cb, nullptr }); + + return PropertyDescriptor({ + nullptr, + name, + CbData::Wrapper, + nullptr, + nullptr, + nullptr, + attributes, + callbackData + }); +} + +template +inline PropertyDescriptor PropertyDescriptor::Function(Name name, + Callable cb, + napi_property_attributes attributes, + void* data) { + napi_value nameValue = name; + return PropertyDescriptor::Function(nameValue, cb, attributes, data); +} + +#endif // !SRC_NAPI_INL_DEPRECATED_H_ diff --git a/napi-inl.h b/napi-inl.h index aead8b9..6d8a021 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -60,6 +60,41 @@ namespace details { } \ } while (0) +// Attach a data item to an object and delete it when the object gets +// garbage-collected. +// TODO: Replace this code with `napi_add_finalizer()` whenever it becomes +// available on all supported versions of Node.js. +template +static inline napi_status AttachData(napi_env env, + napi_value obj, + FreeType* data) { + napi_value symbol, external; + napi_status status = napi_create_symbol(env, nullptr, &symbol); + if (status == napi_ok) { + status = napi_create_external(env, + data, + [](napi_env /*env*/, void* data, void* /*hint*/) { + delete static_cast(data); + }, + nullptr, + &external); + if (status == napi_ok) { + napi_property_descriptor desc = { + nullptr, + symbol, + nullptr, + nullptr, + nullptr, + external, + napi_default, + nullptr + }; + status = napi_define_properties(env, obj, 1, &desc); + } + } + return status; +} + // For use in JS to C++ callback wrappers to catch any Napi::Error exceptions // and rethrow them as JavaScript exceptions before returning from the callback. template @@ -162,6 +197,10 @@ struct AccessorCallbackData { } // namespace details +#ifndef NODE_ADDON_API_DISABLE_DEPRECATED +# include "napi-inl.deprecated.h" +#endif // !NODE_ADDON_API_DISABLE_DEPRECATED + //////////////////////////////////////////////////////////////////////////////// // Module registration //////////////////////////////////////////////////////////////////////////////// @@ -1587,6 +1626,22 @@ inline const T* TypedArrayOf::Data() const { // Function class //////////////////////////////////////////////////////////////////////////////// +template +static inline napi_status +CreateFunction(napi_env env, + const char* utf8name, + napi_callback cb, + CbData* data, + napi_value* result) { + napi_status status = + napi_create_function(env, utf8name, NAPI_AUTO_LENGTH, cb, data, result); + if (status == napi_ok) { + status = Napi::details::AttachData(env, *result, data); + } + + return status; +} + template inline Function Function::New(napi_env env, Callable cb, @@ -1594,12 +1649,14 @@ inline Function Function::New(napi_env env, void* data) { typedef decltype(cb(CallbackInfo(nullptr, nullptr))) ReturnType; typedef details::CallbackData CbData; - // TODO: Delete when the function is destroyed auto callbackData = new CbData({ cb, data }); napi_value value; - napi_status status = napi_create_function( - env, utf8name, NAPI_AUTO_LENGTH, CbData::Wrapper, callbackData, &value); + napi_status status = CreateFunction(env, + utf8name, + CbData::Wrapper, + callbackData, + &value); NAPI_THROW_IF_FAILED(env, status, Function()); return Function(env, value); } @@ -2541,14 +2598,18 @@ inline void CallbackInfo::SetData(void* data) { template inline PropertyDescriptor -PropertyDescriptor::Accessor(const char* utf8name, +PropertyDescriptor::Accessor(Napi::Env env, + Napi::Object object, + const char* utf8name, Getter getter, napi_property_attributes attributes, void* /*data*/) { typedef details::CallbackData CbData; - // TODO: Delete when the function is destroyed auto callbackData = new CbData({ getter, nullptr }); + napi_status status = AttachData(env, object, callbackData); + NAPI_THROW_IF_FAILED(env, status, napi_property_descriptor()); + return PropertyDescriptor({ utf8name, nullptr, @@ -2562,22 +2623,28 @@ PropertyDescriptor::Accessor(const char* utf8name, } template -inline PropertyDescriptor PropertyDescriptor::Accessor(const std::string& utf8name, +inline PropertyDescriptor PropertyDescriptor::Accessor(Napi::Env env, + Napi::Object object, + const std::string& utf8name, Getter getter, napi_property_attributes attributes, void* data) { - return Accessor(utf8name.c_str(), getter, attributes, data); + return Accessor(env, object, utf8name.c_str(), getter, attributes, data); } template -inline PropertyDescriptor PropertyDescriptor::Accessor(napi_value name, +inline PropertyDescriptor PropertyDescriptor::Accessor(Napi::Env env, + Napi::Object object, + Name name, Getter getter, napi_property_attributes attributes, void* /*data*/) { typedef details::CallbackData CbData; - // TODO: Delete when the function is destroyed auto callbackData = new CbData({ getter, nullptr }); + napi_status status = AttachData(env, object, callbackData); + NAPI_THROW_IF_FAILED(env, status, napi_property_descriptor()); + return PropertyDescriptor({ nullptr, name, @@ -2590,25 +2657,20 @@ inline PropertyDescriptor PropertyDescriptor::Accessor(napi_value name, }); } -template -inline PropertyDescriptor PropertyDescriptor::Accessor(Name name, - Getter getter, - napi_property_attributes attributes, - void* data) { - napi_value nameValue = name; - return PropertyDescriptor::Accessor(nameValue, getter, attributes, data); -} - template -inline PropertyDescriptor PropertyDescriptor::Accessor(const char* utf8name, +inline PropertyDescriptor PropertyDescriptor::Accessor(Napi::Env env, + Napi::Object object, + const char* utf8name, Getter getter, Setter setter, napi_property_attributes attributes, void* /*data*/) { typedef details::AccessorCallbackData CbData; - // TODO: Delete when the function is destroyed auto callbackData = new CbData({ getter, setter }); + napi_status status = AttachData(env, object, callbackData); + NAPI_THROW_IF_FAILED(env, status, napi_property_descriptor()); + return PropertyDescriptor({ utf8name, nullptr, @@ -2622,24 +2684,30 @@ inline PropertyDescriptor PropertyDescriptor::Accessor(const char* utf8name, } template -inline PropertyDescriptor PropertyDescriptor::Accessor(const std::string& utf8name, +inline PropertyDescriptor PropertyDescriptor::Accessor(Napi::Env env, + Napi::Object object, + const std::string& utf8name, Getter getter, Setter setter, napi_property_attributes attributes, void* data) { - return Accessor(utf8name.c_str(), getter, setter, attributes, data); + return Accessor(env, object, utf8name.c_str(), getter, setter, attributes, data); } template -inline PropertyDescriptor PropertyDescriptor::Accessor(napi_value name, +inline PropertyDescriptor PropertyDescriptor::Accessor(Napi::Env env, + Napi::Object object, + Name name, Getter getter, Setter setter, napi_property_attributes attributes, void* /*data*/) { typedef details::AccessorCallbackData CbData; - // TODO: Delete when the function is destroyed auto callbackData = new CbData({ getter, setter }); + napi_status status = AttachData(env, object, callbackData); + NAPI_THROW_IF_FAILED(env, status, napi_property_descriptor()); + return PropertyDescriptor({ nullptr, name, @@ -2652,77 +2720,54 @@ inline PropertyDescriptor PropertyDescriptor::Accessor(napi_value name, }); } -template -inline PropertyDescriptor PropertyDescriptor::Accessor(Name name, - Getter getter, - Setter setter, - napi_property_attributes attributes, - void* data) { - napi_value nameValue = name; - return PropertyDescriptor::Accessor(nameValue, getter, setter, attributes, data); -} - template -inline PropertyDescriptor PropertyDescriptor::Function(const char* utf8name, +inline PropertyDescriptor PropertyDescriptor::Function(Napi::Env env, + Napi::Object /*object*/, + const char* utf8name, Callable cb, napi_property_attributes attributes, - void* /*data*/) { - typedef decltype(cb(CallbackInfo(nullptr, nullptr))) ReturnType; - typedef details::CallbackData CbData; - // TODO: Delete when the function is destroyed - auto callbackData = new CbData({ cb, nullptr }); - + void* data) { return PropertyDescriptor({ utf8name, nullptr, - CbData::Wrapper, nullptr, nullptr, nullptr, + Napi::Function::New(env, cb, utf8name, data), attributes, - callbackData + nullptr }); } template -inline PropertyDescriptor PropertyDescriptor::Function(const std::string& utf8name, +inline PropertyDescriptor PropertyDescriptor::Function(Napi::Env env, + Napi::Object object, + const std::string& utf8name, Callable cb, napi_property_attributes attributes, void* data) { - return Function(utf8name.c_str(), cb, attributes, data); + return Function(env, object, utf8name.c_str(), cb, attributes, data); } template -inline PropertyDescriptor PropertyDescriptor::Function(napi_value name, +inline PropertyDescriptor PropertyDescriptor::Function(Napi::Env env, + Napi::Object /*object*/, + Name name, Callable cb, napi_property_attributes attributes, - void* /*data*/) { - typedef decltype(cb(CallbackInfo(nullptr, nullptr))) ReturnType; - typedef details::CallbackData CbData; - // TODO: Delete when the function is destroyed - auto callbackData = new CbData({ cb, nullptr }); - + void* data) { return PropertyDescriptor({ nullptr, name, - CbData::Wrapper, nullptr, nullptr, nullptr, + Napi::Function::New(env, cb, nullptr, data), attributes, - callbackData + nullptr }); } -template -inline PropertyDescriptor PropertyDescriptor::Function(Name name, - Callable cb, - napi_property_attributes attributes, - void* data) { - napi_value nameValue = name; - return PropertyDescriptor::Function(nameValue, cb, attributes, data); -} - inline PropertyDescriptor PropertyDescriptor::Value(const char* utf8name, napi_value value, napi_property_attributes attributes) { @@ -2791,20 +2836,106 @@ inline T* ObjectWrap::Unwrap(Object wrapper) { return unwrapped; } +template +inline Function +ObjectWrap::DefineClass(Napi::Env env, + const char* utf8name, + const size_t props_count, + const napi_property_descriptor* descriptors, + void* data) { + napi_status status; + std::vector props(props_count); + + // We copy the descriptors to a local array because before defining the class + // we must replace static method property descriptors with value property + // descriptors such that the value is a function-valued `napi_value` created + // with `CreateFunction()`. + // + // This replacement could be made for instance methods as well, but V8 aborts + // if we do that, because it expects methods defined on the prototype template + // to have `FunctionTemplate`s. + for (size_t index = 0; index < props_count; index++) { + props[index] = descriptors[index]; + napi_property_descriptor* prop = &props[index]; + if (prop->method == T::StaticMethodCallbackWrapper) { + status = CreateFunction(env, + utf8name, + prop->method, + static_cast(prop->data), + &(prop->value)); + NAPI_THROW_IF_FAILED(env, status, Function()); + prop->method = nullptr; + prop->data = nullptr; + } else if (prop->method == T::StaticVoidMethodCallbackWrapper) { + status = CreateFunction(env, + utf8name, + prop->method, + static_cast(prop->data), + &(prop->value)); + NAPI_THROW_IF_FAILED(env, status, Function()); + prop->method = nullptr; + prop->data = nullptr; + } + } + + napi_value value; + status = napi_define_class(env, + utf8name, + NAPI_AUTO_LENGTH, + T::ConstructorCallbackWrapper, + data, + props_count, + props.data(), + &value); + NAPI_THROW_IF_FAILED(env, status, Function()); + + // After defining the class we iterate once more over the property descriptors + // and attach the data associated with accessors and instance methods to the + // newly created JavaScript class. + for (size_t idx = 0; idx < props_count; idx++) { + const napi_property_descriptor* prop = &props[idx]; + + if (prop->getter == T::StaticGetterCallbackWrapper || + prop->setter == T::StaticSetterCallbackWrapper) { + status = Napi::details::AttachData(env, + value, + static_cast(prop->data)); + NAPI_THROW_IF_FAILED(env, status, Function()); + } else if (prop->getter == T::InstanceGetterCallbackWrapper || + prop->setter == T::InstanceSetterCallbackWrapper) { + status = Napi::details::AttachData(env, + value, + static_cast(prop->data)); + NAPI_THROW_IF_FAILED(env, status, Function()); + } else if (prop->method != nullptr && !(prop->attributes & napi_static)) { + if (prop->method == T::InstanceVoidMethodCallbackWrapper) { + status = Napi::details::AttachData(env, + value, + static_cast(prop->data)); + NAPI_THROW_IF_FAILED(env, status, Function()); + } else if (prop->method == T::InstanceMethodCallbackWrapper) { + status = Napi::details::AttachData(env, + value, + static_cast(prop->data)); + NAPI_THROW_IF_FAILED(env, status, Function()); + } + } + } + + return Function(env, value); +} + template inline Function ObjectWrap::DefineClass( Napi::Env env, const char* utf8name, const std::initializer_list>& properties, void* data) { - napi_value value; - napi_status status = napi_define_class( - env, utf8name, NAPI_AUTO_LENGTH, - T::ConstructorCallbackWrapper, data, properties.size(), - reinterpret_cast(properties.begin()), &value); - NAPI_THROW_IF_FAILED(env, status, Function()); - - return Function(env, value); + return DefineClass(env, + utf8name, + properties.size(), + reinterpret_cast(properties.begin()), + data); } template @@ -2813,14 +2944,11 @@ inline Function ObjectWrap::DefineClass( const char* utf8name, const std::vector>& properties, void* data) { - napi_value value; - napi_status status = napi_define_class( - env, utf8name, NAPI_AUTO_LENGTH, - T::ConstructorCallbackWrapper, data, properties.size(), - reinterpret_cast(properties.data()), &value); - NAPI_THROW_IF_FAILED(env, status, Function()); - - return Function(env, value); + return DefineClass(env, + utf8name, + properties.size(), + reinterpret_cast(properties.data()), + data); } template @@ -2829,7 +2957,6 @@ inline ClassPropertyDescriptor ObjectWrap::StaticMethod( StaticVoidMethodCallback method, napi_property_attributes attributes, void* data) { - // TODO: Delete when the class is destroyed StaticVoidMethodCallbackData* callbackData = new StaticVoidMethodCallbackData({ method, data }); napi_property_descriptor desc = napi_property_descriptor(); @@ -2846,7 +2973,6 @@ inline ClassPropertyDescriptor ObjectWrap::StaticMethod( StaticMethodCallback method, napi_property_attributes attributes, void* data) { - // TODO: Delete when the class is destroyed StaticMethodCallbackData* callbackData = new StaticMethodCallbackData({ method, data }); napi_property_descriptor desc = napi_property_descriptor(); @@ -2863,7 +2989,6 @@ inline ClassPropertyDescriptor ObjectWrap::StaticMethod( StaticVoidMethodCallback method, napi_property_attributes attributes, void* data) { - // TODO: Delete when the class is destroyed StaticVoidMethodCallbackData* callbackData = new StaticVoidMethodCallbackData({ method, data }); napi_property_descriptor desc = napi_property_descriptor(); @@ -2880,7 +3005,6 @@ inline ClassPropertyDescriptor ObjectWrap::StaticMethod( StaticMethodCallback method, napi_property_attributes attributes, void* data) { - // TODO: Delete when the class is destroyed StaticMethodCallbackData* callbackData = new StaticMethodCallbackData({ method, data }); napi_property_descriptor desc = napi_property_descriptor(); @@ -2898,7 +3022,6 @@ inline ClassPropertyDescriptor ObjectWrap::StaticAccessor( StaticSetterCallback setter, napi_property_attributes attributes, void* data) { - // TODO: Delete when the class is destroyed StaticAccessorCallbackData* callbackData = new StaticAccessorCallbackData({ getter, setter, data }); @@ -2918,7 +3041,6 @@ inline ClassPropertyDescriptor ObjectWrap::StaticAccessor( StaticSetterCallback setter, napi_property_attributes attributes, void* data) { - // TODO: Delete when the class is destroyed StaticAccessorCallbackData* callbackData = new StaticAccessorCallbackData({ getter, setter, data }); @@ -2937,7 +3059,6 @@ inline ClassPropertyDescriptor ObjectWrap::InstanceMethod( InstanceVoidMethodCallback method, napi_property_attributes attributes, void* data) { - // TODO: Delete when the class is destroyed InstanceVoidMethodCallbackData* callbackData = new InstanceVoidMethodCallbackData({ method, data}); @@ -2955,7 +3076,6 @@ inline ClassPropertyDescriptor ObjectWrap::InstanceMethod( InstanceMethodCallback method, napi_property_attributes attributes, void* data) { - // TODO: Delete when the class is destroyed InstanceMethodCallbackData* callbackData = new InstanceMethodCallbackData({ method, data }); napi_property_descriptor desc = napi_property_descriptor(); @@ -2972,7 +3092,6 @@ inline ClassPropertyDescriptor ObjectWrap::InstanceMethod( InstanceVoidMethodCallback method, napi_property_attributes attributes, void* data) { - // TODO: Delete when the class is destroyed InstanceVoidMethodCallbackData* callbackData = new InstanceVoidMethodCallbackData({ method, data}); @@ -2990,7 +3109,6 @@ inline ClassPropertyDescriptor ObjectWrap::InstanceMethod( InstanceMethodCallback method, napi_property_attributes attributes, void* data) { - // TODO: Delete when the class is destroyed InstanceMethodCallbackData* callbackData = new InstanceMethodCallbackData({ method, data }); napi_property_descriptor desc = napi_property_descriptor(); @@ -3008,7 +3126,6 @@ inline ClassPropertyDescriptor ObjectWrap::InstanceAccessor( InstanceSetterCallback setter, napi_property_attributes attributes, void* data) { - // TODO: Delete when the class is destroyed InstanceAccessorCallbackData* callbackData = new InstanceAccessorCallbackData({ getter, setter, data }); @@ -3028,7 +3145,6 @@ inline ClassPropertyDescriptor ObjectWrap::InstanceAccessor( InstanceSetterCallback setter, napi_property_attributes attributes, void* data) { - // TODO: Delete when the class is destroyed InstanceAccessorCallbackData* callbackData = new InstanceAccessorCallbackData({ getter, setter, data }); diff --git a/napi.h b/napi.h index ed86272..61df5fe 100644 --- a/napi.h +++ b/napi.h @@ -1312,6 +1312,7 @@ namespace Napi { class PropertyDescriptor { public: +#ifndef NODE_ADDON_API_DISABLE_DEPRECATED template static PropertyDescriptor Accessor(const char* utf8name, Getter getter, @@ -1376,6 +1377,74 @@ namespace Napi { Callable cb, napi_property_attributes attributes = napi_default, void* data = nullptr); +#endif // !NODE_ADDON_API_DISABLE_DEPRECATED + + template + static PropertyDescriptor Accessor(Napi::Env env, + Napi::Object object, + const char* utf8name, + Getter getter, + napi_property_attributes attributes = napi_default, + void* data = nullptr); + template + static PropertyDescriptor Accessor(Napi::Env env, + Napi::Object object, + const std::string& utf8name, + Getter getter, + napi_property_attributes attributes = napi_default, + void* data = nullptr); + template + static PropertyDescriptor Accessor(Napi::Env env, + Napi::Object object, + Name name, + Getter getter, + napi_property_attributes attributes = napi_default, + void* data = nullptr); + template + static PropertyDescriptor Accessor(Napi::Env env, + Napi::Object object, + const char* utf8name, + Getter getter, + Setter setter, + napi_property_attributes attributes = napi_default, + void* data = nullptr); + template + static PropertyDescriptor Accessor(Napi::Env env, + Napi::Object object, + const std::string& utf8name, + Getter getter, + Setter setter, + napi_property_attributes attributes = napi_default, + void* data = nullptr); + template + static PropertyDescriptor Accessor(Napi::Env env, + Napi::Object object, + Name name, + Getter getter, + Setter setter, + napi_property_attributes attributes = napi_default, + void* data = nullptr); + template + static PropertyDescriptor Function(Napi::Env env, + Napi::Object object, + const char* utf8name, + Callable cb, + napi_property_attributes attributes = napi_default, + void* data = nullptr); + template + static PropertyDescriptor Function(Napi::Env env, + Napi::Object object, + const std::string& utf8name, + Callable cb, + napi_property_attributes attributes = napi_default, + void* data = nullptr); + template + static PropertyDescriptor Function(Napi::Env env, + Napi::Object object, + Name name, + Callable cb, + napi_property_attributes attributes = napi_default, + void* data = nullptr); static PropertyDescriptor Value(const char* utf8name, napi_value value, napi_property_attributes attributes = napi_default); @@ -1543,6 +1612,11 @@ namespace Napi { static napi_value InstanceGetterCallbackWrapper(napi_env env, napi_callback_info info); static napi_value InstanceSetterCallbackWrapper(napi_env env, napi_callback_info info); static void FinalizeCallback(napi_env env, void* data, void* hint); + static Function DefineClass(Napi::Env env, + const char* utf8name, + const size_t props_count, + const napi_property_descriptor* props, + void* data = nullptr); template struct MethodCallbackData { diff --git a/test/binding.cc b/test/binding.cc index ffa3ec7..101ac1f 100644 --- a/test/binding.cc +++ b/test/binding.cc @@ -24,11 +24,15 @@ Object InitHandleScope(Env env); Object InitMemoryManagement(Env env); Object InitName(Env env); Object InitObject(Env env); +#ifndef NODE_ADDON_API_DISABLE_DEPRECATED +Object InitObjectDeprecated(Env env); +#endif // !NODE_ADDON_API_DISABLE_DEPRECATED Object InitPromise(Env env); Object InitTypedArray(Env env); Object InitObjectWrap(Env env); Object InitObjectReference(Env env); Object InitVersionManagement(Env env); +Object InitThunkingManual(Env env); Object Init(Env env, Object exports) { exports.Set("arraybuffer", InitArrayBuffer(env)); @@ -53,11 +57,15 @@ Object Init(Env env, Object exports) { exports.Set("handlescope", InitHandleScope(env)); exports.Set("memory_management", InitMemoryManagement(env)); exports.Set("object", InitObject(env)); +#ifndef NODE_ADDON_API_DISABLE_DEPRECATED + exports.Set("object_deprecated", InitObjectDeprecated(env)); +#endif // !NODE_ADDON_API_DISABLE_DEPRECATED exports.Set("promise", InitPromise(env)); exports.Set("typedarray", InitTypedArray(env)); exports.Set("objectwrap", InitObjectWrap(env)); exports.Set("objectreference", InitObjectReference(env)); exports.Set("version_management", InitVersionManagement(env)); + exports.Set("thunking_manual", InitThunkingManual(env)); return exports; } diff --git a/test/binding.gyp b/test/binding.gyp index 2eaccdb..417a2bb 100644 --- a/test/binding.gyp +++ b/test/binding.gyp @@ -1,6 +1,7 @@ { 'variables': { - 'NAPI_VERSION%': "" + 'NAPI_VERSION%': "", + 'disable_deprecated': "(); String nameType = info[1].As(); + Env env = info.Env(); - Boolean trueValue = Boolean::New(info.Env(), true); + Boolean trueValue = Boolean::New(env, true); if (nameType.Utf8Value() == "literal") { obj.DefineProperties({ - PropertyDescriptor::Accessor("readonlyAccessor", TestGetter), - PropertyDescriptor::Accessor("readwriteAccessor", TestGetter, TestSetter), + PropertyDescriptor::Accessor(env, obj, "readonlyAccessor", TestGetter), + PropertyDescriptor::Accessor(env, obj, "readwriteAccessor", TestGetter, TestSetter), PropertyDescriptor::Value("readonlyValue", trueValue), PropertyDescriptor::Value("readwriteValue", trueValue, napi_writable), PropertyDescriptor::Value("enumerableValue", trueValue, napi_enumerable), PropertyDescriptor::Value("configurableValue", trueValue, napi_configurable), - PropertyDescriptor::Function("function", TestFunction), + PropertyDescriptor::Function(env, obj, "function", TestFunction), }); } else if (nameType.Utf8Value() == "string") { // VS2013 has lifetime issues when passing temporary objects into the constructor of another @@ -82,30 +83,30 @@ void DefineProperties(const CallbackInfo& info) { std::string str7("function"); obj.DefineProperties({ - PropertyDescriptor::Accessor(str1, TestGetter), - PropertyDescriptor::Accessor(str2, TestGetter, TestSetter), + PropertyDescriptor::Accessor(env, obj, str1, TestGetter), + PropertyDescriptor::Accessor(env, obj, str2, TestGetter, TestSetter), PropertyDescriptor::Value(str3, trueValue), PropertyDescriptor::Value(str4, trueValue, napi_writable), PropertyDescriptor::Value(str5, trueValue, napi_enumerable), PropertyDescriptor::Value(str6, trueValue, napi_configurable), - PropertyDescriptor::Function(str7, TestFunction), + PropertyDescriptor::Function(env, obj, str7, TestFunction), }); } else if (nameType.Utf8Value() == "value") { obj.DefineProperties({ - PropertyDescriptor::Accessor( - Napi::String::New(info.Env(), "readonlyAccessor"), TestGetter), - PropertyDescriptor::Accessor( - Napi::String::New(info.Env(), "readwriteAccessor"), TestGetter, TestSetter), + PropertyDescriptor::Accessor(env, obj, + Napi::String::New(env, "readonlyAccessor"), TestGetter), + PropertyDescriptor::Accessor(env, obj, + Napi::String::New(env, "readwriteAccessor"), TestGetter, TestSetter), PropertyDescriptor::Value( - Napi::String::New(info.Env(), "readonlyValue"), trueValue), + Napi::String::New(env, "readonlyValue"), trueValue), PropertyDescriptor::Value( - Napi::String::New(info.Env(), "readwriteValue"), trueValue, napi_writable), + Napi::String::New(env, "readwriteValue"), trueValue, napi_writable), PropertyDescriptor::Value( - Napi::String::New(info.Env(), "enumerableValue"), trueValue, napi_enumerable), + Napi::String::New(env, "enumerableValue"), trueValue, napi_enumerable), PropertyDescriptor::Value( - Napi::String::New(info.Env(), "configurableValue"), trueValue, napi_configurable), - PropertyDescriptor::Function( - Napi::String::New(info.Env(), "function"), TestFunction), + Napi::String::New(env, "configurableValue"), trueValue, napi_configurable), + PropertyDescriptor::Function(env, obj, + Napi::String::New(env, "function"), TestFunction), }); } } diff --git a/test/object/object_deprecated.cc b/test/object/object_deprecated.cc new file mode 100644 index 0000000..2ec16e5 --- /dev/null +++ b/test/object/object_deprecated.cc @@ -0,0 +1,66 @@ +#include "napi.h" + +using namespace Napi; + +static bool testValue = true; + +namespace { + +Value TestGetter(const CallbackInfo& info) { + return Boolean::New(info.Env(), testValue); +} + +void TestSetter(const CallbackInfo& info) { + testValue = info[0].As(); +} + +Value TestFunction(const CallbackInfo& info) { + return Boolean::New(info.Env(), true); +} + +void DefineProperties(const CallbackInfo& info) { + Object obj = info[0].As(); + String nameType = info[1].As(); + Env env = info.Env(); + + if (nameType.Utf8Value() == "literal") { + obj.DefineProperties({ + PropertyDescriptor::Accessor("readonlyAccessor", TestGetter), + PropertyDescriptor::Accessor("readwriteAccessor", TestGetter, TestSetter), + PropertyDescriptor::Function("function", TestFunction), + }); + } else if (nameType.Utf8Value() == "string") { + // VS2013 has lifetime issues when passing temporary objects into the constructor of another + // object. It generates code to destruct the object as soon as the constructor call returns. + // Since this isn't a common case for using std::string objects, I'm refactoring the test to + // work around the issue. + std::string str1("readonlyAccessor"); + std::string str2("readwriteAccessor"); + std::string str7("function"); + + obj.DefineProperties({ + PropertyDescriptor::Accessor(str1, TestGetter), + PropertyDescriptor::Accessor(str2, TestGetter, TestSetter), + PropertyDescriptor::Function(str7, TestFunction), + }); + } else if (nameType.Utf8Value() == "value") { + obj.DefineProperties({ + PropertyDescriptor::Accessor( + Napi::String::New(env, "readonlyAccessor"), TestGetter), + PropertyDescriptor::Accessor( + Napi::String::New(env, "readwriteAccessor"), TestGetter, TestSetter), + PropertyDescriptor::Function( + Napi::String::New(env, "function"), TestFunction), + }); + } +} + +} // end of anonymous namespace + +Object InitObjectDeprecated(Env env) { + Object exports = Object::New(env); + + exports["defineProperties"] = Function::New(env, DefineProperties); + + return exports; +} diff --git a/test/object/object_deprecated.js b/test/object/object_deprecated.js new file mode 100644 index 0000000..153fb11 --- /dev/null +++ b/test/object/object_deprecated.js @@ -0,0 +1,48 @@ +'use strict'; +const buildType = process.config.target_defaults.default_configuration; +const assert = require('assert'); + +test(require(`../build/${buildType}/binding.node`)); +test(require(`../build/${buildType}/binding_noexcept.node`)); + +function test(binding) { + if (!('object_deprecated' in binding)) { + return; + } + function assertPropertyIs(obj, key, attribute) { + const propDesc = Object.getOwnPropertyDescriptor(obj, key); + assert.ok(propDesc); + assert.ok(propDesc[attribute]); + } + + function assertPropertyIsNot(obj, key, attribute) { + const propDesc = Object.getOwnPropertyDescriptor(obj, key); + assert.ok(propDesc); + assert.ok(!propDesc[attribute]); + } + + function testDefineProperties(nameType) { + const obj = {}; + binding.object.defineProperties(obj, nameType); + + assertPropertyIsNot(obj, 'readonlyAccessor', 'enumerable'); + assertPropertyIsNot(obj, 'readonlyAccessor', 'configurable'); + assert.strictEqual(obj.readonlyAccessor, true); + + assertPropertyIsNot(obj, 'readwriteAccessor', 'enumerable'); + assertPropertyIsNot(obj, 'readwriteAccessor', 'configurable'); + obj.readwriteAccessor = false; + assert.strictEqual(obj.readwriteAccessor, false); + obj.readwriteAccessor = true; + assert.strictEqual(obj.readwriteAccessor, true); + + assertPropertyIsNot(obj, 'function', 'writable'); + assertPropertyIsNot(obj, 'function', 'enumerable'); + assertPropertyIsNot(obj, 'function', 'configurable'); + assert.strictEqual(obj.function(), true); + } + + testDefineProperties('literal'); + testDefineProperties('string'); + testDefineProperties('value'); +} diff --git a/test/objectwrap.js b/test/objectwrap.js index 7280500..1f88823 100644 --- a/test/objectwrap.js +++ b/test/objectwrap.js @@ -207,4 +207,4 @@ const test = (binding) => { } test(require(`./build/${buildType}/binding.node`)); -test(require(`./build/${buildType}/binding_noexcept.node`)); \ No newline at end of file +test(require(`./build/${buildType}/binding_noexcept.node`)); diff --git a/test/thunking_manual.cc b/test/thunking_manual.cc new file mode 100644 index 0000000..d52302e --- /dev/null +++ b/test/thunking_manual.cc @@ -0,0 +1,140 @@ +#include + +// The formulaic comment below should accompany any code that results in an +// internal piece of heap data getting created, because each such piece of heap +// data must be attached to an object by way of a deleter which gets called when +// the object gets garbage-collected. +// +// At the very least, you can add a fprintf(stderr, ...) to the deleter in +// napi-inl.h and then count the number of times the deleter prints by running +// node --expose-gc test/thunking_manual.js and counting the number of prints +// between the two rows of dashes. That number should coincide with the number +// of formulaic comments below. +// +// Note that currently this result can only be achieved with node-chakracore, +// because V8 does not garbage-collect classes. + +static Napi::Value TestMethod(const Napi::CallbackInfo& /*info*/) { + return Napi::Value(); +} + +static Napi::Value TestGetter(const Napi::CallbackInfo& /*info*/) { + return Napi::Value(); +} + +static void TestSetter(const Napi::CallbackInfo& /*info*/) { +} + +class TestClass : public Napi::ObjectWrap { + public: + TestClass(const Napi::CallbackInfo& info): + ObjectWrap(info) { + } + static Napi::Value TestClassStaticMethod(const Napi::CallbackInfo& info) { + return Napi::Number::New(info.Env(), 42); + } + + static void TestClassStaticVoidMethod(const Napi::CallbackInfo& /*info*/) { + } + + Napi::Value TestClassInstanceMethod(const Napi::CallbackInfo& info) { + return Napi::Number::New(info.Env(), 42); + } + + void TestClassInstanceVoidMethod(const Napi::CallbackInfo& /*info*/) { + } + + Napi::Value TestClassInstanceGetter(const Napi::CallbackInfo& info) { + return Napi::Number::New(info.Env(), 42); + } + + void TestClassInstanceSetter(const Napi::CallbackInfo& /*info*/, + const Napi::Value& /*new_value*/) { + } + + static Napi::Function NewClass(Napi::Env env) { + return DefineClass(env, "TestClass", { + // Make sure to check that the deleter gets called. + StaticMethod("staticMethod", TestClassStaticMethod), + // Make sure to check that the deleter gets called. + StaticMethod("staticVoidMethod", TestClassStaticVoidMethod), + // Make sure to check that the deleter gets called. + StaticMethod(Napi::Symbol::New(env, "staticMethod"), + TestClassStaticMethod), + // Make sure to check that the deleter gets called. + StaticMethod(Napi::Symbol::New(env, "staticVoidMethod"), + TestClassStaticVoidMethod), + // Make sure to check that the deleter gets called. + InstanceMethod("instanceMethod", &TestClass::TestClassInstanceMethod), + // Make sure to check that the deleter gets called. + InstanceMethod("instanceVoidMethod", + &TestClass::TestClassInstanceVoidMethod), + // Make sure to check that the deleter gets called. + InstanceMethod(Napi::Symbol::New(env, "instanceMethod"), + &TestClass::TestClassInstanceMethod), + // Make sure to check that the deleter gets called. + InstanceMethod(Napi::Symbol::New(env, "instanceVoidMethod"), + &TestClass::TestClassInstanceVoidMethod), + // Make sure to check that the deleter gets called. + InstanceAccessor("instanceAccessor", + &TestClass::TestClassInstanceGetter, + &TestClass::TestClassInstanceSetter) + }); + } +}; + +static Napi::Value CreateTestObject(const Napi::CallbackInfo& info) { + Napi::Env env = info.Env(); + Napi::Object item = Napi::Object::New(env); + + // Make sure to check that the deleter gets called. + item["testMethod"] = Napi::Function::New(env, TestMethod, "testMethod"); + + item.DefineProperties({ + // Make sure to check that the deleter gets called. + Napi::PropertyDescriptor::Accessor(env, + item, + "accessor_1", + TestGetter), + // Make sure to check that the deleter gets called. + Napi::PropertyDescriptor::Accessor(env, + item, + std::string("accessor_1_std_string"), + TestGetter), + // Make sure to check that the deleter gets called. + Napi::PropertyDescriptor::Accessor(env, + item, + Napi::String::New(info.Env(), + "accessor_1_js_string"), + TestGetter), + // Make sure to check that the deleter gets called. + Napi::PropertyDescriptor::Accessor(env, + item, + "accessor_2", + TestGetter, + TestSetter), + // Make sure to check that the deleter gets called. + Napi::PropertyDescriptor::Accessor(env, + item, + std::string("accessor_2_std_string"), + TestGetter, + TestSetter), + // Make sure to check that the deleter gets called. + Napi::PropertyDescriptor::Accessor(env, + item, + Napi::String::New(env, + "accessor_2_js_string"), + TestGetter, + TestSetter), + Napi::PropertyDescriptor::Value("TestClass", TestClass::NewClass(env)), + }); + + return item; +} + +Napi::Object InitThunkingManual(Napi::Env env) { + Napi::Object exports = Napi::Object::New(env); + exports["createTestObject"] = + Napi::Function::New(env, CreateTestObject, "createTestObject"); + return exports; +} diff --git a/test/thunking_manual.js b/test/thunking_manual.js new file mode 100644 index 0000000..22fb887 --- /dev/null +++ b/test/thunking_manual.js @@ -0,0 +1,18 @@ +// Flags: --expose-gc +'use strict'; +const buildType = 'Debug'; +const assert = require('assert'); + +test(require(`./build/${buildType}/binding.node`)); +test(require(`./build/${buildType}/binding_noexcept.node`)); + +function test(binding) { + console.log("Thunking: Performing initial GC"); + global.gc(); + console.log("Thunking: Creating test object"); + let object = binding.thunking_manual.createTestObject(); + object = null; + console.log("Thunking: About to GC\n--------"); + global.gc(); + console.log("--------\nThunking: GC complete"); +}