deps: upgrade v8 to 4.1.0.21 #952
Conversation
|
You linked to the wrong bug number. ;) |
|
Last number got chopped off. Rectified. |
|
Now it doesn't work :) |
|
@bnoordhuis 403 |
|
That's what embargoed means. (I can't tell if you're joking or not.) |
|
Aaaah, I got it now :) LGTM, If it works. Is that revert commit actually a revert of that commit, or some improvisation? |
|
It's a clean revert safe for |
|
Ok, good to go then. |
|
Thanks, Fedor. I'll hold off on merging it until other TC members have had an opportunity to chime in. |
|
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? |
|
@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.) |
|
kk, sounds reasonable, lgtm |
|
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
force-pushed
the
upgrade-v8
branch
from
4f3e8cd
to
a558cd0
Compare
|
@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.
|
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. |
|
@domenic I think it is to be applied when 4.1 lands as stable? |
|
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. |
|
@domenic correct, I think that is the plan. @bnoordhuis? |
|
oh, I re-read it. Seems like the plan above is to hold off to 4.2.. |
|
I initially suggested 4.2 but I think 4.1 is also fine once it goes stable. |
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?