Segfault in node 12

[mhofman]

Not sure what's going on, but in Node 12, I'm getting some segfaults. I've traced it to ObjectInfo.target calls after the target has been collected, but it doesn't seem to always trigger.

To reproduce, just run the test suite of your package, or npm run test:node of https://github.com/mhofman/tc39-weakrefs-shim

[addaleax]

Hi! Thanks, I’ve not forgotten your other issue either, but I’ll try to look into this sooner rather than later. The native addon part isn’t particularly complex, so this might well be a bug in Node.

[mhofman]

Yeah I looked at the source of the native addon and I really don't see why it would segfault. My C++ development days are way in the past so I couldn't easily troubleshoot further.

[mhofman]

Interestingly, the tests in this package also fail on node 11, but the tests in mine work fine on node 11. Maybe there are 2 different segfaults triggers.

[addaleax]

Interesting! Sorry it took me a while to get to it, but this looks like it’s a bug in the N-API C++ convenience wrapper. I’ll post a fix shortly :)

Edit: nodejs/node-addon-api#475

[VoltanaDMG]

The provided solution by @addaleax works fine for Node 10.x and higher versions (tested it for Node v10.x, v11.x, v12.x). But the solution breaks the tests of the weak-napi module for Node v9.x and lower.
I got the following error for test case create():
*** Error in '/home/travis/.nvs/node/9.11.2/x64/bin/node': double free or corruption (fasttop): 0x000000000305edf0 ***

Here is a screenshot of test run for Node v9.x:

[addaleax]

@VoltanaDMG Fascinating … I’ll take a look.

[addaleax]

I think this is happening because nodejs/node#24494 isn’t on v8.x. That’s not ideal, and I’ll have to think about whether/how to work around that.

[VoltanaDMG]

Hm, this isn't good, a lot of people and companies are still using v8.x.x.
I will look into it as well, because there must be solution for mainline versions.

Edit: Fixed some typos 🤣

[mhdawson]

From what I remember nodejs/node#24494 was not backported to 8.x because the original issue seemed to occur in 10.x but not 8.x (I think the code might have been different too). We may want to consider backporting if it affects 8.x as well.

[addaleax]

@mhdawson The code appeared to be the same to me when looking into this; so it should be affected too. In general, it seems like a bad idea to me to backport only some N-API changes.

That being said, backporting to v8.x wouldn’t affect already-released versions, so it doesn’t really solve the problem – from the looks of it, some versions of N-API want one thing, some versions of N-API want another thing and there’s no way to detect which ones want what. That’s just kind of fundamentally bad for a stable API like N-API.

It might make sense to try and figure out what caused this to fail in v12.x, and if possible try to undo that. That would still leave the C++ wrapper API broken until a new major release comes out, but would fix this particular bug and I guess that’s just something we may have to live with.

[mhdawson]

If the code looks the same then I must have remembered wrong. In that case we should look at backporting.

A bug in the C++ wrapper which only affects some versions (or by bad luck on shows up in some versions even though it could affect any of them) is painful. It would be good to understand if there is something different in 12.x that caused this to surface or its simply different timing.

If it was that `some versions of N-API want one thing, some versions of N-API want another thing' then the N-API version might help (although that should not be the case anyway since versions should be adding, not changing behaviour). Instead, I think what you are saying is that different versions of Node.js with the same N-API version seem to want something different. The fundamental problem seems to be that the fix for what we see in one version makes the problem much more visible in another.

In terms of backporting it not fixing the problem in earlier versions that is sometimes going to be the case and I'm not sure how we can avoid needing an upgrade to the latest v8.x if an application hits that bug unless we can work around it in the wrapper or calling code.

[mhdawson]

@addaleax if you want me to start putting together a backport for 8.x just let me know.

[addaleax]

Actually, from bisecting it looks like nodejs/node#24494 also introduced the crash here in the first place… I guess I’ll have to dig a bit more