broken on Node.js 13

[gengjiawen]

Full log: https://travis-ci.com/gengjiawen/ref-napi/jobs/283269452

[addaleax]

Yeah, I’ve been thinking about this. I think the only reasonable solution here is to create a maximum-size ArrayBuffer over a large memory region and then use slices of that ArrayBuffer as the Buffers returned by ref-napi.

[devoto13]

Not sure if this is related to the linked Travis log, but it also crashes with segfault on garbage collection. E.g. given test.js

require('ref-napi');
global.gc();

$ node --version
v13.4.0
$ node --expose-gc test.js
[1]    21419 segmentation fault  node --expose-gc test.js

Also below is a stack trace I received using segfault-handler in case it is helpful somehow:

PID 21501 received SIGSEGV for address: 0xa118
0   segfault-handler.node               0x00000001041e2066 _ZL16segfault_handleriP9__siginfoPv + 310
1   libsystem_platform.dylib            0x00007fff6d96f42d _sigtramp + 29
2   ???                                 0x00007ffeefbff898 0x0 + 140732920756376
3   node                                0x000000010006024b _ZN4node11Environment27NativeImmediateCallbackImplIZN6v8impl12_GLOBAL__N_115BufferFinalizer22FinalizeBufferCallbackEPcPvEUlPS0_E_E4CallES7_ + 49
4   node                                0x000000010003ae06 _ZN4node11Environment27RunAndClearNativeImmediatesEb + 218
5   node                                0x000000010003ac17 _ZN4node11Environment14CleanupHandlesEv + 53
6   node                                0x000000010003b064 _ZN4node11Environment10RunCleanupEv + 192
7   node                                0x00000001000b3fc0 _ZN4node16NodeMainInstance3RunEv + 572
8   node                                0x000000010005e0d7 _ZN4node5StartEiPPc + 263
9   libdyld.dylib                       0x00007fff6d7767fd start + 1

[addaleax]

@devoto13 I think that particular bug has been fixed in more recent versions of Node.js (by nodejs/node#31140), but it’s hard to tell without a reproduction.

[devoto13]

Thanks for the pointer, @addaleax. It reproduces neither with the script from my previous message, nor the real application using Node 13.8.0.

[onichandame]

I have problem installing ffi-napi and ref-napi as dependencies with node 13.12.0. tested on centos8 and windows 10. Anyone seeing the same problem?
Package ffi and ref also have similar compatibility problems on the newer node. I am quite curious why it is difficult to make these packages independent of the node version?

[addaleax]

@onichandame In this case, there are problems because V8 makes changes that break the way that this addon works. I’ll do my best to get on top of that, but it’s going to be quite a bit of work.

[onichandame]

@addaleax Would you kindly point me to the exact changes in V8 that possibly have caused this problem?
I may have time to contribute in the near future so I want a starting point to look at.

[addaleax]

@onichandame https://bugs.chromium.org/p/v8/issues/detail?id=9908 might be a starting point, there’s quite a few referenced V8 changes – basically, the whole BackingStore migration is problematic … I’m currently trying to fix this, btw, so there may not be a need to fix this for others.

That doesn’t mean that this codebase is in a great state and couldn’t use some work, though… 😄

[onichandame]

@addaleax thx for your great work! I will take a look anyways as the problems of the other repo like ref and node-ffi may have a similar root cause.

[addaleax]

Yeah, fixing ref-napi is part of that plan… I actually have that part working by now but it’s quite tightly integrated with the fixes for the ffi-napi module itself, so I’ll push/release them together

It’s also going to be very hacky, so I’m definitely open to people thinking about other solutions. Basically, what I’m doing right now is adding a list of all ArrayBuffers passed to or returned from ref-napi and ffi-napi, indexed by their base pointers, in order to avoid creating duplicate Buffer instances for the same base pointer (because that is basically what V8 has started forbidding now)…

[addaleax]

@onichandame Fwiw, #32 should have resolved this for ref-napi, and node-ffi-napi/node-ffi-napi#68 should do that for node-ffi-napi … I’m still having trouble getting everything to work 100 % in CI – and either way, feel free to try to newer branches/releases out

[onichandame]

@addaleax I just retried to install ffi-napi and ref-napi on centos 7 with both node 13.13.0 and 14.0.0 using yarn add ref-napi ffi-napi. It just worked! Thanks!

FYI, here are the commands I ran for the quick test:

podman pull onichandame/docker-dev:node13 # or node14
podman run -it --rm onichandame/docker-dev:node13
yarn add ffi-napi ref-napi

[addaleax]

This should be fixed in node-ffi-napi@3.0.0, please feel free to try it out everybody!