bundle v8 that is compatible with bundled node

This is process.versions in electron 1.2.0:

{
  "http_parser": "2.7.0",
  "node": "6.1.0",
  "v8": "5.1.281.47",
  "uv": "1.9.0",
  "zlib": "1.2.8",
  "ares": "1.10.1-DEV",
  "modules": "48",
  "openssl": "1.0.2h",
  "electron": "1.2.0",
  "atom-shell": "1.2.0",
  "chrome": "51.0.2704.63"
}

v8 5.1 is not abi compatible with 5.0, which node 6 ships with. So, .modules=48 is incorrect. The way to fix this (I guess) would be to bundle a chrome version that comes with v8 that is compatible with the bundled node version.

See nodejs/nan#578 for answers from node, nan and v8 maintainers.

We usually use the same version of NODE_MODULE_VERSION with Node, so NAN can work with Electron without modifications. But there hasn't been a Node release with V8 5.1, and NAN has not been prepared for V8 5.1, it is then not safe for us to change NODE_MODULE_VERSION on our side.

I think the only thing we can do is to wait for Node to release a new version that uses V8 5.1.

@0x00A

What about reverting the v8 upgrade then? The problem with the current situation that I and others like @0x00A are facing is that some native modules that do work with node don't work with electron. And I agree with you that it doesn't make much sense to just go ahead and make it abi compatible with node still being behind.

It's not a big deal for me personally to just use electron 1.1.3, but from that point of view you could consider 1.2.0 to be "broken".

@juliangruber

@juliangruber Did you rebuild them?

@paulatbipsync

@paulatbipsync yes, same results as with the prebuilt binaries

That's surprising, are you sure that your package isn't doing some kind of node-pre-gyp stuff where it's no-op'ing the build?

To me that's actually not surprising because v8 5.1 isn't abi compatible with v8 5.0, see nodejs/nan#578 (comment)

(And yes, I manually rebuilt and changed stuff around to verify it's not a problem with prebuilts)

Sure, but ABI doesn't matter if you're building from source

V8 5.1 is interesting in that it has only ABI breakages from 5.0 and it seems the Node team plans to apply an API compatibililty layer and merge V8 5.1 to Node 6.

What's the Electron plan to deal with this compatibility layer? NAN will most likely assume the layer is present as Node.js 6 will have it.

I feel as if some people on this thread are misusing ABI vs API breaking? Does everyone understand what an ABI-but-not-API breaking change is?

Like, this change breaks ABI but not API:

// In v1.0.0
struct Foo {
   int bar;
   char baz[32];
};

// In v1.0.1
struct Foo {
   char baz[32];
   int bar;
};

// In v1.0.0 bar is located at +0 from the beginning of the struct, in v1.0.1 this is at +0x20
// Compiling this code from source works in both v1.0.0 and v1.0.1, it is not an API break
// but it _is_ an ABI break.
myFoo->bar = 5;

Being a header-only library, NAN itself does not care about the ABI. As far as I know, there have been no significant API breakage in V8 5.1 either. Moreover, if NAN did not work, it would almost surely refuse to compile. Thus, NAN is almost surely compatible with V8 5.1. However, none of this applies when using prebuilt binaries of incompatible ABIs.

I am fairly certain that the problem here is ABI related and that the alleged rebuild does not properly build against the V8 version included in Electron, which for all I know could still be different from what is picked up by node-gyp operating on the bundled Node, which does not seem to use its bundled V8 under Electron. That the observed problem could not be reproduced on Windows, which does not support prebuild, points towards this.

There is no apparent reason why Nan::Null, which does nothing but forward to v8::Null would evaluate to true. It simply makes no sense, unless there is an ABI incompatibility. One possibility is that the attempts to rebuild the Node addon were built against the V8 version bundled in the bundled Node, which is not what Electron uses. It could also have been built against some other V8 version on the local file system. Who knows.

@kkoopa

@kkoopa i recompiled with a patch that returns a string instead of that Nan::Null and I got the string as expected, but if i recompiled with the original source that returns the Nan::Null, it was returning again true instead. Idk if that helps.

I haven't yet verified whether my rebuild process was using the v8 version electron bundles with as a target, but the problem already starts with electron advertising it's shipping with ABI 48, but breaking the 48 abi due to the newer v8 version.

@juliangruber

@juliangruber It does not bring any clarity to me.
The code in question hardly does anything.

inline v8::Local<v8::Primitive> Null() {
  return v8::Null(v8::Isolate::GetCurrent());
}

So the most likely source of error is indeed an ABI incompatibility.

v8::Null is defined here

Local<Primitive> Null(Isolate* isolate) {
  typedef internal::Object* S;
  typedef internal::Internals I;
  I::CheckInitialized(isolate);
  S* slot = I::GetRoot(isolate, I::kNullValueRootIndex);
  return Local<Primitive>(reinterpret_cast<Primitive*>(slot));
}

v8::True is defined here

Local<Boolean> True(Isolate* isolate) {
  typedef internal::Object* S;
  typedef internal::Internals I;
  I::CheckInitialized(isolate);
  S* slot = I::GetRoot(isolate, I::kTrueValueRootIndex);
  return Local<Boolean>(reinterpret_cast<Boolean*>(slot));
}

v8::Boolean is a v8::Primitive, so the only significant difference between the two functions is I::kNullValueRootIndex vs I::kTrueValueRootIndex.

In V8 5.0.71, we have

  static const int kNullValueRootIndex = 7;
  static const int kTrueValueRootIndex = 8;

In V8 5.1.281, we have

  static const int kNullValueRootIndex = 6;
  static const int kTrueValueRootIndex = 7;

So, the ABI incompatiblity is obvious: the precompiled addon, correctly using the ABI for V8 5.0.71 calls v8::Null, which returns the field at index 7. When using the wrong V8 version, like Electron does, this ends up returning true instead of null, since the ABI changed and true is now located at index 7.

src: update NODE_MODULE_VERSION to 51

and at that point the expectation would be that the V8 version no longer dictates the NODE_MODULE_VERSION. It would be best to avoid tight coupling between V8 version and NODE_MODULE_VERSION.

At the very least, there’s going to be a transition time in which many addons don’t use the new native API. It might make more sense to just start another counter that can be incremented specifically when the new ABI/API changes?

Support future Node and Electron versions

I'm not sure if it solves this issue, but Node 8 has this new thing:

The N-API is an API for building native addons. It is independent of the underlying JavaScript runtime and is maintained as part of Node.js itself. The goal of this project is to keep the Application Binary Interface (ABI) stable across different Node.js versions.

The purpose of N-API is to separate add-ons from changes in the underlying JavaScript engine so that native add-ons can run with different Node.js versions without recompilation.

https://nodejs.org/dist/latest-v8.x/docs/api/n-api.html

@zeke

@zeke the N-API is definitely the thing that will eventually help solve this. in 8.x it is behind a flag as it is experimental.

It is also worth mentioning that it cannot be relied on until userland modules can build on top of

edit: I shouldn't say can... they technically can and it works. I meant more that it needs to reach a critical mass

At present (after v1.8) V8 versions should be exactly the same for both Node.js and Chromium!

Navigation Menu

Repository navigation

bundle v8 that is compatible with bundled node #5851

54 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

bundle v8 that is compatible with bundled node #5851

Description

Activity

zcbenz commented on Jun 6, 2016

juliangruber commented on Jun 6, 2016

anaisbetts commented on Jun 6, 2016

juliangruber commented on Jun 6, 2016

anaisbetts commented on Jun 6, 2016

juliangruber commented on Jun 6, 2016

juliangruber commented on Jun 6, 2016

anaisbetts commented on Jun 6, 2016

mgol commented on Jun 7, 2016

anaisbetts commented on Jun 7, 2016

kkoopa commented on Jun 7, 2016

juliangruber commented on Jun 7, 2016

kkoopa commented on Jun 7, 2016

54 remaining items

addaleax commented on Sep 30, 2016

zeke commented on May 30, 2017

MylesBorins commented on May 30, 2017

codebytere commented on Sep 23, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions