n-api: test for finalizer calls JS from first round phantom callback

[mlippautz]

V8's API does not allow calling directly back into V8 from weak callbacks, see: `

node/deps/v8/include/v8.h

Line 540 in 938e118

// calls may be called in the first callback. Should additional work be

Instead, a second pass callback should be set using the WeakCallbackInfo API.

Affected are tests using this testing API:

node/test/js-native-api/test_general/test_general.c

Line 234 in 938e118

napi_call_function(env, undefined, js_cb, 0, NULL, NULL));

This should fire a DCHECK for allowed allocations as soon as the JS callback executed non-trivial JS.

It also breaks the assumption that V8 can call out to the callback during the GC when not all pointers (of other v8::Persistent handles) have not been updated.

@hashseed

[hashseed]

@nodejs/n-api
@addaleax
@mcollina

[hashseed]

I guess in V8 we should add a v8::internal::DisallowJavaScriptExecution scope.

[mhdawson]

@mlippautz do you believe its an issue in the test or in the N-API implementation itself?

[addaleax]

@mhdawson It is, yes

[gabrielschulhof]

I interpret this to mean that we should honour the engine's constraints and throw an exception from the N-API implementation whenever we're about to call back into V8 knowing that we're coming from a weak callback and knowing that the call would result in JS execution. I think we can capture this with NAPI_PREAMBLE().

[gabrielschulhof]

@hashseed does this restriction apply also to things like Object::Set() as well? IOW must I avoid all calls that return a Maybe from a weak callback?

[addaleax]

@gabrielschulhof What we do elsewhere in core (e.g. async hooks) is to delay running callbacks that may invoke JS with a SetImmediate call, btw. GC is nondeterministic anyway, and delaying the callback gives us the freedom to do whatever we want in it.

I’d first try either that, or use @mlippautz’ suggestion of using SetSecondPassCallback().

[mlippautz]

I am not fluent in n-api but it looks like napi_add_finalizer would only accept a native callback in finalize_cb. That seems safe.

The problem is that the other parameters passed can be used to invoke JS. Prohibit parameters likely limits usability so documentation should probably mention that no V8 API may be called on them and they should only be passed along. (That's how V8 went about this problem.)

As already mentioned, solutions are posting another task to the platform using node or using SetSecondPassCallback which would post the task from within V8, or potentially execute it synchronously after GC in V8 (e.g. when under memory pressure).

[hashseed]

@hashseed does this restriction apply also to things like Object::Set() as well? IOW must I avoid all calls that return a Maybe from a weak callback?

Yes. It's not so much JS execution that's bad, but all access to the heap that assumes consistent heap state. During GC, that may not be the case.

[gabrielschulhof]

@mlippautz you're right in that there is no indication that it is unsafe to call into V8. It's also true that other engines may not have this restriction. Therefore I think we should try to provide the feature whereby we invoke the weak callback in such a way that it is safe to call into the engine from there.