src: prevent persistent handle resource leaks #18656

bnoordhuis · 2018-02-08T21:12:29Z

Replace v8::Persistent with node::Persistent, a specialization that
resets the persistent handle on destruction. Prevents accidental
resource leaks when forgetting to call .Reset() manually.

I'm fairly confident this commit fixes a number of resource leaks that
have gone undiagnosed so far.

Related to #18650.

joyeecheung · 2018-02-08T22:08:39Z

src/node_file.cc

@@ -537,7 +536,8 @@ inline FSReqBase* AsyncDestCall(Environment* env,
  }

  if (req_wrap != nullptr) {
-    args.GetReturnValue().Set(req_wrap->persistent());
+    auto object = PersistentToLocal(args.GetIsolate(), req_wrap->persistent());
+    args.GetReturnValue().Set(object);


Maybe just req_wrap->object() here?

Good point, don't know what I was thinking.

jasnell · 2018-02-09T23:51:15Z

src/node_persistent.h

+namespace node {
+
+template <typename T>
+struct ResetInDestructorPersistentTraits {


A key reason leaks end up happening around all this is the fact that much of this is horribly under-documented. Mind adding some code comments in here that explains how/why this is here?

Good idea, I'll do that.

bnoordhuis · 2018-02-11T20:49:02Z

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

bnoordhuis · 2018-02-12T00:45:12Z

parallel/test-http2-createwritereq crashes on some buildbots; the other instances of red are infrastructure issues. Unfortunately, I can't reproduce the failure locally.

jasnell · 2018-02-12T01:51:58Z

The http2-createwritereq test does seem to have become a bit flaky lately. Will investigate. /cc @addaleax

addaleax · 2018-02-13T00:49:03Z

Unfortunately, I can't reproduce the failure locally.

I can, it fails about 1/4 of the time; I’ll take a look today or tomorrow.

Here’s the stack trace:

Thread 1 "node_g" received signal SIGSEGV, Segmentation fault.
0x0000558f7386c0f9 in v8::base::Relaxed_Load (ptr=0x6057ca10) at ../deps/v8/src/base/atomicops_internals_portable.h:168
168	  return __atomic_load_n(ptr, __ATOMIC_RELAXED);
(gdb) bt
#0  0x0000558f7386c0f9 in v8::base::Relaxed_Load (ptr=0x6057ca10) at ../deps/v8/src/base/atomicops_internals_portable.h:168
#1  0x0000558f73888eb7 in v8::internal::HeapObject::map_word (this=0x6057ca11) at ../deps/v8/src/objects-inl.h:1040
#2  0x0000558f73888e65 in v8::internal::HeapObject::map (this=0x6057ca11) at ../deps/v8/src/objects-inl.h:976
#3  0x0000558f738885ac in v8::internal::HeapObject::IsJSReceiver (this=0x6057ca11) at ../deps/v8/src/objects-inl.h:295
#4  0x0000558f73888224 in v8::internal::Object::IsJSReceiver (this=0x6057ca11) at ../deps/v8/src/objects-inl.h:174
#5  0x0000558f7388577a in v8::Utils::OpenHandle (that=0x558f7706d720, allow_empty_handle=false) at ../deps/v8/src/api.h:353
#6  0x0000558f738cac2e in v8::Object::InternalFieldCount (this=0x558f7706d720) at ../deps/v8/src/api.cc:6257
#7  0x0000558f735b17e4 in node::Wrap<void> (object=..., pointer=0x0) at ../src/util-inl.h:223
#8  0x0000558f735b064e in node::ClearWrap (object=...) at ../src/util-inl.h:228
#9  0x0000558f7362f50f in node::http2::Http2Session::~Http2Session (this=0x558f77145fe0, __in_chrg=<optimized out>) at ../src/node_http2.cc:535
#10 0x0000558f7362f5d0 in node::http2::Http2Session::~Http2Session (this=0x558f77145fe0, __in_chrg=<optimized out>) at ../src/node_http2.cc:538
#11 0x0000558f7364e1c2 in node::BaseObject::WeakCallback<node::http2::Http2Session> (data=...) at ../src/base_object-inl.h:63
#12 0x0000558f73f34f0d in v8::internal::GlobalHandles::PendingPhantomCallback::Invoke (this=0x7fff11532300, isolate=0x558f770027b0) at ../deps/v8/src/global-handles.cc:859
#13 0x0000558f73f34c8e in v8::internal::GlobalHandles::DispatchPendingPhantomCallbacks (this=0x558f770226e0, synchronous_second_pass=true) at ../deps/v8/src/global-handles.cc:824
#14 0x0000558f73f35009 in v8::internal::GlobalHandles::PostGarbageCollectionProcessing (this=0x558f770226e0, collector=v8::internal::MARK_COMPACTOR, gc_callback_flags=v8::kGCCallbackFlagForced) at ../deps/v8/src/global-handles.cc:880
#15 0x0000558f73f5eeb1 in v8::internal::Heap::PerformGarbageCollection (this=0x558f770027d0, collector=v8::internal::MARK_COMPACTOR, gc_callback_flags=v8::kGCCallbackFlagForced) at ../deps/v8/src/heap/heap.cc:1636
#16 0x0000558f73f5d57d in v8::internal::Heap::CollectGarbage (this=0x558f770027d0, space=v8::internal::OLD_SPACE, gc_reason=v8::internal::GarbageCollectionReason::kTesting, gc_callback_flags=v8::kGCCallbackFlagForced) at ../deps/v8/src/heap/heap.cc:1257
#17 0x0000558f73f5ce85 in v8::internal::Heap::CollectAllGarbage (this=0x558f770027d0, flags=2, gc_reason=v8::internal::GarbageCollectionReason::kTesting, gc_callback_flags=v8::kGCCallbackFlagForced) at ../deps/v8/src/heap/heap.cc:1112
#18 0x0000558f738daa9d in v8::Isolate::RequestGarbageCollectionForTesting (this=0x558f770027b0, type=v8::Isolate::kFullGarbageCollection) at ../deps/v8/src/api.cc:8540
#19 0x0000558f73ee1d33 in v8::internal::GCExtension::GC (args=...) at ../deps/v8/src/extensions/gc-extension.cc:20
#20 0x0000558f73906534 in v8::internal::FunctionCallbackArguments::Call (this=0x7fff11532800, f=0x558f73ee1c8e <v8::internal::GCExtension::GC(v8::FunctionCallbackInfo<v8::Value> const&)>) at ../deps/v8/src/api-arguments.cc:25
#21 0x0000558f739c4d96 in v8::internal::(anonymous namespace)::HandleApiCallHelper<false> (isolate=0x558f770027b0, function=..., new_target=..., fun_data=..., receiver=..., args=...) at ../deps/v8/src/builtins/builtins-api.cc:112
#22 0x0000558f739c2dc9 in v8::internal::Builtin_Impl_HandleApiCall (args=..., isolate=0x558f770027b0) at ../deps/v8/src/builtins/builtins-api.cc:142
#23 0x0000558f739c2b63 in v8::internal::Builtin_HandleApiCall (args_length=5, args_object=0x7fff115329e8, isolate=0x558f770027b0) at ../deps/v8/src/builtins/builtins-api.cc:130

It sounds a lot like that’s #17840 resurfacing?

addaleax · 2018-02-13T19:15:25Z

I’m sorry, I don’t think I’ll able to look into the failures until later this week. @bnoordhuis Would you be okay with me landing #18676 before this one does? Resolving the issue you pointed out over there should be easy enough, and if you want I can do that too.

bnoordhuis · 2018-02-13T19:46:59Z

No problem, go merge. I'll rebase.

bnoordhuis · 2018-02-19T13:38:06Z

Forgot to push the fixes... CI: https://ci.nodejs.org/job/node-test-pull-request/13271/

Probably needs another review, the fixes are d1ac87b and f0a3019.

Replace v8::Persistent with node::Persistent, a specialization that resets the persistent handle on destruction. Prevents accidental resource leaks when forgetting to call .Reset() manually. I'm fairly confident this commit fixes a number of resource leaks that have gone undiagnosed so far. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

The previous commit made persistent handles auto-reset on destruction. This commit removes the Reset() calls that are now no longer necessary. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Don't try to update the internal field pointer of the JS object in the destructor. The garbage collector invokes the destructor when the object is collected and is not necessarily in a valid state anymore. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

bnoordhuis · 2018-02-21T14:31:13Z

Landed in e53275d...6bdc18c. Infrastructure issue on one of the ARM buildbots, otherwise green:

/usr/bin/ssh: error while loading shared libraries: libkrb5.so.3: cannot open shared object file: No such file or directory

I'm going to guess someone did an upgrade without rebooting.

MylesBorins · 2018-02-21T16:10:14Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Replace v8::Persistent with node::Persistent, a specialization that resets the persistent handle on destruction. Prevents accidental resource leaks when forgetting to call .Reset() manually. I'm fairly confident this commit fixes a number of resource leaks that have gone undiagnosed so far. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

The previous commit made persistent handles auto-reset on destruction. This commit removes the Reset() calls that are now no longer necessary. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Don't try to update the internal field pointer of the JS object in the destructor. The garbage collector invokes the destructor when the object is collected and is not necessarily in a valid state anymore. PR-URL: nodejs#18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Replace v8::Persistent with node::Persistent, a specialization that resets the persistent handle on destruction. Prevents accidental resource leaks when forgetting to call .Reset() manually. I'm fairly confident this commit fixes a number of resource leaks that have gone undiagnosed so far. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

The previous commit made persistent handles auto-reset on destruction. This commit removes the Reset() calls that are now no longer necessary. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Don't try to update the internal field pointer of the JS object in the destructor. The garbage collector invokes the destructor when the object is collected and is not necessarily in a valid state anymore. Backport-PR-URL: #19185 PR-URL: #18656 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>