v8 5.1.281.47 issue with Nan::Null()

[juliangruber]

Running electron 1.2.0, which bundles v8 5.1.281.47 (node 6.2.0 bundles v8 5.0.71.47), i hit an issue where this code from leveldown (https://github.com/Level/leveldown/blob/master/src/database_async.cc#L141):

  v8::Local<v8::Value> argv[] = {
      Nan::Null()
    , returnValue
  };
  callback->Call(2, argv);

calls the js callback with a boolean true as the first parameter.

Steps to reproduce:

create app.js with:

var db = require('level')('/tmp/db');
db.db.put('foo', 'bar', console.log.bind(console));
db.db.get('foo', err => console.log('err', err));

then

$ nvm use 6
$ npm install electron-prebuilt@1.2.0 level
$ node_modules/.bin/electron app.js

It logs err true, although that argument should be null.

This error does not happen with electron 1.1.3, which is bundling v8 5.0.71.48.

[mathiask88]

Can't reproduce on windows. Did you run electron-rebuild?

[juliangruber]

@mathiask88 sorry, my reproduction steps were missing a few details. can you try again?

[mathiask88]

@juliangruber I meant that I could reproduce your code but not the error :) I ran the electron-starter repository with electron-prebuild 1.2.0 and level 1.4.0 and I get null. bar

[juliangruber]

@mathiask88 hm, then it looks like this issue doesn't appear on windows I'd say

[mathiask88]

@juliangruber Started my Ubuntu VM and it seems that electron-rebuild does not rebuild leveldown even if I use the --force argument. electron-rebuild does not have a verbose option. No NAN issue in my opinion. Maybe an error with prebuild? Because for windows there are no prebuilds available and it is necessary to compile.

[juliangruber]

@mathiask88 the problem persists when i manually rebuild leveldown. you shouldn't need to rebuild leveldown though if you use node 6 when installing, as both node 6 and electron 1.1.3/1.2.0 have abi 48.

my problem literally disappears by issuing npm install electron-prebuilt@1.1.3 and rerunning app.js.

[ofrobots]

V8 5.1 is not ABI compatible with V8 5.0. Electron should not be shipping
this with ABI 48.
On May 31, 2016 6:35 AM, "Julian Gruber" notifications@github.com wrote:

@mathiask88 https://github.com/mathiask88 the problem persists when i
manually rebuild leveldown. you shouldn't need to rebuild leveldown though
if you use node 6 when installing, as both node 6 and electron 1.1.3/1.2.0
have abi 48.

my problem literally disappears by issuing npm install
electron-prebuilt@1.1.3 and rerunning app.js.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#578 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAE0qelTMBzhxn1t6anBjKnbKNdP8YsUks5qHDkEgaJpZM4IqeQu
.

[ofrobots]

@juliangruber Can you try this patch on top of the version of V8 5.1 included by electron: ofrobots/node#23 and see if that solves the problem?

[heapwolf]

Seems like this issue breaks most of the level projects. /cc @rvagg

[rvagg]

as both node 6 and electron 1.1.3/1.2.0 have abi 48

Wait, what?? they are shipping 5.1 and haven't changed ABI, I thought they went with a >100 number cause they were doing things differently.

[rvagg]

as both node 6 and electron 1.1.3/1.2.0 have abi 48

This is the V8 that's linked to the 1.2.0 Electron tag (via node) https://github.com/electron/node/blob/d4528c219df8f442d769bae054883e1af79f105e/deps/v8/include/v8-version.h and it says 5.0. How are you determining that you have 5.1 @juliangruber?

[juliangruber]

@rvagg at least that's what process.versions says:

{
  "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"
}

[kkoopa]

Because it does not build Node against the bundled V8, but downloads whatever Chromium was built with and links against that. All of this is rather messed up, to say the least, but none of it pertains to NAN, so I am closing this as invalid.