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

src: use global SealHandleScope #3945

Closed
wants to merge 6 commits into from
Closed

src: use global SealHandleScope #3945

wants to merge 6 commits into from

Conversation

trevnorris
Copy link
Contributor

Helps to find Handle leaks in Debug mode.

Ref: a5244d3 "deps: backport 1f8555 from v8's upstream"

R=@indutny
R=@jasnell

bnoordhuis and others added 2 commits November 14, 2015 16:35
@bnoordhuis @jasnell
build: fix --without-ssl compile time error
8344161 
Fix the following build error by putting #if guards around the
variables:

    ../src/node.cc: In function 'void node::ParseArgs(int*,
    const char**, int*, const char***, int*, const char***)':
    ../src/node.cc:3037:7: error: 'SSL2_ENABLE' was not declared
    in this scope
           SSL2_ENABLE = true;
           ^
    ../src/node.cc:3039:7: error: 'SSL3_ENABLE' was not declared
    in this scope
           SSL3_ENABLE = true;

Fixes: nodejs/node-v0.x-archive#8645
PR-URL: nodejs#3825
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@othiym23 @jasnell
deps: upgrade npm to 2.14.9
ca97fb6 
This is a roll-up release that includes all changes to npm since 2.13.4.

PR-URL: nodejs#3684
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@indutny
Copy link
Member

indutny commented Nov 20, 2015

Mmm... any reason why it should be backported all the way down to v0.12?

@mscdex mscdex added v8 engine Issues and PRs related to the V8 dependency. v0.12 labels Nov 20, 2015
@trevnorris
Copy link
Contributor Author

@indutny it was easy, and there are still enough people using it v0.12 to make me think it may be useful.

@indutny
Copy link
Member

indutny commented Nov 20, 2015

@trevnorris but I don't think that anyone use Debug builds except core contributors...

@trevnorris
Copy link
Contributor Author

I only copied the commit message about being useful in Debug b/c that's what was there, but if you run

$ node test/simple/test-debug-signal-cluster.js

with a release build you'll see that it still aborts.

@indutny
Copy link
Member

indutny commented Nov 20, 2015

Oh, ok then... This is quite odd, though. LGTM

@jasnell
Copy link
Member

jasnell commented Nov 20, 2015

@trevnorris ... my v8 kung fu is still weak but from what I can tell about what this is doing, LGTM

@mhdawson
tls: prevent server from using dhe keys < 768
caa16b4 
As part of the fix for logjam, node was upgraded to a
level of openssl which rejects connections to servers that
are using keys smaller than 768 bits. It is still possible,
however, to create a server that uses a smaller key size
and and older client may be able to connect to it.

This PR moves us to a secure by default stance on the
server side as well, preventing the creation of a server
using a dhe key size less than 768. This can be overridden
with the command line option which is also added.

It is derived from

nodejs@9b35be5

which was landed in later io.js/node versions but makes
the limit 1024.  This PR uses the smaller limit in order
to meet the recomendations for logjam while matching was
was done on the client side in openssl to minimize the
potential impacton users.

The command line option will only be documented in the
release notes and will not be added to the tls
documentation.  The goal is that people who are
upgrading are aware and can use the option if they
run into issues, but otherwise the option is not
visible/used.

PR-URL: nodejs#3890
Fixes: nodejs/Release#49
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: James Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@trevnorris trevnorris mentioned this pull request Nov 24, 2015
Merged
@trevnorris
Copy link
Contributor Author

@jasnell This commit causes two crashes from debugger tests because they leak. I've given it a go fixing the debugger issue, but no go so far. I'd like this to get into the v0.12.8 release, but unsure w/ the debugger test failures.

@trevnorris
Copy link
Contributor Author

@indutny Figured out the issue. Apparently Environment::GetCurrent(Isolate*) creates a new handle, which from DispatchDebugMessagesAsyncCallback(uv_async_t*) was causing a handle to bubble. I'm not sure if this is still the case today, but I'd hope that simply getting the Environment* wouldn't require a HandleScope. Since it's often necessary to have the Environment* to retrieve the Isolate*.

@trevnorris
deps: backport 1f8555 from v8's upstream
3aa093e 
Original commit message:

    api: introduce SealHandleScope

    When debugging Handle leaks in io.js we found it very convenient to be
    able to Seal some specific (root in our case) scope to prevent Handle
    allocations in it, and easily find leakage.

    R=yangguo
    BUG=

    Review URL: https://codereview.chromium.org/1079713002

    Cr-Commit-Position: refs/heads/master@{nodejs#27766}

Should help us identify and fix Handle leaks in core and user-space code.

NOTE: Works only in Debug build now, but is still better than nothing.
@trevnorris
src: use global SealHandleScope
c44a95d 
Helps to find Handle leaks in Debug mode.

Ref: a5244d3 "deps: backport 1f8555 from v8's upstream"
@trevnorris
node: fix leaking Context handle
7d72435 
The call to node::Environment::GetCurrent(Isolate*) makes the call to
v8::Isolate::GetCurrentContext(). Doing so creates a new handle that was
bubbled to the SealHandleScope.
@trevnorris
Copy link
Contributor Author

@jasnell Now that the issue is fixed, going to run CI one more time then hopefully this can make it into the v0.12 release.

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

trevnorris added a commit that referenced this pull request Nov 24, 2015
@trevnorris
deps: backport 1f8555 from v8's upstream
a042e41 
Original commit message:

    api: introduce SealHandleScope

    When debugging Handle leaks in io.js we found it very convenient to be
    able to Seal some specific (root in our case) scope to prevent Handle
    allocations in it, and easily find leakage.

    R=yangguo
    BUG=

    Review URL: https://codereview.chromium.org/1079713002

    Cr-Commit-Position: refs/heads/master@{#27766}

Should help us identify and fix Handle leaks in core and user-space code.

PR-URL: #3945
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
trevnorris added a commit that referenced this pull request Nov 24, 2015
@trevnorris
src: use global SealHandleScope
f21212a 
Helps to find Handle leaks in Debug mode.

Ref: a5244d3 "deps: backport 1f8555 from v8's upstream"

PR-URL: #3945
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
trevnorris added a commit that referenced this pull request Nov 24, 2015
@trevnorris
node: fix leaking Context handle
5707ef5 
The call to node::Environment::GetCurrent(Isolate*) makes the call to
v8::Isolate::GetCurrentContext(). Doing so creates a new handle that
bubbled to the v8::SealHandleScope().

PR-URL: #3945
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
@trevnorris
Copy link
Contributor Author

Landed on v0.12-staging in a042e41, f21212a and 5707ef5.

@trevnorris trevnorris closed this Nov 24, 2015
@jasnell
Copy link
Member

jasnell commented Nov 24, 2015

@trevnorris ... Ping @rvagg about landing in v0.12. I'm away from my laptop
this week for vacation with the family. Might be able to check in tomorrow
but won't be able to do any PR reviews until Monday.
On Nov 24, 2015 11:37 AM, "Trevor Norris" [email protected] wrote:

Closed #3945 #3945.

—
Reply to this email directly or view it on GitHub
#3945 (comment).

@trevnorris
Copy link
Contributor Author

@jasnell I believe @rvagg is away on vacation this week as well.

trevnorris added a commit that referenced this pull request Dec 4, 2015
@trevnorris @rvagg
deps: backport 1f8555 from v8's upstream
fbc8cd9 
Original commit message:

    api: introduce SealHandleScope

    When debugging Handle leaks in io.js we found it very convenient to be
    able to Seal some specific (root in our case) scope to prevent Handle
    allocations in it, and easily find leakage.

    R=yangguo
    BUG=

    Review URL: https://codereview.chromium.org/1079713002

    Cr-Commit-Position: refs/heads/master@{#27766}

Should help us identify and fix Handle leaks in core and user-space code.

PR-URL: #3945
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
trevnorris added a commit that referenced this pull request Dec 4, 2015
@trevnorris @rvagg
src: use global SealHandleScope
409f6a9 
Helps to find Handle leaks in Debug mode.

Ref: a5244d3 "deps: backport 1f8555 from v8's upstream"

PR-URL: #3945
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
trevnorris added a commit that referenced this pull request Dec 4, 2015
@trevnorris @rvagg
node: fix leaking Context handle
9abcead 
The call to node::Environment::GetCurrent(Isolate*) makes the call to
v8::Isolate::GetCurrentContext(). Doing so creates a new handle that
bubbled to the v8::SealHandleScope().

PR-URL: #3945
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
@rvagg rvagg mentioned this pull request Feb 8, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
@trevnorris
deps: backport 1f8555 from v8's upstream
46c8e21 
Original commit message:

    api: introduce SealHandleScope

    When debugging Handle leaks in io.js we found it very convenient to be
    able to Seal some specific (root in our case) scope to prevent Handle
    allocations in it, and easily find leakage.

    R=yangguo
    BUG=

    Review URL: https://codereview.chromium.org/1079713002

    Cr-Commit-Position: refs/heads/master@{#27766}

Should help us identify and fix Handle leaks in core and user-space code.

PR-URL: #3945
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
@trevnorris
src: use global SealHandleScope
a79baf0 
Helps to find Handle leaks in Debug mode.

Ref: a5244d3 "deps: backport 1f8555 from v8's upstream"

PR-URL: #3945
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
@trevnorris
node: fix leaking Context handle
3a48f00 
The call to node::Environment::GetCurrent(Isolate*) makes the call to
v8::Isolate::GetCurrentContext(). Doing so creates a new handle that
bubbled to the v8::SealHandleScope().

PR-URL: #3945
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
@trevnorris trevnorris deleted the sealhandlescope branch March 28, 2016 22:28
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
@trevnorris @jBarz
deps: backport 1f8555 from v8's upstream
6130273 
Original commit message:

    api: introduce SealHandleScope

    When debugging Handle leaks in io.js we found it very convenient to be
    able to Seal some specific (root in our case) scope to prevent Handle
    allocations in it, and easily find leakage.

    R=yangguo
    BUG=

    Review URL: https://codereview.chromium.org/1079713002

    Cr-Commit-Position: refs/heads/master@{#27766}

Should help us identify and fix Handle leaks in core and user-space code.

PR-URL: nodejs/node#3945
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
@trevnorris @jBarz
src: use global SealHandleScope
b5571c6 
Helps to find Handle leaks in Debug mode.

Ref: a5244d3 "deps: backport 1f8555 from v8's upstream"

PR-URL: nodejs/node#3945
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
@trevnorris @jBarz
node: fix leaking Context handle
6cd82cd 
The call to node::Environment::GetCurrent(Isolate*) makes the call to
v8::Isolate::GetCurrentContext(). Doing so creates a new handle that
bubbled to the v8::SealHandleScope().

PR-URL: nodejs/node#3945
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@trevnorris @indutny @jasnell @mscdex @bnoordhuis @othiym23 @mhdawson