Upgrade to V8 4.8 in master

[ofrobots]

V8 4.8 is now stable with the stable update of Chrome 48: http://googlechromereleases.blogspot.com/2016/01/stable-channel-update_20.html

Pick up V8 4.8 branch-head [ https://github.com/v8/v8/commit/fa163e2]. This branch brings in @@isConcatSpreadable,@@toPrimitive and ToLength ES6 changes. For full details see: http://v8project.blogspot.de/2015/11/v8-release-48.html

/cc @nodejs/v8

[trevnorris]

Seems SetHiddenValue and GetHiddenValue have been marked deprecate soon. Take care of that now, or worry about it later?

[cjihrig]

@trevnorris mind if I work on updating the uses in /src?

[trevnorris]

@cjihrig Be my guest. The usage is slightly different and doubt it'll change how we use it, but figured I'd mention it.

[rvagg]

FYI @nodejs/v8 I turned on nightlies for master recently, you can grab binaries from https://nodejs.org/download/nightly/ if you want to work on the cutting edge. They should work with native addons too as long as NAN is doing the right thing and the addons are coded properly. It'd be great to have some folks regularly using these.

I'm also working up a canary-like strategy and will post a strawman soon but wanted to get the nightlies on your radar before we get to that.

FWIW this is a script I regularly use to upgrade my Node install and it can also do nightlies: https://gist.github.com/rvagg/742f811be491a49ba0b9. Since I turned on master nightlies it defaults to those as the latest (currently it doesn't have a way to select the 5.x nightlies).

[bnoordhuis]

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/1340/

@indutny You should check if postmortem still works.

[evanlucas]

From a very basic test with llnode, seems to be working fine.

EDIT: Actually, I was using the nightly. Disregard my noise please

[ofrobots]

I also started a CI yesterday: https://ci.nodejs.org/job/node-test-pull-request/1330/. I will restart the arm-fanned subset, but it seems like they are failing on every build?

@rvagg thanks for setting up the nightlies. Looking forward to the canary strawman.

BTW, I have been using llnode for day-to-day development on V8 ToT and it seems to work okay, although I don't test every feature.

[ofrobots]

Restarted arm-fanned subset: https://ci.nodejs.org/job/node-test-commit-arm-fanned/1240/

[ofrobots]

The lone failure on arm is test-http-exit-delay.js which looks flaky as it is failing in other builds too: AssertionError: Entire test should take less than one second.

[rvagg]

There is a proposal to completely remove test-http-exit-delay: #4786

/cc @Trott

[ofrobots]

Landed as 5f6dfab...db9e122.

[cjihrig]

@trevnorris I started looking into SetPrivate() and GetPrivate(), the new alternatives to SetHiddenValue() and GetHiddenValue(). Seems like they work like symbols now. From what I can tell, this will cause problems if you try to retrieve a hidden value without access to the original Local<Private>. In the past you could just provide the same string anywhere in the program. Hopefully I'm wrong. If not, I guess we could use Private::ForApi, which are global symbols. Thoughts?

[bnoordhuis]

v8::Private is kind of annoying because it's not a v8::Value; you can't export it to JS land. It's doubly annoying because under the hood it's just a v8::Symbol, which is a v8::Value.

One possible workaround is to export numeric ids that map to v8::Private handles in C++ land and pass those to .getHiddenValue() and .setHiddenValue().

[trevnorris]

@bnoordhuis when you say it is a v8::Symbol does that mean it can be reinterpret_cast'd, or just that they're similar? (doing this from phone, will refresh on the API later)

[bnoordhuis]

Yes, it's literally a reinterpret_casted v8::Symbol. We could cast it back but that's relying on an implementation detail, of course.

[domenic]

Please don't do that. The existence of native node packages that did that reinterpret_cast was part of the reason v8::Private was removed in the 4.5 (IIRC) timeframe. It is only back now because the need in Blink became more urgent, but that kind of abuse will cause complications...

In the browser, it's imperative for security reasons that private symbols never be exposed to JS code. In Node, Node core and/or any native modules are already running native code on your machine, so you are already in not-great-shape if they want to do something malicious; I doubt the security argument applies. But it still breaks a bunch of invariants V8 relies on to maintain a consistent internal state.

[cjihrig]

Is there any reason we shouldn't use the Private::ForApi functionality? It seems like the implementation would be much simpler. We wouldn't want to ever expose it as public API, as the global symbols are never collected, but for our internal use it should be fine (we currently only have a handful of unique hidden values).

[trevnorris]

@domenic

Please don't do that. The existence of native node packages that did that reinterpret_cast was part of the reason v8::Private was removed in the 4.5 (IIRC) timeframe.

Heh. Didn't think we should, just wanted to better understand the scope of the problem. :)

[vkurchatkin]

In the browser, it's imperative for security reasons that private symbols never be exposed to JS code.

I thought Privates are separate from v8 internal private symbols

The existence of native node packages that did that reinterpret_cast was part of the reason v8::Private was removed

Wow. I guess it was me: https://github.com/vkurchatkin/private-symbol )

But it still breaks a bunch of invariants V8 relies on to maintain a consistent internal state

Why? They are exposed to C++ code so the still can be used, but API would be uglier.

[bnoordhuis]

#5045