n-api: fix inheritance

[gabrielschulhof]

In napi_define_class() we assign prototype properties to InstanceTemplate. This seems to cause an Illegal invocation exception when attempting to call a method on an object which is inherited from its parent class.

OTOH if we retrieve the "prototype" property of the class and assign prototype properties to the resulting object, following the prototype chain and retrieving the method and invoking it from the parent class' prototype works.

This problem has been discussed in nodejs/node-addon-api#246.

Should there be a distinction between assigning properties to a FunctionTemplate's InstanceTemplate and to the object residing at the resulting function's "prototype" property?

@TimothyGu @hashseed what do you think?

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[BridgeAR]

@nodejs/n-api @nodejs/addon-api PTAL

[mhdawson]

This looks odd. Bracket just needs to be removed?

[mhdawson]

Same here code, likely needs to be removed or uncommented.

[mhdawson]

I guess never mind since I was just looking at a single commit and I think its not there when all commits are reviewed.

[mhdawson]

Change looks reasonable as long as @TimothyGu @hashseed confirm its correct.

[gabrielschulhof]

I think the problem is that V8 does not recognize a subclass as being derived from a superclass for the case where the superclass was declared natively, and it's methods include a signature which normally forces their this to be an instance of the superclass or its derivatives.

So, assuming NativeClass has an instance method named superclassMethod,

const NativeClass = require('./build/Release/native_class.node');
class Subclass extends NativeClass {};
const x = new Subclass();
x.superclassMethod();

will work, whereas

const NativeClass = require('./build/Release/native_class.node');
const Subclass = function() {};
Object.setPrototypeOf(Subclass.prototype, NativeClass.prototype);
const x = new Subclass();
x.superclassMethod();

will throw Illegal invocation if the v8::FunctionTemplate for superclassMethod is defined on the native side with the v8::Signature of the superclass v8::FunctionTemplate.

Yet, in both cases x instanceof NativeClass will work and return true.

We apply such a v8::Signature in napi_define_class(), which is why we're getting Illegal invocation errors whenever subclassing classically, rather than with the new extends syntax.

On the native side this means that we cannot really implement napi_inherit() because we would need to be able to retrieve the v8::FunctionTemplate somehow, even for functions which were not created using N-API.

[gabrielschulhof]

This PR actually drops the v8::Signature - which may be what makes it work, rather than the move away from the v8::ObjectTemplate to the object at the "prototype" property.

[gabrielschulhof]

Yep - as soon as I add the signature back in I get the Illegal invocation error.

[gabrielschulhof]

So, alternatively, we could remove the v8::Signature and rely on addon authors to check the incoming this value via napi_instanceof().

[mhdawson]

@gabrielschulhof can we get together for 15 mins to discuss on Monday? That will help me understand/comment more quickly.

[gabrielschulhof]

@mhdawson absolutely. I'll be lurking in the hangout.

[gabrielschulhof]

... starting at around 9:15 AM EDT.

[gabrielschulhof]

@mhdawson it seems that v8::Signature has been there for a long time. Nan has had it because Node.js has had it since 0.10.27. So, all classes created using Nan::ObjectWrap or node::ObjectWrap have methods which throw "Illegal invocation" when invoked on an instance of a class derived in any way other than class X extends TheNativeClass on the JS side or v8::FunctionTemplate::Inherit on the native side.

So, nodejs/node-addon-api#246 is really a feature request.

Yet, I'm still not sure whether V8 could be modified to recognize a method call as valid not only when the receiver is an instance of a subclass declared via extends on the JS side and v8::FunctionTemplate::Inherit on the native side, but also when the receiver is an instance of a subclass by virtue of having its prototype chained to the superclass by a call to Object.setPrototypeOf().

After all, in both cases inst instanceof Superclass will return true.

@hashseed @TimothyGu please weigh in from the V8 side!

[gabrielschulhof]

The reason why v8::Signature was added:

commit 08a5b442e42ff67bd5666604eebe69f1b4a1c919
    node: add signature to SET_PROTOTYPE_METHOD
    
    This prevents segfaults when a native method is reassigned to a
    different object (which corrupts args.This()).  When unwrapping,
    clients should use args.Holder() instead of args.This().
    
    Closes #6690.

[hashseed]

Sorry, I just found about this thread now. @verwaest probably knows more about this!

[hashseed]

If I'm not mistaken, you are running into this check. If you have a signature defined, we will iterate the prototype chain in GetCompatibleReceiver until we find an object that was created from the signature template. If you don't need the signature restriction, I'd say remove it.

[TimothyGu]

Sorry, just found out about this thread as well. I will take a look soon, but my initial hunch is that the invalid invocation due to signature check is hinting at another bug.

[gabrielschulhof]

@hashseed Node.js has always had this restriction. As a result, instances subclasses of classes created with node::ObjectWrap, Nan::ObjectWrap, and now napi_define_class() cannot call methods defined on the parent class unless the subclasses are defined with extends in JavaScript or FunctionTemplate::Inherit in C++.

If GetCompatibleReceiver() iterates the prototype chain, then it seems to miss the prototype assigned with Object.setPrototypeOf().

[gabrielschulhof]

@hashseed I checked the way GetCompatibleReceiver() behaves when the subclass is defined via extends, and when it's defined via Object.setPrototypeOf().

In the former case, it returns before it ever iterates, because it seems that the function template from the signature is also the function template for the subclass as well. IOW, it returns from here.

OTOH, when the subclass is constructed via prototype assignment, it also doesn't iterate over the prototype chain, because the following check succeeds and it immediately returns nullptr.

[gabrielschulhof]

Closing this as dropping the signature is not the right way to go.