Fix memory-related TODOs

[gabrielschulhof]

napi-inl.h has a bunch of places where it allocates memory and leaves a // TODO ... comment as to freeing it.

The following lists the places where the code allocates memory and leaves a TODO, and the solution for implementing the freeing of the memory:

• Creating a function
  Use napi_wrap() to associate the internal data with the function and provide a finalizer.
• Filling out a function-valued property descriptor
  Convert to a napi_value-ed property descriptor where the napi_value is created using the code that creates functions, as modified above.
• Filling out a function-valued or an accessor-valued instance property descriptor
  After calling napi_define_class(), iterate over the property descriptors and, if any of them have their methor, getter, or setter field set to the callback provided by the ObjectWrap class, then attach a vector to the JavaScript class using napi_wrap() and add deleters for the data of all the matching property descriptors.
• Filling out an accessor-valued property descriptor.
  No clear solution.

Normally the solution that applies to function-valued or accessor-valued instance property descriptors could also be used for accessor-valued property descriptors of plain objects, but plain objects are more likely to be wrapped by the user than class constructors, so using napi_wrap() on them would take up a potentially valuable slot.

In N-API, before switching to v8::Private for wrapping we used to do the wrapping by injecting an object into the prototype chain which had an internal slot, and then walking the prototype chain to find the object when asked to napi_unwrap(). We could retain the napi_wrap() slot on a plain object if we stored the deleters list in such an injected object, but retrieving the object becomes somewhat murky, because, as we walk the prototype chain looking for the injected object, the only way we can detect it is by the fact that it is a wrapped object. This does not tell us with certainty that the object was wrapped by us.

In N-API we solved the problem of identifying the object in the prototype chain which holds the wrapped data by maintaining a JavaScript class on the env from which all such injected objects were derived. Thus, for each object encountered on the prototype chain we could check whether it was an instance of this class.

In node-addon-api maintaining such a class is untenable because we do not have the ability to attach classes to env.

Another, perhaps cleaner, solution is to attach the data at a symbol-valued property on the object.

This problem would be rendered trivial by the napi_add_finalizer() PR, but that code would not be available on all versions of Node.js supported by node-addon-api even once it lands.

So, the question remains: Where, on a plain object, to store the data created as part of defining accessor-valued properties, keeping in mind that this data is not visible to users?

[gabrielschulhof]

I think I'll go with a symbol-keyed and external-valued read-only-and-hidden-yet-visible-from-JS property. I'll be unable to cache the symbol value, so I will have to

• retrieve the global
• retrieve the Symbol property
• retrieve the for property
• call the retrieved function with a magic string, like node-addon-api:method-data
• use the resulting value as the name in napi_define_property, along with an external created for this purpose

every time the user creates a function or defines a class or sets accessor-valued properties on an object.

With napi_add_finalizer() hopefully landing soon and getting backported to v8.x, we should be able to use it in node-addon-api as soon as v6.x reaches end-of-life. Honestly though, we may not even then be able to use napi_add_finalizer() until people actually move away from v6.x 😕

There is another mitigating factor in the case of V8: prototype methods and classes do not get garbage-collected because they are based on FunctionTemplate, so the finalizer we add will never actually be called.

So, I guess it's better to just concentrate on the cases where garbage collection is actually possible, and limit a first fix to those cases.

A plain object with an accessor-valued property is almost certainly going to get garbage-collected so the symbol-keyed and external-valued read-only-and-hidden-yet-visible-from-JS property definition is still unavoidable for that case. At least, if/when we decide to use that approach for the prototype methods and accessors, the code will already be there.