Fatal error with ObjectWrap since 1.5.0

[BotellaA]

I update to node-addon-api 1.5.0 (from 1.4.0) and I got this error at runtime using gdb

#
# Fatal error in , line 0
# Check failed: !value_obj->IsJSReceiver() || value_obj->IsTemplateInfo().
#
#
#
#FailureMessage Object: 0x7fffffffb220
Thread 1 "node" received signal SIGILL, Illegal instruction.
0x00000000016abfa9 in v8::base::OS::Abort() ()

and this stack

#0 0x00000000016abfa9 in v8::base::OS::Abort() ()
#1 0x00000000016a8a94 in V8_Fatal(char const*, int, char const*, ...) ()
#2 0x0000000000b04353 in v8::Template::Set(v8::Local<v8::Name>, v8::Local<v8::Data>, v8::PropertyAttribute) ()
#3 0x00000000008eac00 in napi_define_class ()
#4 0x00007ffff406c218 in Napi::ObjectWrap<genepi::ClassWrapper<Test> >::DefineClass(Napi::Env, char const*, unsigned long, napi_property_descriptor const*, void*) ()
from /home/camaud/programming/genepi/build/Release/genepi.node
#5 0x00007ffff406e10c in genepi::BindClass<Test>::initialize(Napi::Env&, Napi::Object&) const () from /home/camaud/programming/genepi/build/Release/genepi.node
#6 0x00007ffff406f322 in initialize(Napi::Env, Napi::Object) () from /home/camaud/programming/genepi/build/Release/genepi.node
#7 0x00007ffff406f4ac in __napi_initialize(napi_env__*, napi_value__*) () from /home/camaud/programming/genepi/build/Release/genepi.node
#8 0x00000000008e14a1 in napi_module_register_by_symbol(v8::Local<v8::Object>, v8::Local<v8::Value>, v8::Local<v8::Context>, napi_value__* (*)(napi_env__*, napi_value__*)) ()
#9 0x00000000008d3352 in node::DLOpen(v8::FunctionCallbackInfo<v8::Value> const&) ()
#10 0x0000000000b8b32f in v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) ()
#11 0x0000000000b8be99 in v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) ()

Do you have an idea why this crashes in 1.5.0 and not in 1.4.0? I had the same crash on several computers with several gcc and node versions. Any major changes in Napi::ObjectWrap::DefineClass?

Thanks for your help!

[gabrielschulhof]

@BotellaA which precise versions of Node.js have you tried this with? We've had crashes caused by Node.js itself, because it was accessing a stale pointer as part of the execution of OnOK.

[BotellaA]

I tried 8.12, 10.11 and 10.12.

[mhdawson]

Changelog is here: https://github.com/nodejs/node-addon-api/blob/master/CHANGELOG.md

[mhdawson]

If we have them properly labelled then these should be the only candiates:

[917bd60] - src: remove TODOs by fixing memory leaks (Gabriel Schulhof) #343
[dfcb939] - src: implement AsyncContext class (Jinho Bang) #252
[211ed38] - src: make 'nothing' target a static library (Gabriel Schulhof) #348
[97c4ab5] - src: add Call and MakeCallback that accept cargs (NickNaso) #344
[b6e2d92] - src: enable DataView feature by default (Jinho) #331
[0a00e7c] - src: implement missing descriptor defs for symbols (Philipp Renoth) #280
[38e01b7] - src: first pass on adding version management apis (NickNaso) #325
[79ee838] - src: fix compile failure in test (Michael Dawson) #345
[4d92a60] - src: Add ObjectReference test case (Anisha Rohra) #212

[mhdawson]

Guessing that we can eliminate ones which add new functionality, which I think leaves this one unless we have some problems with the instance of a method with the wrong parameters being selected because there are additional options is a possibility(for example [97c4ab5] - src: add Call and MakeCallback that accept cargs (NickNaso) #344)

[917bd60] - src: remove TODOs by fixing memory leaks (Gabriel Schulhof) #343

[mhdawson]

I believe [917bd60] did affect the functionality for define class as well, so probably a good candidate.

[mhdawson]

@gabrielschulhof since you are already engaged maybe you can help @BotellaA revert that change and see if it is related?

[gabrielschulhof]

@BotellaA can you rebuild with --debug so I can see line numbers on the node-addon-api part of the stack? I suspect that a function-valued property gets added as an instance method. This is not allowed.

Also, can you point me to the source code, if it's open?

[BotellaA]

Here is the debug stack

#1  0x00000000016a8a94 in V8_Fatal(char const*, int, char const*, ...) ()
#2  0x0000000000b04353 in v8::Template::Set(v8::Local<v8::Name>, v8::Local<v8::Data>, v8::PropertyAttribute) ()
#3  0x00000000008eac00 in napi_define_class ()
#4  0x00007fffdfde0728 in Napi::ObjectWrap<genepi::ClassWrapper<Test> >::DefineClass (env=..., 
    utf8name=0x7fffdffff978 <genepi::BindClass<Test>::getInstance()::instance+24> "Test", props_count=4, descriptors=0x251d1f0, data=0x0)
    at /home/camaud/programming/genepi/node_modules/node-addon-api/napi-inl.h:2882
#5  0x00007fffdfddf2c2 in Napi::ObjectWrap<genepi::ClassWrapper<Test> >::DefineClass (env=..., 
    utf8name=0x7fffdffff978 <genepi::BindClass<Test>::getInstance()::instance+24> "Test", properties=std::vector of length 4, capacity 4 = {...}, data=0x0)
    at /home/camaud/programming/genepi/node_modules/node-addon-api/napi-inl.h:2951
#6  0x00007fffdfdde834 in genepi::ClassWrapper<Test>::Initialize (env=..., 
    target=..., name="Test", 
    static_methodList=std::deque with 1 element = {...}, 
    methodList=std::deque with 3 elements = {...})
    at /home/camaud/programming/genepi/include/genepi/class_wrapper.h:38
#7  0x00007fffdfdde406 in genepi::BindClass<Test>::initialize (
    this=0x7fffdffff960 <genepi::BindClass<Test>::getInstance()::instance>, 
    env=..., target=...)
    at /home/camaud/programming/genepi/include/genepi/bind_class.h:32
#8  0x00007fffdfde5f06 in initialize (env=..., exports=...)
    at /home/camaud/programming/genepi/src/genepi/genepi.cpp:3
#9  0x00007fffdfde623f in Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}::operator()() const (
    __closure=0x7fffffffbb50)
    at /home/camaud/programming/genepi/node_modules/node-addon-api/napi-inl.h:222
#10 0x00007fffdfde62c4 in Napi::details::WrapCallback<Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}>(Napi::RegisterModule(napi_env__*, napi_value__*, Napi::Object (*)(Napi::Env, Napi::Object))::{lambda()#1}) (callback=...)
    at /home/camaud/programming/genepi/node_modules/node-addon-api/napi-inl.h:104
#11 0x00007fffdfde62ac in Napi::RegisterModule (env=0x2541500, 
    exports=0x251df18, 
    registerCallback=0x7fffdfde5cf2 <initialize(Napi::Env, Napi::Object)>)
    at /home/camaud/programming/genepi/node_modules/node-addon-api/napi-inl.h:224
#12 0x00007fffdfde5f6c in __napi_initialize (env=0x2541500, exports=0x251df18)
    at /home/camaud/programming/genepi/src/genepi/genepi.cpp:3
#13 0x00000000008e14a1 in napi_module_register_by_symbol(v8::Local<v8::Object>, v8::Local<v8::Value>, v8::Local<v8::Context>, napi_value__* (*)(napi_env__*, napi_value__*)) ()
#14 0x00000000008d3352 in node::DLOpen(v8::FunctionCallbackInfo<v8::Value> const&) ()
#15 0x0000000000b8b32f in v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) ()
#16 0x0000000000b8be99 in v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) ()

The code is not open yet but if we cannot find the issue, I will send some files to you.

[BotellaA]

I read the commit [917bd60] - src: remove TODOs by fixing memory leaks (Gabriel Schulhof) #343 and I see now why my code does not work anymore.

    template < class Bound >
    class ClassWrapper : public Napi::ObjectWrap< ClassWrapper< Bound > >
    {
    public:
        using Wrapper = ClassWrapper< Bound >;
        using Descriptor = typename Wrapper::PropertyDescriptor;

        ClassWrapper( const Napi::CallbackInfo& info );

        static void Initialize( Napi::Env& env,
            Napi::Object& target,
            const std::string& name,
            const std::deque< MethodDef >& static_methodList,
            const std::deque< MethodDef >& methodList )
        {
            std::vector< Descriptor > descriptors;
            descriptors.reserve( static_methodList.size() + methodList.size() );
            add_methods( env, static_methodList, descriptors );
            add_methods( env, methodList, descriptors );

            for( unsigned int i = static_methodList.size();
                 i < descriptors.size(); i++ )
            {
                auto& attr =
                    static_cast< napi_property_descriptor& >( descriptors[i] )
                        .attributes;
                attr = static_cast< napi_property_attributes >(
                    attr & ~napi_static );
            }

            auto function =
                Wrapper::DefineClass( env, name.c_str(), descriptors );
            constructor_ = Napi::Persistent( function );
            constructor_.SuppressDestruct();
            target.Set( name.c_str(), function );
        }

        static Napi::Object create(
            Napi::Env env, const std::vector< napi_value >& args )
        {
            Napi::EscapableHandleScope scope( env );
            return scope.Escape( constructor_.New( args ) ).ToObject();
        }

        template < typename... Args >
        static void create_obj( const Napi::CallbackInfo& info, Args&&... args )
        {
            Wrapper::get_smartpointer( info.This() )
                .reset( new Bound{ args... } );
        }

        static Bound* get_bound( const Napi::CallbackInfo& info )
        {
            return Wrapper::get_smartpointer( info.This() ).get();
        }

        static Bound* get_bound( const Napi::Value& arg )
        {
            return Wrapper::get_smartpointer( arg ).get();
        }

    private:
        static std::shared_ptr< Bound >& get_smartpointer(
            const Napi::Value& value )
        {
            return Wrapper::Unwrap( value.ToObject() )->underlying_class_;
        }

        static void add_methods( Napi::Env& env,
            const std::deque< MethodDef >& methodList,
            std::vector< Descriptor >& descriptors )
        {
            for( const auto& method : methodList )
            {
                auto* method_param = new genepi::SignatureParam;
                method_param->methodNum = method.getNum();
                descriptors.emplace_back(
                    Wrapper::StaticMethod( method.getName().c_str(),
                        method.getSignature()->getCaller(), napi_default,
                        static_cast< void* >(
                            Napi::External< genepi::SignatureParam >::New(
                                env, method_param )
                                .Data() ) ) );
            }
        }

    private:
        static Napi::FunctionReference constructor_;
        std::shared_ptr< Bound > underlying_class_;
    };

The idea is to have an automatic wrapper arround a custom class Bound. I have the list of all static function and method pointers. I want to hook to these pointers to the ClassWrapper instance.

The trick is that the methods are not defined in the ClassWrapper, they are defined as classic C++ functions. So I can only use your StaticMethod helper function but I want to use them as a method from the JS side. Removing the napi_static flag does the trick.

To sum up (because I am not sure to be clear...), I want a StaticMethod of Napi but a classic method from JS side. I think this trick can no longer be done with your new implementation of DefineClass.

[gabrielschulhof]

@BotellaA Each function and accessor you define causes the wrapper to allocate data on the heap. This data has to be deleted when it is clear that the accessor or the function will never be called again. It will usually never be called again when it has been garbage-collected. In the case of JS classes this means that the data associated with accessors and prototype methods will be deleted when the JS class itself is garbage-collected, and in the case of static methods the data will be deleted when the static methods are garbage collected, because the static methods can be detached from the class and thus may outlive the class. To reflect this, the new implementation converts static method descriptions to Napi::Functions before calling napi_define_class, and attaches the heap-allocated data to the Napi::Function.

So the reason this does not work anymore is that V8 crashes when attempting to attach a ready-made Napi::Function to the JS class prototype.

[BotellaA]

@gabrielschulhof Ok, I understand the need to create Napi::Functions but not really how to make this design to work... Any recommendation?

[gabrielschulhof]

@BotellaA you can always reach down into N-API for custom behaviour:

Header:
https://github.com/gabrielschulhof/node-addon-examples/blob/5c6c9731b1e337650d887c30b990437d67e43e30/6_object_wrap/node-addon-api/myobject.h#L17

Implementation:
https://github.com/gabrielschulhof/node-addon-examples/blob/5c6c9731b1e337650d887c30b990437d67e43e30/6_object_wrap/node-addon-api/myobject.cc#L5

[BotellaA]

@gabrielschulhof Thanks, I will look into this. Since the error is not on your side, I close the issue.