Debuggers & profilers broken on Node.js 5.2

[ofrobots]

Debuggers and profilers are broken in Node.js 5.2 by the recent changes to how we wrap modules. Specifically the PRs: #4254 and #2867 are causing issues.

The following test-case works with Node.js 5.1.1, but doesn't work with Node.js 5.2:

var vm = require('vm');
var Debug = vm.runInDebugContext('Debug');
var foo = require('./foo.js');

Debug.setListener(function (e,s,d) {
  console.log('debug event', e);
});
Debug.setBreakPoint(foo);
foo('world');
console.log(Debug.showBreakPoints(foo));

Where foo.js is

module.exports = function bar(who) { console.log('hello', who); }

The expected output is:

debug event 1
hello world
(who) { [B0]console.log('hello', who); }

This has to do with how V8 interprets line_offset provided with the scripts. V8 ends up setting the breakpoint at the wrong line, or in this case, not setting the breakpoint at all because it looks like a breakpoint beyond the end of the file.

Profilers are broken for a similar reason as the line-ticks are going to be reported incorrectly.

I am still investigating the issue to figure out if it is a bug in V8. In the meanwhile, I would argue that #4254 and #2867 should be reverted.

@cjihrig @bnoordhuis Thoughts?
/cc @nodejs/v8

[matthewloring]

In the profiler case, this issue manifests as failed v8 assertions (debug builds) and garbled state (release builds) when the v8 profiler is active.

For example, profiling the script:

require('child_process').spawn('ls');

yields:

#
# Fatal error in ../deps/v8/src/profiler/profile-generator.cc, line 29
# Check failed: line > 0.
#

==== C stack trace ===============================

 1: V8_Fatal
 2: v8::internal::JITLineInfoTable::SetPosition(int, int)
 3: v8::internal::CpuProfiler::CodeCreateEvent(v8::internal::Logger::LogEventsAndTags, v8::internal::Code*, v8::internal::SharedFunctionInfo*, v8::internal::CompilationInfo*, v8::internal::Name*, int, int)
 4: v8::internal::Logger::CodeCreateEvent(v8::internal::Logger::LogEventsAndTags, v8::internal::Code*, v8::internal::SharedFunctionInfo*, v8::internal::CompilationInfo*, v8::internal::Name*, int, int)
 5: 0x100100f
 6: v8::internal::Compiler::GetSharedFunctionInfo(v8::internal::FunctionLiteral*, v8::internal::Handle<v8::internal::Script>, v8::internal::CompilationInfo*)
 7: v8::internal::FullCodeGenerator::VisitFunctionLiteral(v8::internal::FunctionLiteral*)
 8: v8::internal::FunctionLiteral::Accept(v8::internal::AstVisitor*)
 9: v8::internal::FullCodeGenerator::Visit(v8::internal::AstNode*)
10: v8::internal::FullCodeGenerator::VisitForAccumulatorValue(v8::internal::Expression*)

[cjihrig]

I'm fine with delaying the release to get this sorted out. For what it's worth, the release is ready and tested, and was just getting ready to go out the door.

I don't think those PRs need to be completely reverted, but I'll submit a PR to revert the relevant pieces so we can get debugging and profiling working again.

@ofrobots can you keep us updated as you find out if this is a V8 bug or not?

[ofrobots]

@cjihrig I'll update this issue as I make progress.

[cjihrig]

@ofrobots and @matthewloring can you verify that #4298 fixed the issues you were seeing?

[ofrobots]

I have opened a V8 issue for -1 line_offset here: https://bugs.chromium.org/p/v8/issues/detail?id=4620

[matthewloring]

#4298 Fixed the failed asserts inside the v8 profiler.