Add wrappers for `node_api_nogc_env` and `node_api_nogc_finalize`

[KevinEady]

Since https://github.com/nodejs/node/pull/50060 is now in core, we need to add support for these types.

Looking at the Node-API docs, we can find which node-addon-api methods on Napi::Env should be split between normal and nogc variants. The normal variant can use nogc methods, but not the other way around:

• nogc:
  • AddCleanupHook
  • GetInstanceData
  • SetInstanceData
  • GetModuleFileName
• normal:
  • Global
  • Undefined
  • Null
  • IsExceptionPending
  • GetAndClearPendingException
  • RunScript

Additionally, all JavaScript-using paradigms (creating new Values, calling functions, ...) should use the normal variant, and have a compile-time error if a nogc variant is supplied.

[KevinEady]

Currently, the four nogc methods call NAPI_THROW_IF_FAILED_VOID, creating a new Napi::Error. This won't work in nogc-land since it's creating a new Value. I would like to discuss this on the 24 May Node-API call.

[KevinEady]

Also struggling a bit with figuring out how to make napi[-inl].h smart enough to know when node_api_nogc_env is available for use. For example, we cannot use these new types if compiling with headers for node v18.17.1, and I don't think we want to break compilation on non-most-recent-Node.js builds.

[KevinEady]

Discussed in 24 May Node-API meeting:

• Check under which circumstances the nogc functions could fail; if they are "reasonable" (eg. invalid parameters passed), we could just some fatal exception instead
• Use the POST FINALIZER definition to guard against nogc types

[KevinEady]

Another issue that i'm struggling with regarding ObjectWrap. The destructor uses Reference<Napi::Object>::Value() which calls napi_get_reference_value, and this method cannot be used inside GC:

https://github.com/nodejs/node-addon-api/blob/7c79c331405ffc41f8d5dab958cde3e93bf90306/napi-inl.h#L4438-L4450

https://github.com/nodejs/node-addon-api/blob/7c79c331405ffc41f8d5dab958cde3e93bf90306/napi-inl.h#L3265-L3275

Thoughts on how to proceed in this use case...? Does this napi_remove_wrap need to be moved into a post-finalizer block, scheduled/added at the ObjectWrap instance construction?

cc: @legendecas @gabrielschulhof @vmoroz

[KevinEady]

Additionally, in ObjectWrap construction, the napi_wrap gets passed ObjectWrap<T>::FinalizeCallback, which in turn creates some handle scopes, which cannot be done in finalization.

https://github.com/nodejs/node-addon-api/blob/7c79c331405ffc41f8d5dab958cde3e93bf90306/napi-inl.h#L4431

Not sure how to proceed on this either. https://github.com/nodejs/node-addon-api/blob/7c79c331405ffc41f8d5dab958cde3e93bf90306/napi-inl.h#L4878-L4886

[KevinEady]

So continuing to dig a bit, mostly keeping this as notes... The combination of the destructor ~ObjectWrap and the FinalizeCallback is supposed to behave in this manner:

• When the C++ object is deleted, the destructor removes the wrap if the reference holds a value.
• When the JS object is garbage collected, the user's custom virtual void Finalize(Env) runs.

So, there are two scenarios we need to cover:

1. The C++ object is deleted prior to GC. Eg: Creating an ObjectWrap'd instance inside a native C++ method on the stack that is never returned to Node, causing the deletion when the object goes out of scope.
2. The JS object is GC'd prior to destruction. Eg: Creating an ObjectWrap'd instance and returning it back to Node for ownership.

[legendecas]

Seems like this is an exploit of the behavior of napi_get_reference_value. napi_get_reference_value will fail if it is called in finalizers without opening a handle scope. However, in a finalizer, the reference was reset and napi_get_reference_value will simply return a nullptr (ref).

[KevinEady]

napi_get_reference_value will fail if it is called in finalizers without opening a handle scope.

From my understanding, this is intended behavior...? Since the function takes a napi_env (vs. the nogc counterpart), I expect that it requires some sort of JavaScript engine integration/usage.

Hopefully we can discuss this in today's meeting.

[github-actions[bot]]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.