src: fix handle leaks #7711

bnoordhuis · 2016-07-13T15:38:23Z

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

cjihrig · 2016-07-13T15:45:10Z

LGTM pending CI

rvagg · 2016-07-14T04:31:56Z

src/node_buffer.cc

  Environment* env = Environment::GetCurrent(isolate);
-  EscapableHandleScope handle_scope(env->isolate());


Can you explain how this is leaky? Just out of interest.

Environment::GetCurrent() calls Isolate::GetCurrentContext() which creates a Local<Context> in the current handle scope, i.e., the scope of the caller.

trevnorris · 2016-07-14T05:08:09Z

LGTM

Fishrock123 · 2016-07-14T09:13:10Z

Are these commits squashable? Maybe add a note in the commit message explaining Rod's question for posterity

bnoordhuis · 2016-07-14T13:25:37Z

They're distinct fixes, IMO. Also, explanations are (and were) in the commit logs...

cjihrig · 2016-07-14T13:25:54Z

That information is already in the commit message. I was wondering the same thing during review.

addaleax · 2016-07-15T15:09:24Z

LGTM

bnoordhuis · 2016-07-16T14:17:18Z

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

Fix handle leaks in Buffer::New() and Buffer::Copy() by creating the handle scope before looking up the env with Environment::GetCurrent(). Environment::GetCurrent() calls v8::Isolate::GetCurrentContext(), which creates a handle in the current scope, i.e., the scope created by the caller of Buffer::New() or Buffer::Copy(). PR-URL: nodejs#7711 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Create a handle scope before performing a check that creates a handle, otherwise the handle is leaked into the handle scope of the caller. PR-URL: nodejs#7711 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

API function callbacks run inside an implicit HandleScope. We don't need to explicitly create one and in fact introduce some unnecessary overhead when we do. PR-URL: nodejs#7711 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

bnoordhuis · 2016-07-18T08:56:10Z

The node-test-commit-plinux buildbots seem to be offline but everything else is green. Landed in c897d0b...48c52d7, thanks for the reviews.

Fix handle leaks in Buffer::New() and Buffer::Copy() by creating the handle scope before looking up the env with Environment::GetCurrent(). Environment::GetCurrent() calls v8::Isolate::GetCurrentContext(), which creates a handle in the current scope, i.e., the scope created by the caller of Buffer::New() or Buffer::Copy(). PR-URL: #7711 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Create a handle scope before performing a check that creates a handle, otherwise the handle is leaked into the handle scope of the caller. PR-URL: #7711 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

API function callbacks run inside an implicit HandleScope. We don't need to explicitly create one and in fact introduce some unnecessary overhead when we do. PR-URL: #7711 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>