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.

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

src: create HandleScope for env retrieval #30130

Closed

Conversation

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Oct 26, 2019

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
@gabrielschulhof gabrielschulhof requested a review from addaleax Oct 26, 2019
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Oct 26, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Oct 26, 2019

@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

This comment has been minimized.

Copy link
Member

addaleax commented Oct 26, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Oct 26, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Oct 26, 2019

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

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Oct 26, 2019

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

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Oct 26, 2019

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

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Oct 26, 2019

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

This comment has been minimized.

Copy link
Member

addaleax commented Oct 26, 2019

@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

This comment has been minimized.

Copy link
Member

addaleax commented Oct 26, 2019

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

@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:missing-handle-scope branch from 49bc1be to 3b52bc6 Oct 26, 2019
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Oct 26, 2019

@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.

src/node_api.cc Outdated Show resolved Hide resolved
@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:missing-handle-scope branch from 3b52bc6 to bf28635 Oct 26, 2019
@shiretu

This comment has been minimized.

Copy link

shiretu commented Oct 27, 2019

Hi guys,

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

Thanks

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Oct 27, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Oct 27, 2019

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

@shiretu

This comment has been minimized.

Copy link

shiretu commented Oct 28, 2019

@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

This comment has been minimized.

Copy link
Member

addaleax commented Oct 28, 2019

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

This comment has been minimized.

Copy link
Member

addaleax commented Oct 28, 2019

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

@shiretu

This comment has been minimized.

Copy link

shiretu commented Oct 28, 2019

@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

This comment has been minimized.

Copy link

shiretu commented Oct 28, 2019

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

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Oct 28, 2019

@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.

@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:missing-handle-scope branch from bf28635 to 383e888 Oct 30, 2019
@gabrielschulhof gabrielschulhof dismissed cjihrig’s stale review Oct 30, 2019

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

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Oct 30, 2019

@targos oh, wow! I thought I had. I guess I forgot to commit that. Sorry!

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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127
@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:missing-handle-scope branch from 383e888 to 53e8bcc Oct 30, 2019
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Oct 30, 2019

@targos I think I did bump the embedder version, but then I must have rebased past another cherry-pick and git helpfully realized that one of my hunks had already been applied 🙂 In any case, it's fixed now 🙂

@targos
targos approved these changes Oct 30, 2019
Copy link
Member

targos left a comment

thanks!

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@mhofman mhofman mentioned this pull request Oct 31, 2019
3 of 3 tasks complete
gabrielschulhof added a commit that referenced this pull request Oct 31, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127
PR-URL: #30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Oct 31, 2019

Landed in 61d6144.

@gabrielschulhof gabrielschulhof deleted the gabrielschulhof:missing-handle-scope branch Oct 31, 2019
targos added a commit to targos/node that referenced this pull request Nov 1, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: nodejs#30127
PR-URL: nodejs#30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos added a commit to targos/node that referenced this pull request Nov 4, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: nodejs#30127
PR-URL: nodejs#30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos added a commit that referenced this pull request Nov 5, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127
PR-URL: #30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos mentioned this pull request Nov 5, 2019
addaleax added a commit to targos/node that referenced this pull request Nov 5, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: nodejs#30127
PR-URL: nodejs#30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos added a commit that referenced this pull request Nov 8, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127
PR-URL: #30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos added a commit to targos/node that referenced this pull request Nov 8, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: nodejs#30127
PR-URL: nodejs#30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos added a commit to targos/node that referenced this pull request Nov 8, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: nodejs#30127
PR-URL: nodejs#30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos added a commit that referenced this pull request Nov 10, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127
PR-URL: #30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos added a commit that referenced this pull request Nov 10, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127
PR-URL: #30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos added a commit that referenced this pull request Nov 11, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127
PR-URL: #30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos added a commit to targos/node that referenced this pull request Nov 17, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: nodejs#30127
PR-URL: nodejs#30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins added a commit that referenced this pull request Nov 21, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: #30127

Backport-PR-URL: #30513
PR-URL: #30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BridgeAR BridgeAR mentioned this pull request Nov 21, 2019
targos added a commit to targos/node that referenced this pull request Dec 5, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: nodejs#30127
PR-URL: nodejs#30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos added a commit to targos/node that referenced this pull request Dec 5, 2019
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 <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64583}

Refs: v8/v8@e5dbc95
Fixes: nodejs#30127
PR-URL: nodejs#30130
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.