Repository navigation
n-api: test for finalizer calls JS from first round phantom callback #25927
Description
V8's API does not allow calling directly back into V8 from weak callbacks, see: `
Line 540 in 938e118
Instead, a second pass callback should be set using the WeakCallbackInfo API.
Affected are tests using this testing API:
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.
Activity
hashseed commentedon Feb 4, 2019
@nodejs/n-api
@addaleax
@mcollina
hashseed commentedon Feb 4, 2019
I guess in V8 we should add a
v8::internal::DisallowJavaScriptExecutionscope.mhdawson commentedon Feb 6, 2019
@mlippautz do you believe its an issue in the test or in the N-API implementation itself?
addaleax commentedon Feb 6, 2019
@mhdawson It is, yes
gabrielschulhof commentedon Feb 6, 2019
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 commentedon Feb 6, 2019
@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?addaleax commentedon Feb 6, 2019
@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().mlippautz commentedon Feb 7, 2019
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 commentedon Feb 7, 2019
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 commentedon Feb 7, 2019
@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.
n-api: finalize during second-pass callback
n-api: finalize during second-pass callback
n-api: finalize during second-pass callback
n-api: finalize during second-pass callback
n-api: finalize during second-pass callback
n-api: finalize during second-pass callback