Fix Electron 3 ABI

[bendemboski]

Electron 3's ABI was incorrectly specified as 64 instead of 59, and now that Electron 4 has shipped and actually has ABI version 64, this is causing prebuild/prebuild-install to try to share binaries between Electron 3 and 4 which doesn't work.

Fixes #54

[MarshallOfSound]

The ABI of Electron 3 is 64, you can verify this with electron --abi with Electron 3.x installed. The thing is, although the node team are very careful with ABI compatibiility, between Electron 3 and Electron 4 we switched from GYP to GN and OpenSSL to BoringSSL (amongst other things) in such a way that the builds probably aren't 100% compatible.

Their are solutions to this being worked on upstream so that Electron can reserve ABI values independently of Node (along with other channels of distribution).

nodejs/TSC#621

Not sure what the best solution is for this right now but this isn't it.

[bendemboski]

Hmm...does this tag not actually correspond to the 3.0.0 release then? I see that Electron 3.0.0 reports Node 10.2.0, even though the 3.0.0 tag lists a 9.7 version in DEPS.

Regardless, I'm pretty sure that the actual ABI differs between Electron 3 and Electron 4. Here is a keytar.node binary built out of the master of node-keytar by running ./node_modules/.bin/prebuild -t 3.0.0 -r electron:

keytar.node.zip

Electron 3 can load it, but Electron 4 can't, and fails with

dyld: lazy symbol binding failed: Symbol not found: __ZN2v816FunctionTemplate3NewEPNS_7IsolateEPFvRKNS_20FunctionCallbackInfoINS_5ValueEEEENS_5LocalIS4_EENSA_INS_9SignatureEEEiNS_19ConstructorBehaviorE
  Referenced from: /private/tmp/elec4/node_modules/keytar/build/Release/keytar.node
  Expected in: flat namespace

dyld: Symbol not found: __ZN2v816FunctionTemplate3NewEPNS_7IsolateEPFvRKNS_20FunctionCallbackInfoINS_5ValueEEEENS_5LocalIS4_EENSA_INS_9SignatureEEEiNS_19ConstructorBehaviorE
  Referenced from: /private/tmp/elec4/node_modules/keytar/build/Release/keytar.node
  Expected in: flat namespace

A symbol dump includes

U v8::FunctionTemplate::New(v8::Isolate*, void (*)(v8::FunctionCallbackInfo<v8::Value> const&), v8::Local<v8::Value>, v8::Local<v8::Signature>, int, v8::ConstructorBehavior)

Since Electron 3 can load it, its v8 must not include these changes to the FunctionTemplate::New signature (among others), which was first introduced in v8 6.7, but was patched into Node 10.0, even though it (Electron 3) reports Node 10.2 as its Node version.

Node 10.2 (running independent of Electron), on the other hand, cannot load this binary, and fails with the same error as Electron 4.

So I'm not entirely sure what's going on here, but I'm pretty sure Electron 3 and 4 do not have ABI-compatible versions of v8::FunctionTemplate::New, and I'm also pretty sure that Electron 3's v8 is binary-incompatible with Node 10.2's even though Electron 3 includes Node 10.2.

The end result is that binaries built for Electron 3 that reference any of those symbols that had a SideEffectType argument added fail to load in Electron 4 even though the two report the same ABI.

So I'm seeing that this isn't really a bug in node-abi because it's just report what Electron reports ABI-wise -- perhaps this is a bug in Electron itself?

[bendemboski]

The result being that I think all packages using prebuild cannot be consumed in Electron 4 projects without forcing a local rebuild -- see here.

[MarshallOfSound]

@bendemboski We changed build systems from 3.0 to 4.0

The node used in the GYP build is in the vendor/node directory. You can see this for 3.0.0 here and can see that it is 10.2

https://github.com/electron/node/tree/3a619a78bcbf105f6c7d48a2eeeda919111fbc0f

As mentioned above there may be an incompatibility and there is active work upstream to determine how best to handle downstream distributions of node and their ABI compatibility. This issue is not a bug in Electron or node-abi rather a fundamental issue with how ABI numbers are currently chosen/declared. Once a solution is chosen for that we will be able to declare our own ABI numbers.

[MarshallOfSound]

Requesting changes to prevent merge

[bendemboski]

@MarshallOfSound thanks for taking the time to explain this to me -- I'm new to chasing around Electron's Node/v8 dependencies and ABI versions, and it seems to be a kinda steep learning curve 😄

The core issue that prebuilt packages such as keytar cannot run against Electron 4 because Electron 3 and 4 have different binary interfaces is starting to cause widespread problems as people upgrade to Electron 4 (see here here here, maybe here) and I think preventing Electron 4 adoption.

Can you point me to where the discussions are happening so I can track the status of a fix to these issues, or suggest where else I might raise this issue?

[MarshallOfSound]

@bendemboski We've raised an issue with the Node.JS TSC to get some unique ABI numbers for Electron so that we can assign a new Electron specific ABI to Electron 4. We'll update this PR / issue with information as we figure it out.

🔗 nodejs/TSC#651

[bendemboski]

Great, thanks, I'll follow that issue for updates.

[MarshallOfSound]

Electron 4.0.4 now has a dedicated ABI number of 69.

Refs: #57
Refs: electron/node@8bc5d17