deps: upgrade v8 to 4.1.0.21 by bnoordhuis · Pull Request #952 · nodejs/node

bnoordhuis · 2015-02-25T11:31:55Z

R=@indutny, /cc @iojs/tc

This upgrade is an interesting conundrum. There is an incompatible ABI (but not API) change in here that would require bumping NODE_MODULE_VERSION and NODE_MINOR_VERSION.

However, skipping the upgrade is probably a bad idea because it also contains a fix for https://code.google.com/p/chromium/issues/detail?id=430201, which annoyingly is still under embargo but looks like it's a use-after-free and may be exploitable.

As a proposed fix, I backed out the ABI change. WDYT?

Havvy · 2015-02-25T13:52:19Z

You linked to the wrong bug number. ;)

bnoordhuis · 2015-02-25T14:05:11Z

Last number got chopped off. Rectified.

indutny · 2015-02-25T14:06:12Z

Now it doesn't work :)

indutny · 2015-02-25T14:06:17Z

@bnoordhuis 403

Havvy · 2015-02-25T14:15:17Z

That's what embargoed means.

(I can't tell if you're joking or not.)

indutny · 2015-02-25T14:26:41Z

Aaaah, I got it now :) LGTM, If it works. Is that revert commit actually a revert of that commit, or some improvisation?

bnoordhuis · 2015-02-25T14:38:04Z

It's a clean revert safe for --exclude=deps/v8/src/version.cc.

indutny · 2015-02-25T14:45:46Z

Ok, good to go then.

bnoordhuis · 2015-02-25T14:57:19Z

Thanks, Fedor. I'll hold off on merging it until other TC members have had an opportunity to chime in.

rvagg · 2015-02-25T16:11:00Z

4f3e8cdd8d7 is kind of large, what's the strategy with that going forward? It's going to have to land at some point anyway, why not now? Do we just need to light a fire under NAN and get a patch out for this?

bnoordhuis · 2015-02-25T16:29:05Z

@rvagg The motivation for the revert patch is to hold off until we upgrade to 4.2 because then we'll be bumping NODE_MODULE_VERSION anyway.

I don't think nan needs to do anything here. The issue is that the size of the struct changed, which makes it an ABI change (compiled code will need to be recompiled) but not an API change (no source code changes are necessary.)

rvagg · 2015-02-25T17:41:39Z

kk, sounds reasonable, lgtm

rvagg · 2015-02-25T17:59:52Z

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/196/

rvagg · 2015-02-25T18:30:32Z

CI is happy enough, @bnoordhuis are you around to merge this? if you don't respond soon I'll go ahead and do this. Will list "embargoed fix, details available at https://code.google.com/p/chromium/issues/detail?id=430201 when embargo is lifted" in the notable changes in 1.4.0

PR-URL: nodejs#952 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Rod Vagg <rod@vagg.org>

Fix: nodejs#461 PR-URL: nodejs#706 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Undo the ABI (but not API) change to NamedPropertyHandlerConfiguration. This avoids a NODE_MODULE_VERSION bump and forcing everyone to recompile their add-ons, at the cost of increasing the delta with upstream V8. This commit effectively backs out 4.1.0.16, the release that introduced the ABI change (and nothing else.) PR-URL: nodejs#952 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Rod Vagg <rod@vagg.org>

bnoordhuis · 2015-02-25T18:36:09Z

@rvagg Landed in 78f4837...a558cd0.

Notable changes: * process / promises: An'unhandledRejection' event is now emitted on process whenever a Promise is rejected and no error handler is attached to the Promise within a turn of the event loop. A 'rejectionHandled' event is now emitted whenever a Promise was rejected and an error handler was attached to it later than after an event loop turn. See the process documentation for more detail. #758 (Petka Antonov) * streams: you can now use regular streams as an underlying socket for tls.connect() #758 (Fedor Indutny) * V8: Upgrade V8 to 4.1.0.21. Includes an embargoed fix, details should be available at https://code.google.com/p/chromium/issues/detail?id=430201 when embargo is lifted. A breaking ABI change has been held back from this upgrade, possibly to be included when io.js merges V8 4.2. See #952 for discussion. * npm: Upgrade npm to 2.6.0. Includes features to support the new registry and to prepare for npm@3. See npm CHANGELOG.md https://github.com/npm/npm/blob/master/CHANGELOG.md#v260-2015-02-12 for details. * libuv: Upgrade to 1.4.1. See libuv ChangeLog https://github.com/libuv/libuv/blob/v1.x/ChangeLog for details of fixes.

Notable changes: * process / promises: An'unhandledRejection' event is now emitted on process whenever a Promise is rejected and no error handler is attached to the Promise within a turn of the event loop. A 'rejectionHandled' event is now emitted whenever a Promise was rejected and an error handler was attached to it later than after an event loop turn. See the process documentation for more detail. #758 (Petka Antonov) * streams: you can now use regular streams as an underlying socket for tls.connect() #758 (Fedor Indutny) * http: A new 'abort' event emitted when a http.ClientRequest is aborted by the client. #945 (Evan Lucas) * V8: Upgrade V8 to 4.1.0.21. Includes an embargoed fix, details should be available at https://code.google.com/p/chromium/issues/detail?id=430201 when embargo is lifted. A breaking ABI change has been held back from this upgrade, possibly to be included when io.js merges V8 4.2. See #952 for discussion. * npm: Upgrade npm to 2.6.0. Includes features to support the new registry and to prepare for npm@3. See npm CHANGELOG.md https://github.com/npm/npm/blob/master/CHANGELOG.md#v260-2015-02-12 for details. * libuv: Upgrade to 1.4.2. See libuv ChangeLog https://github.com/libuv/libuv/blob/v1.x/ChangeLog for details of fixes.

domenic · 2015-02-26T22:58:13Z

I think the revert is unfortunate. We should be able to accept ABI changes in minor releases, shouldn't we?

We won't be getting 4.2 for ~8 weeks so this is going to be a floating revert for a long time.

And I can't really be sure but the added functionality seems potentially useful for some vm-related stuff.

Fishrock123 · 2015-02-26T22:59:10Z

@domenic I think it is to be applied when 4.1 lands as stable?

domenic · 2015-02-26T23:00:23Z

One more thing: I feel like taking these kinds of changes is what we signed up for when we decided to take 4.1 even though it wasn't in Chrome stable yet. Our fault, and we have to own it.

@Fishrock123 are you suggesting that we'd float the revert for 2 weeks, until 4.1 reaches Chrome stable, and then un-revert? That's better than 8 weeks at least.

Fishrock123 · 2015-02-27T21:57:17Z

@domenic correct, I think that is the plan. @bnoordhuis?

Fishrock123 · 2015-02-27T22:01:30Z

oh, I re-read it. Seems like the plan above is to hold off to 4.2..

bnoordhuis · 2015-03-03T15:53:57Z

I initially suggested 4.2 but I think 4.1 is also fine once it goes stable.

bnoordhuisand others added 3 commits February 25, 2015 19:33

deps: upgrade v8 to 4.1.0.21
78f4837
PR-URL: nodejs#952 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Rod Vagg <rod@vagg.org>

deps: fix postmortem support in v8
54532a9
Fix: nodejs#461 PR-URL: nodejs#706 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

bnoordhuis force-pushed the upgrade-v8 branch from 4f3e8cd to a558cd0Compare February 25, 2015 18:35

bnoordhuis merged commit a558cd0 into nodejs:v1.xFeb 25, 2015

bnoordhuis deleted the upgrade-v8 branch February 25, 2015 18:35

rvagg mentioned this pull request Feb 25, 2015
Release proposal: v1.4.0 #932
Closed

popomore mentioned this pull request Feb 27, 2015
1.4.1 translate nodejs/nodejs-zh-CN#119
Merged

bnb mentioned this pull request Feb 27, 2015
Evangelism: Weekly Update Feb 27th nodejs/evangelism#4
Closed

yosuke-furukawa mentioned this pull request Feb 28, 2015
io.js weekly translation 2015-02-27 nodejs/nodejs-ja#42
Merged

domenic mentioned this pull request Mar 3, 2015
RFC: upgrade to V8 4.2? #1026
Closed

chrisdickinson mentioned this pull request Mar 4, 2015
Major version bump #1061
Closed

springmeyer mentioned this pull request Mar 6, 2015
NODE_MODULE_VERSION for io.js mapbox/node-pre-gyp#150
Closed

This was referenced Mar 8, 2015
RFC: Binary Coverage Matrix sass/node-sass#655
Closed
Better support for io.js rapid release cycle sass/node-sass#694
Closed

Uh oh!

Uh oh!

Conversation

bnoordhuis commented Feb 25, 2015

Uh oh!

Havvy commented Feb 25, 2015

Uh oh!

bnoordhuis commented Feb 25, 2015

Uh oh!

indutny commented Feb 25, 2015

Uh oh!

indutny commented Feb 25, 2015

Uh oh!

Havvy commented Feb 25, 2015

Uh oh!

indutny commented Feb 25, 2015

Uh oh!

bnoordhuis commented Feb 25, 2015

Uh oh!

indutny commented Feb 25, 2015

Uh oh!

bnoordhuis commented Feb 25, 2015

Uh oh!

rvagg commented Feb 25, 2015

Uh oh!

bnoordhuis commented Feb 25, 2015

Uh oh!

rvagg commented Feb 25, 2015

Uh oh!

rvagg commented Feb 25, 2015

Uh oh!

rvagg commented Feb 25, 2015

Uh oh!

bnoordhuis commented Feb 25, 2015

Uh oh!

domenic commented Feb 26, 2015

Uh oh!

Fishrock123 commented Feb 26, 2015

Uh oh!

domenic commented Feb 26, 2015

Uh oh!

Fishrock123 commented Feb 27, 2015

Uh oh!

Fishrock123 commented Feb 27, 2015

Uh oh!

bnoordhuis commented Mar 3, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants