Object.getOwnPropertyDescriptor throws on accessor properties of an ObjectWrap class' prototype object

[argv-minus-one]

An error is thrown when trying to get the property descriptor of a Napi::ObjectWrap::InstanceAccessor from the class' prototype object.

Suppose you have a class named DemoClass, which was defined by Napi::ObjectWrap::DefineClass. Suppose it has a property named hello, which was defined by Napi::ObjectWrap::InstanceAccessor. If you then try to Object.getOwnPropertyDescriptor(DemoClass.prototype, "hello"), you get an error:

run.js:2
Object.getOwnPropertyDescriptor(DemoClass.prototype, "hello");
       ^

Error: Invalid argument
    at Function.getOwnPropertyDescriptor (<anonymous>)
    at Object.<anonymous> (run.js:2:8)
    at Module._compile (internal/modules/cjs/loader.js:738:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:749:10)
    at Module.load (internal/modules/cjs/loader.js:630:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:570:12)
    at Function.Module._load (internal/modules/cjs/loader.js:562:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:801:12)
    at internal/main/run_main_module.js:21:11

The reason is that Napi::ObjectWrap::InstanceGetterCallbackWrapper attempts to unwrap this, which doesn't work because DemoClass.prototype is not a wrapper!

But why is the callback even being called? Node checks whether native instance methods are called on a valid instance of the class, and doesn't even call the native function if not. Shouldn't it perform the same checks on native instance accessors?

I have prepared a Gist demonstrating the problem. To use:

1. Download or git clone it.
2. npm install
3. node run.js

[mhdawson]

To me it seems more that maybe InstanceGetterCallbackWrapper should handle being called when Unwrap fails or that it should check in that is has been called on a wrapper.

I don't think we thought anybody would ask for the property or descriptor on the prototype, it's just there so that it is inherited when instances are created for Wrapped objects.

I think we've effectively told the runtime that the InstnaceGEtterCallbackWrapper is a property on the prototype so unless I'm missing something it's reasonable for them to be called.

This is what it would look like if it figured out that it was not called on a wrapper and just returned undefined:

undefined
{
  value: undefined,
  writable: false,
  enumerable: false,
  configurable: false
}

Not sure if that is useful though.

[mhdawson]

With the latest comment this might also have some effect on the behaviour: nodejs/node#27851 (comment)

I'd tried the earlier version but it did not resolve the reported issue.

[mhdawson]

This is the output of the test case provided once the current version of nodejs/node#27851 is applied:

{
  get: [Function (anonymous)],
  set: undefined,
  enumerable: false,
  configurable: false
}

[mhdawson]

Interestingly the change in f nodejs/node#27851 breaks one of our unit tests. My first guess is a test problem as it depends on the order that the keys are reports by "for in"

[mhdawson]

This confirms that depending on ordering is incorrect:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in
A for...in loop iterates over the properties of an object in an arbitrary order (see the delete operator for more on why one cannot depend on the seeming orderliness of iteration, at least in a cross-browser setting).