Possible crash related to Weak refs

[TomMettam]

• Version: 12.16.3
• Platform: Windows / Linux

I've created a simple test case to reproduce this issue. I'm not certain if it's a bug in my code, Nan, V8 or Node.

https://github.com/CasperTech/NanWeakFunctionTest

How often does it reproduce? Is there a required condition?

100% reproduction - tested on Windows and Linux

What do you see instead?

The test app registers 10,000 callbacks, then clears them and forces a GC clear, then registers them again, every 5 seconds. After the second or third run the app crashes:

Thread 1 "node" received signal SIGSEGV, Segmentation fault.
0x0000555555ddb0a7 in v8::internal::GlobalHandles::Create(v8::internal::Object) ()
(gdb) bt
#0  0x0000555555ddb0a7 in v8::internal::GlobalHandles::Create(v8::internal::Object) ()
#1  0x0000555555c90a27 in v8::V8::GlobalizeReference(v8::internal::Isolate*, unsigned long*) ()
#2  0x00007ffff46d0fd0 in TestObject::CreateCallback(Nan::FunctionCallbackInfo<v8::Value> const&) ()
   from NanWeakFunctionTest/build/Release/NanWeakFunctionTest.node
#3  0x00007ffff46cf8a7 in Nan::imp::FunctionCallbackWrapper(v8::FunctionCallbackInfo<v8::Value> const&) ()
   from NanWeakFunctionTest/build/Release/NanWeakFunctionTest.node
#4  0x00005555563daf0d in ?? ()
#5  0x00007fffffffd880 in ?? ()
#6  0x00007fffffffd8b8 in ?? ()

After the second global.gc, not all the Weak references are released, but I understand that GC is unpredictable so this may or may not be related.

Process memory usage only rises to about ~15mb on my system and the system has plenty to spare.

[TomMettam]

( This also looks very similar to nodejs/node-addon-api#393 )

[addaleax]

I think the problem here is not Node, it’s Nan – the callbacks[callbackID]->Reset(); call happens in a second-pass callback, where calling .Reset() on the persistent that caused it to happen does not appear to be valid. It’s also not necessary, because Nan resets the persistent for you in the first-pass callback (which is where the .Reset() should happen). It’s a bit unfortunate that Nan diverges from V8 here, but I don’t think there’s anything that can be done about that.

[TomMettam]

Are you saying that my call to callbacks[callbackID]->Reset(); is superfluous in the callbackDestroyed method? At what stage does the persistent get automatically reset?

[addaleax]

@TomMettam Yes, it’s what’s causing the crash and it’s not needed if you’re using Nan (but unfortunately only when you’re using Nan).

At what stage does the persistent get automatically reset?

That would be https://github.com/nodejs/nan/blob/eaae2683a3df542376797a11385b87b9c0d071c5/nan_weak.h#L120

[TomMettam]

Alright! Thank you very much for your time, I appreciate it!