-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
deps: update v8 to 4.4.63.26 #2220
Conversation
Rubberstamp LGTM. |
I'd love a way to verify that what's in tree is what's upstream, a |
@rvagg I just tried your suggestion and it works :) |
@rvagg that's precisely what I verified before my LGTM. This PR seems to be the equivalent to |
@@ -32,7 +32,7 @@ | |||
# library. | |||
|
|||
import os, re, sys, string | |||
import optparse | |||
import argparse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Centos CI boxes are failing because of this. I suspect the Centos dists we are using use Python 2.6, while argparse
was added in Python 2.7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @nodejs/build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous conversation about this here: #2022 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been fixed with https://crrev.com/44bc918458481d60b08d5566f0f31a79e39b85d7 but not backported to 4.4. I will include the patch tomorrow.
Thanks @ofrobots.
I cherry-picked the argparse fix. |
af162c8 lgtm |
Includes cherry-picks for: * JitCodeEvent patch: https://crrev.com/f7969b1d5a55e66237221a463daf422ac7611788 * argparse patch: https://crrev.com/44bc918458481d60b08d5566f0f31a79e39b85d7
Sorry for the delay, Chrome jumped to 4.4.63.25 so I updated the PR. |
LGTM. The last two regress-crbug tests I missed on the last upgrade, mea culpa. @targos Maybe you can run the CI one more time? The test-vm-debug-context.js failure on the pi1-raspbian-wheezy is probably a flake but it would be good to have that confirmed. |
@bnoordhuis test-vm-debug-context.js failed again. |
Maybe someone from @nodejs/build can give you temp access? |
@rvagg gave me access and I could successfully compile but after that I lost control of the machine and I cannot connect anymore. |
I wonder if it would be possible to reproduce the failure on an QEmu ARM machine. Some instructions here. |
it turns out that ssh tunnel from that machine could only handle one connection, after which it fails to properly accept additional connections, I've set up a tunnel using a different machine to do the same thing and it should be fine .. I hope |
I'm connected, it is fine, thank you. |
I've tracked the issue to the following part of the test: var vm = require('vm');
var assert = require('assert');
// See https://github.com/nodejs/io.js/issues/1190, accessing named interceptors
// and accessors inside a debug event listener should not crash.
(function() {
var Debug = vm.runInDebugContext('Debug');
var breaks = 0;
function ondebugevent(evt, exc) {
if (evt !== Debug.DebugEvent.Break) return;
exc.frame(0).evaluate('process.env').properties(); // Named interceptor.
exc.frame(0).evaluate('process.title').getTruncatedValue(); // Accessor.
breaks += 1;
}
function breakpoint() {
debugger;
}
assert.equal(breaks, 0);
Debug.setListener(ondebugevent);
assert.equal(breaks, 0);
breakpoint();
assert.equal(breaks, 1);
})(); Crash happens when The way the process crashes is variable. Here are 3 cases I had:
|
Here is a very minimal test case: var vm = require('vm');
var Debug = vm.runInDebugContext('Debug');
function breakpoint() {
debugger;
}
Debug.setListener(function(){console.log('debug event')});
console.log('before break');
breakpoint();
console.log('after break'); Output:
The crash happens outside of my script |
Long shot: does it work when you pass |
@bnoordhuis yes it does ! |
I'm sorry I don't know what to do now, it is beyond my understanding of v8 |
You could disable it in src/node.cc for now, grep for It might be worthwhile to file a V8 issue for it. |
OK, let's try again with that change CI: https://jenkins-iojs.nodesource.com/job/node-test-commit/62/ |
Includes cherry-picks for:
This is the v8 version of stable Chrome 44.