Skip to content

src: do not leak handles on debug and exit - #1133

Closed
indutny wants to merge 1 commit into
nodejs:v1.xfrom
indutny:feature/no-more-leaks
Closed

src: do not leak handles on debug and exit#1133
indutny wants to merge 1 commit into
nodejs:v1.xfrom
indutny:feature/no-more-leaks

Conversation

@indutny

@indutny indutny commented Mar 12, 2015

Copy link
Copy Markdown
Member

Ensure HandleScope is created before creating any new handles (which
Context::Scope and Environment::GetCurrent does).

cc @iojs/collaborators @trevnorris @bnoordhuis

I have identified these with the help of : https://codereview.chromium.org/985873002/

Ensure HandleScope is created before creating any new handles (which
`Context::Scope` and `Environment::GetCurrent` does).
@indutny

indutny commented Mar 12, 2015

Copy link
Copy Markdown
Member Author

@bnoordhuis

Copy link
Copy Markdown
Member

Nice! (And obvious in retrospect. :-)) LGTM.

@indutny

indutny commented Mar 12, 2015

Copy link
Copy Markdown
Member Author

Thanks!

@rvagg

rvagg commented Mar 12, 2015

Copy link
Copy Markdown
Member

sweet, LGTM, but sadly I don't imagine these particular ones would impact the apparent leak in #1075 unless I'm missing something

@mscdex

mscdex commented Mar 12, 2015

Copy link
Copy Markdown
Contributor

LGTM

@indutny

indutny commented Mar 12, 2015

Copy link
Copy Markdown
Member Author

@rvagg they won't for sure. I'm still don't know what kind of leak are we dealing with in #1075: JS heap, or C++ heap. Might be both, who knows!

indutny added a commit that referenced this pull request Mar 12, 2015
Ensure HandleScope is created before creating any new handles (which
`Context::Scope` and `Environment::GetCurrent` does).

PR-URL: #1133
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Brian White <mscdex@mscdex.net>
@indutny

indutny commented Mar 12, 2015

Copy link
Copy Markdown
Member Author

Landed in e2c9040, thanks everyone!

@indutny indutny closed this Mar 12, 2015
@indutny
indutny deleted the feature/no-more-leaks branch March 12, 2015 21:52
@rvagg rvagg mentioned this pull request Mar 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants