Repository navigation
n-api: test for finalizer calls JS from first round phantom callback #25927
Description
Activity
I guess in V8 we should add a
v8::internal::DisallowJavaScriptExecutionscope.@mlippautz do you believe its an issue in the test or in the N-API implementation itself?
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().@hashseed does this restriction apply also to things like
Object::Set()as well? IOW must I avoid all calls that return aMaybefrom a weak callback?@gabrielschulhof What we do elsewhere in core (e.g. async hooks) is to delay running callbacks that may invoke JS with a
SetImmediatecall, 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().I am not fluent in n-api but it looks like
napi_add_finalizerwould only accept a native callback infinalize_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
SetSecondPassCallbackwhich would post the task from within V8, or potentially execute it synchronously after GC in V8 (e.g. when under memory pressure).@hashseed does this restriction apply also to things like
Object::Set()as well? IOW must I avoid all calls that return aMaybefrom 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.
@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.
- added a commit that references this issue
on Feb 13, 2019 - added 2 commits that reference this issue
on Mar 4, 2019 - added a commit that references this issue
on Mar 12, 2019 - added a commit that references this issue
on Mar 20, 2019 - added a commit that references this issue
on Jul 27, 2026
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
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
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