src: create HandleScope for env retrieval

[gabrielschulhof]

Getting the current node::Environment using a
v8::Local<v8::Context> needs to be done within a v8::HandleScope.

Fixes: #30127

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[addaleax]

@gabrielschulhof Can you tell more about what this solves? The call would usually be inside a handle scope anyway because it takes a Local<> argument (the only exception I can think of would be if there was an additional SealHandleScope inside the outer HandleScope), and the part of the code that's being wrapped in a handle scope in this PR shouldn't allocate any new local handles (so if it does that might be a bug in V8)?

Edit: Looks like the slow path in https://github.com/v8/v8/blob/fbbcbba7bbb5c06031b74eef5ec895ceaaa91a97/src/api/api.cc#L1291-L1300 does need a HandleScope, yes, but preferably inside that method?

[gabrielschulhof]

@addaleax in N-API we obtain the v8::Local<v8::Context> by converting a persistent reference to a local one: https://github.com/nodejs/node/blob/master/src/js_native_api_v8.h#L35 The conversion is just a cast (https://github.com/nodejs/node/blob/master/src/util.h#L717-L722), so we don't allocate any handle scopes. Thus, if the stack is such that it's coming straight from a weak callback then there is no handle scope in any of the frames leading up to it.

I agree that perhaps the V8 method should create a handle scope if it needs one and if there is any doubt as to whether a handle scope is present in any of the frames leading up to it.

[addaleax]

@addaleax in N-API we obtain the v8::Local<v8::Context> by converting a persistent reference to a local one: https://github.com/nodejs/node/blob/master/src/js_native_api_v8.h#L35 The conversion is just a cast (https://github.com/nodejs/node/blob/master/src/util.h#L717-L722), so we don't allocate any handle scopes. Thus, if the stack is such that it's coming straight from a weak callback then there is no handle scope in any of the frames leading up to it.

I know, but … that’s kind of because we’re hacking into V8 internals here, and I’d argue that if we have to fix this in Node.js, the call site should behave as if it were accessing the handle properly (i.e. a HandleScope in node_napi_env__::node_env()).

I agree that perhaps the V8 method should create a handle scope if it needs one and if there is any doubt as to whether a handle scope is present in any of the frames leading up to it.

Ok – do you want to open a V8 CL or should I?

[gabrielschulhof]

@addaleax actually, I think it may not be to V8 after all. The fact that we are hacking V8 here might allow the V8 folks to make the argument that what we're doing is not supported and therefore it's up to us (Node.js) to fix it.

But if it is up to us, then what is the best way? Does adding a handle scope come with a significant performance penalty? If so, we should add the HandleScope as far away from node::Environment::GetCurrent as possible so as to affect as few code paths as possible.

OTOH, if the performance penalty of adding a HandleScope is not so large, then adding the handle scope at napi_env__::context() makes the most sense because it would cover current and future use cases where a context is "somehow" (by way of our hack) available without first having created a HandleScope.

[gabrielschulhof]

For perspective, I also ran into this problem as I was working on the reference cleanup in #28428.

[gabrielschulhof]

NM, I don't think we can do napi_env__::context() because the HandleScope would immediately go out of scope 🤦

[devsnek]

You can use an EscapableHandleScope and return handle_scope.Escape(context), but that relies on there being an outer handle scope.

[gabrielschulhof]

Looking at the stack in #30127 I'm thinking that v8impl::(anonymous namespace)::BufferFinalizer::FinalizeBufferCallback might be a good place to declare a HandleScope because that's where we need the node::Environment for adding an immediate.

[addaleax]

@addaleax actually, I think it may not be to V8 after all. The fact that we are hacking V8 here might allow the V8 folks to make the argument that what we're doing is not supported and therefore it's up to us (Node.js) to fix it.

I don’t think so. This is not the only way to trigger this issue (a SealHandleScope would also lead to it), and I think this qualifies as a handle leak otherwise.

Does adding a handle scope come with a significant performance penalty?

It’s not a huge difference, but it’s also avoidable and Environment::GetCurrent() is a frequently used function.

NM, I don't think we can do napi_env__::context() because the HandleScope would immediately go out of scope 🤦

It kind of doesn’t, as you pointed out – we don’t actually access the handle in a way that V8 doesn’t know anything about. But it also wouldn’t help with this issue, that’s true.

Looking at the stack in #30127 I'm thinking that v8impl::(anonymous namespace)::BufferFinalizer::FinalizeBufferCallback might be a good place to declare a HandleScope because that's where we need the node::Environment for adding an immediate.

I think that’s fine but if you do, maybe add a comment that that is a workaround and not intended to be permanent.

[addaleax]

Fwiw, here’s a V8 CL for this: https://chromium-review.googlesource.com/c/v8/v8/+/1879902

[gabrielschulhof]

@addaleax I updated the PR to add the HandleScope at v8impl::(anonymous namespace)::BufferFinalizer::FinalizeBufferCallback, and a comment to remove it if/once we have your CL.

[shiretu]

Hi guys,

Is there an work around for this with existing 8, 10 and 12 releases?

Thanks

[addaleax]

@shiretu I wouldn’t know how to work around this bug if you run into it, sorry.

That being said, I also wouldn’t know how to cause this bug to happen in a standard (non-debug) Node.js build – we don’t compile V8 with checks by default. Do you use the Node.js binary downloaded from https://nodejs.org/?

[gabrielschulhof]

@shiretu we'll certainly backport this as far back as possible once it lands, but, of course, that'll take time.

[shiretu]

@addaleax : yes, it happens with downloaded and locally compiled bins. The downloaded ones are fetched with ‘nvm’, but can’t say for sure where is it fetches them from.

@gabrielschulhof: thanks. I’ll watch this issue for closure than.

[addaleax]

yes, it happens with downloaded and locally compiled bins.

That’s … somewhat concerning? SlowGetAlignedPointerFromEmbedderData should only really ever be called if N-API/Node.js was compiled with V8_ENABLE_CHECKS, but that is not the case for our release builds?

@shiretu Do you get the same issue with Node 13? Just to verify…

[addaleax]

Also, the V8 CL has landed as v8/v8@e5dbc95cc0bf, so I think this PR can also be turned into a cherry-pick of that.

[shiretu]

@addaleax: I have checked this again. Made absolutely sure that the right bins are used. 8, 10 and 12 from nvm all behave as expected, no crash.

The locally compiled version is produced like this:

./configure \ --prefix=/tmp/node \ --debug \ --gdb \ --without-intl \ --ninja \ ninja -C out/Debug -j32 

That is always failing.

I'm so sorry, I was so sure that the right bins were used in the tests, but it seems that I've made a mistake and I was running the locally compiled version thinking it was actually one of the official versions.

Nevertheless, the bug still stands IMHO: that finalizer is executed without a HS

[shiretu]

@addaleax�: the locally compiled version is from a release tag, same version as the official releases

[addaleax]

@shiretu Yeah, sure, a bug in debug mode is still a bug. But it’s good to know that this doesn’t affect release builds.

@cjihrig please take another look as the PR has changed radically.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/26250/

[gabrielschulhof]

Landed in 61d6144.