Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Finalizers are executed without a handle scope #30127

Closed
shiretu opened this issue Oct 26, 2019 · 3 comments
Closed

Finalizers are executed without a handle scope #30127

shiretu opened this issue Oct 26, 2019 · 3 comments
Labels
node-api Issues and PRs related to the Node-API.

Comments

@shiretu
Copy link

shiretu commented Oct 26, 2019

  • Version: v12.5.0
  • Platform: Darwin beast 18.7.0 Darwin Kernel Version 18.7.0: Tue Aug 20 16:57:14 PDT 2019; root:xnu-4903.271.2~2/RELEASE_X86_64 x86_64
  • Subsystem:

I'm trying to make use of 0-copy when passing buffers from native to JS. For this I'm using N-API for C++. Here is a simple example of such a little function which just returns a buffer with 3 bytes in it.

1. The sample native addon source, exporting a single function

struct sample_finalized_buffer_t {
	std::vector<uint8_t> _buffer;

	static Napi::Value New(Napi::Env&& env) {
		sample_finalized_buffer_t *pResult = new sample_finalized_buffer_t();
		pResult->_buffer.push_back(1);
		pResult->_buffer.push_back(2);
		pResult->_buffer.push_back(3);
		return Napi::Buffer<uint8_t>::New(env,
				pResult->_buffer.data(),
				pResult->_buffer.size(),
				Delete,
				pResult
				);
	}

	static void Delete(Napi::Env env, uint8_t *pRawBytes, sample_finalized_buffer_t *pThis) {
		if (pThis != NULL)
			delete pThis;
	}
};

Napi::Value getSampleBuffer(const Napi::CallbackInfo& args){
	return sample_finalized_buffer_t::New(args.Env());
}

Napi::Object RegisterModule(Napi::Env env, Napi::Object exports) {
	exports.Set(Napi::String::New(env, "getSampleBuffer"), Napi::Function::New(env, getSampleBuffer));
	return exports;
}

NODE_API_MODULE(addon, RegisterModule);

2. The sample JS source which calls that native function

const addon = require('path/to/addon');
console.log(addon.getSampleBuffer());
setInterval(() => {
	console.log("working...");
}, 1000);

3. The output of running that sample JS script

<Buffer 01 02 03>
working...
working...
working...
working...
working...
working...
working...
working...
FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope
 1: 0x10006d2dc node::DumpBacktrace(__sFILE*) [/Users/shiretu/work/node/out/Debug/node]
 2: 0x100167bdb node::Abort() [/Users/shiretu/work/node/out/Debug/node]
 3: 0x100168762 node::OnFatalError(char const*, char const*) [/Users/shiretu/work/node/out/Debug/node]
 4: 0x1003ef951 v8::Utils::ReportApiFailure(char const*, char const*) [/Users/shiretu/work/node/out/Debug/node]
 5: 0x1007212f2 v8::internal::HandleScope::Extend(v8::internal::Isolate*) [/Users/shiretu/work/node/out/Debug/node]
 6: 0x1003e0a67 v8::internal::HandleScope::CreateHandle(v8::internal::Isolate*, unsigned long) [/Users/shiretu/work/node/out/Debug/node]
 7: 0x1003f4f8c v8::EmbedderDataFor(v8::Context*, int, bool, char const*) [/Users/shiretu/work/node/out/Debug/node]
 8: 0x1003f541d v8::Context::SlowGetAlignedPointerFromEmbedderData(int) [/Users/shiretu/work/node/out/Debug/node]
 9: 0x10001410b v8::Context::GetAlignedPointerFromEmbedderData(int) [/Users/shiretu/work/node/out/Debug/node]
10: 0x100014051 node::Environment::GetCurrent(v8::Local<v8::Context>) [/Users/shiretu/work/node/out/Debug/node]
11: 0x10010c872 node_napi_env__::node_env() const [/Users/shiretu/work/node/out/Debug/node]
12: 0x10010d1f3 v8impl::(anonymous namespace)::BufferFinalizer::FinalizeBufferCallback(char*, void*) [/Users/shiretu/work/node/out/Debug/node]
13: 0x10012194d node::Buffer::(anonymous namespace)::CallbackInfo::WeakCallback(v8::Isolate*) [/Users/shiretu/work/node/out/Debug/node]
14: 0x100121809 node::Buffer::(anonymous namespace)::CallbackInfo::WeakCallback(v8::WeakCallbackInfo<node::Buffer::(anonymous namespace)::CallbackInfo> const&) [/Users/shiretu/work/node/out/Debug/node]
15: 0x10071b83d unsigned long v8::internal::GlobalHandles::InvokeFirstPassWeakCallbacks<v8::internal::GlobalHandles::Node>(std::__1::vector<std::__1::pair<v8::internal::GlobalHandles::Node*, v8::internal::GlobalHandles::PendingPhantomCallback>, std::__1::allocator<std::__1::pair<v8::internal::GlobalHandles::Node*, v8::internal::GlobalHandles::PendingPhantomCallback> > >*) [/Users/shiretu/work/node/out/Debug/node]
16: 0x10071b703 v8::internal::GlobalHandles::InvokeFirstPassWeakCallbacks() [/Users/shiretu/work/node/out/Debug/node]
17: 0x100777d11 v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::GCCallbackFlags) [/Users/shiretu/work/node/out/Debug/node]
18: 0x100775800 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [/Users/shiretu/work/node/out/Debug/node]
19: 0x100781516 v8::internal::Heap::FinalizeIncrementalMarkingIfComplete(v8::internal::GarbageCollectionReason) [/Users/shiretu/work/node/out/Debug/node]
20: 0x10079ad2f v8::internal::IncrementalMarkingJob::Task::RunInternal() [/Users/shiretu/work/node/out/Debug/node]
21: 0x100261e0e node::PerIsolatePlatformData::RunForegroundTask(std::__1::unique_ptr<v8::Task, std::__1::default_delete<v8::Task> >) [/Users/shiretu/work/node/out/Debug/node]
22: 0x100260966 node::PerIsolatePlatformData::FlushForegroundTasksInternal() [/Users/shiretu/work/node/out/Debug/node]
23: 0x100260730 node::PerIsolatePlatformData::FlushTasks(uv_async_s*) [/Users/shiretu/work/node/out/Debug/node]
24: 0x100f59fc2 uv__async_io [/Users/shiretu/work/node/out/Debug/node]
25: 0x100f78c00 uv__io_poll [/Users/shiretu/work/node/out/Debug/node]
26: 0x100f5a66f uv_run [/Users/shiretu/work/node/out/Debug/node]
27: 0x1001e7d3e node::NodeMainInstance::Run() [/Users/shiretu/work/node/out/Debug/node]
28: 0x100105c3c node::Start(int, char**) [/Users/shiretu/work/node/out/Debug/node]
29: 0x1017368ce main [/Users/shiretu/work/node/out/Debug/node]
30: 0x7fff61d123d5 start [/usr/lib/system/libdyld.dylib]

4. The fix:

Inserting v8::HandleScope hs(isolate); as the first line of code in void CallbackInfo::WeakCallback(Isolate* isolate) function which is located here solves the issue
@shiretu shiretu mentioned this issue Oct 26, 2019
Closed
@legendecas legendecas added the node-api Issues and PRs related to the Node-API. label Oct 26, 2019
@gabrielschulhof
Copy link
Contributor

@addaleax I've seen this in a few places. node::Environment::GetCurrent(v8::Local<v8::Context> context) needs a HandleScope because it performs a property get.

@gabrielschulhof
Copy link
Contributor

Alternatively, we could retrieve the node::Environment using the Isolate* at

node/src/node_api.cc

Line 21 in cb36fa6

return node::Environment::GetCurrent(context());

@gabrielschulhof gabrielschulhof mentioned this issue Oct 26, 2019
Closed
2 tasks
@addaleax
Copy link
Member

Alternatively, we could retrieve the node::Environment using the Isolate* at

node/src/node_api.cc

Line 21 in cb36fa6

return node::Environment::GetCurrent(context());

Node.js Environment instances are conceptually associated with Contexts and not Isolates, so I wouldn’t do that.

Otherwise, see the comment in #30130 – this seems more like a V8 issue to me.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Oct 30, 2019
deps: V8: cherry-pick e5dbc95
53e8bcc 
Original commit message:

    [api] Fix handle leak when getting Context embedder data

    The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns
    a pointer, so the fact that it allocates handles is not obvious to
    the caller.

    Since this is the slow path anyway, simply add a handle scope inside
    of it.

    The tests are also modified to perform the same check for the
    `Object` equivalent of this method.

    Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: nodejs#30127
targos pushed a commit to targos/node that referenced this issue Nov 1, 2019
@targos
deps: V8: cherry-pick e5dbc95
3c71bd0 
Original commit message:

    [api] Fix handle leak when getting Context embedder data

    The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns
    a pointer, so the fact that it allocates handles is not obvious to
    the caller.

    Since this is the slow path anyway, simply add a handle scope inside
    of it.

    The tests are also modified to perform the same check for the
    `Object` equivalent of this method.

    Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: nodejs#30127
PR-URL: nodejs#30130
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this issue Nov 5, 2019
@targos
deps: V8: cherry-pick e5dbc95
104bfb9 
Original commit message:

    [api] Fix handle leak when getting Context embedder data

    The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns
    a pointer, so the fact that it allocates handles is not obvious to
    the caller.

    Since this is the slow path anyway, simply add a handle scope inside
    of it.

    The tests are also modified to perform the same check for the
    `Object` equivalent of this method.

    Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127
PR-URL: #30130
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this issue Nov 8, 2019
@targos
deps: V8: cherry-pick e5dbc95
7a610df 
Original commit message:

    [api] Fix handle leak when getting Context embedder data

    The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns
    a pointer, so the fact that it allocates handles is not obvious to
    the caller.

    Since this is the slow path anyway, simply add a handle scope inside
    of it.

    The tests are also modified to perform the same check for the
    `Object` equivalent of this method.

    Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127
PR-URL: #30130
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit to targos/node that referenced this issue Nov 8, 2019
@targos
deps: V8: cherry-pick e5dbc95
3429e01 
Original commit message:

    [api] Fix handle leak when getting Context embedder data

    The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns
    a pointer, so the fact that it allocates handles is not obvious to
    the caller.

    Since this is the slow path anyway, simply add a handle scope inside
    of it.

    The tests are also modified to perform the same check for the
    `Object` equivalent of this method.

    Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: nodejs#30127
PR-URL: nodejs#30130
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this issue Nov 10, 2019
@targos
deps: V8: cherry-pick e5dbc95
41b1faf 
Original commit message:

    [api] Fix handle leak when getting Context embedder data

    The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns
    a pointer, so the fact that it allocates handles is not obvious to
    the caller.

    Since this is the slow path anyway, simply add a handle scope inside
    of it.

    The tests are also modified to perform the same check for the
    `Object` equivalent of this method.

    Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127
PR-URL: #30130
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this issue Nov 10, 2019
@targos
deps: V8: cherry-pick e5dbc95
3a1a604 
Original commit message:

    [api] Fix handle leak when getting Context embedder data

    The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns
    a pointer, so the fact that it allocates handles is not obvious to
    the caller.

    Since this is the slow path anyway, simply add a handle scope inside
    of it.

    The tests are also modified to perform the same check for the
    `Object` equivalent of this method.

    Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127
PR-URL: #30130
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this issue Nov 11, 2019
@targos
deps: V8: cherry-pick e5dbc95
6bc7a6d 
Original commit message:

    [api] Fix handle leak when getting Context embedder data

    The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns
    a pointer, so the fact that it allocates handles is not obvious to
    the caller.

    Since this is the slow path anyway, simply add a handle scope inside
    of it.

    The tests are also modified to perform the same check for the
    `Object` equivalent of this method.

    Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127
PR-URL: #30130
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit to targos/node that referenced this issue Nov 17, 2019
@targos
deps: V8: cherry-pick e5dbc95
6db7865 
Original commit message:

    [api] Fix handle leak when getting Context embedder data

    The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns
    a pointer, so the fact that it allocates handles is not obvious to
    the caller.

    Since this is the slow path anyway, simply add a handle scope inside
    of it.

    The tests are also modified to perform the same check for the
    `Object` equivalent of this method.

    Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: nodejs#30127
PR-URL: nodejs#30130
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 21, 2019
@MylesBorins
deps: V8: cherry-pick e5dbc95
1e5e8c3 
Original commit message:

    [api] Fix handle leak when getting Context embedder data

    The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns
    a pointer, so the fact that it allocates handles is not obvious to
    the caller.

    Since this is the slow path anyway, simply add a handle scope inside
    of it.

    The tests are also modified to perform the same check for the
    `Object` equivalent of this method.

    Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127

Backport-PR-URL: #30513
PR-URL: #30130
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit to targos/node that referenced this issue Dec 5, 2019
@targos
deps: V8: cherry-pick e5dbc95
cb59fcb 
Original commit message:

    [api] Fix handle leak when getting Context embedder data

    The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns
    a pointer, so the fact that it allocates handles is not obvious to
    the caller.

    Since this is the slow path anyway, simply add a handle scope inside
    of it.

    The tests are also modified to perform the same check for the
    `Object` equivalent of this method.

    Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: nodejs#30127
PR-URL: nodejs#30130
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit to targos/node that referenced this issue Jan 7, 2020
@MylesBorins
deps: V8: cherry-pick e5dbc95
23d792a 
Original commit message:

    [api] Fix handle leak when getting Context embedder data

    The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns
    a pointer, so the fact that it allocates handles is not obvious to
    the caller.

    Since this is the slow path anyway, simply add a handle scope inside
    of it.

    The tests are also modified to perform the same check for the
    `Object` equivalent of this method.

    Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: nodejs#30127
PR-URL: nodejs#30130
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 8, 2020
@MylesBorins
deps: V8: cherry-pick e5dbc95
c5fdb34 
Original commit message:

    [api] Fix handle leak when getting Context embedder data

    The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns
    a pointer, so the fact that it allocates handles is not obvious to
    the caller.

    Since this is the slow path anyway, simply add a handle scope inside
    of it.

    The tests are also modified to perform the same check for the
    `Object` equivalent of this method.

    Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127
Backport-PR-URL: #30109
PR-URL: #30130
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BethGriggs pushed a commit that referenced this issue Feb 6, 2020
@BethGriggs
deps: V8: cherry-pick e5dbc95
25dd890 
Original commit message:

    [api] Fix handle leak when getting Context embedder data

    The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns
    a pointer, so the fact that it allocates handles is not obvious to
    the caller.

    Since this is the slow path anyway, simply add a handle scope inside
    of it.

    The tests are also modified to perform the same check for the
    `Object` equivalent of this method.

    Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127
Backport-PR-URL: #30109
PR-URL: #30130
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
node-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

src: create HandleScope for env retrieval gabrielschulhof/node
4 participants
@addaleax @gabrielschulhof @shiretu @legendecas