node-addon-api: [BUG] node REPL crashes creating ObjectWrap in getter

The node REPL seems to crash if a C++ getter creates an instance of a class that extends ObjectWrap.

This works in a script, but it crashes as soon as the REPL’s auto-complete attempts to “preview” bar.get_new_foo:

#include <napi.h>

struct Foo : public Napi::ObjectWrap<Foo> {
  static Napi::Function Init(Napi::Env env, Napi::Object exports) {
    return DefineClass(env, "Foo", {});
  }
  Foo(Napi::CallbackInfo const& info) : Napi::ObjectWrap<Foo>(info){};
};

struct Bar : public Napi::ObjectWrap<Bar> {
  static Napi::Function Init(Napi::Env env, Napi::Object exports) {
    FooConstructor = Napi::Persistent(Foo::Init(env, exports));
    FooConstructor.SuppressDestruct();
    return DefineClass(env, "Bar", {InstanceAccessor<&Bar::get_new_foo>("get_new_foo")});
  }

  Bar(Napi::CallbackInfo const& info) : Napi::ObjectWrap<Bar>(info){};

 private:
  static Napi::FunctionReference FooConstructor;
  Napi::Value get_new_foo(Napi::CallbackInfo const& info) {
    return FooConstructor.New({});
  }
};

Napi::FunctionReference Bar::FooConstructor;

struct NodeREPLCrashRepro : public Napi::Addon<NodeREPLCrashRepro> {
  NodeREPLCrashRepro(Napi::Env env, Napi::Object exports) {
    DefineAddon(exports, {InstanceValue("Bar", Bar::Init(env, exports))});
  }
};

NODE_API_ADDON(NodeREPLCrashRepro);
$ node
Welcome to Node.js v15.14.0.
Type ".help" for more information.
> var { Bar } = require(`./build/Debug/crash_repro.node`)
undefined
> var bar = new Bar()
undefined
> bar.gFATAL ERROR: Error::Error napi_create_reference
 1: 0xa89e60 node::Abort() [node]
 2: 0x9ade29 node::FatalError(char const*, char const*) [node]
 3: 0x9ade32  [node]
 4: 0xa55b3b napi_fatal_error [node]
 5: 0x7f3bf031ea4e Napi::ObjectReference::~ObjectReference() [build/Debug/crash_repro.node]
 6: 0x7f3bf031eb14 Napi::Error::Error(napi_env__*, napi_value__*) [build/Debug/crash_repro.node]
 7: 0x7f3bf031e97f Napi::Error::New(napi_env__*) [build/Debug/crash_repro.node]
 8: 0x7f3bf031e758 Napi::Function::New(unsigned long, napi_value__* const*) const [build/Debug/crash_repro.node]
 9: 0x7f3bf031e6d9 Napi::Function::New(std::initializer_list<napi_value__*> const&) const [build/Debug/crash_repro.node]
10: 0x7f3bf031f3a8 Napi::FunctionReference::New(std::initializer_list<napi_value__*> const&) const [build/Debug/crash_repro.node]
11: 0x7f3bf032050d Bar::get_new_foo(Napi::CallbackInfo const&) [build/Debug/crash_repro.node]
12: 0x7f3bf031db65  [build/Debug/crash_repro.node]
13: 0x7f3bf031dc39  [build/Debug/crash_repro.node]
14: 0x7f3bf031dbfd  [build/Debug/crash_repro.node]
15: 0xa390bf  [node]
16: 0x149e62d  [node]
Aborted

About this issue

  • Original URL
  • State: closed
  • Created 3 years ago
  • Comments: 28 (23 by maintainers)

Most upvoted comments

Did a quick version bisection. It looks like the node version upgrade from v16.5.0 to v16.6.0 “fixes” this bug. https://github.com/nodejs/node/releases/tag/v16.6.0

Discussed in the Node-api team meeting today. Gabriel confirmed based on past experience that we need to pull in dependent changes. Likelihood being accepted for 14.x will depend on how bid the change ends up being.

I can confirm https://chromium-review.googlesource.com/c/v8/v8/+/2857634 fixed calling side-effect accessors on side-effect-free debug-evaluate call. (manually picked the patch on v14.x locally to test out)

I’m not sure what the policy is of backporting v8 patches. Since this patch does not break ABI, I’d believe we can safely land the patch on v12.x and v14.x lines (with conflicts resolved, apparently there are lots of conflicts).

#1075 has landed, but we still see an error in the define properties as JavaScript execution is not allowed.

@legendecas clarified that it is because the Repl tries to preview the result of a function but sets the environment so JavaScript Execution is not allowed. He already checked and we seem to set the flag telling V8 that the method has side effects which should prevent V8 from trying to call it in preview mode.

Next step is to dig a bit deeper into why the method is still being called. @legendecas will try to do that.

There seem to be two issues here:

  1. The napi_new_instance() call here returns a napi_generic_failure status.
  2. NAPI_THROW_IF_FAILED calls this Error constructor. a. That constructor passes the env/error pair to this Error constructor. b. That constructor’s napi_create_reference call returns an error code.

This is reminiscent of Python’s “while handling the original error, another error occurred” message.