Skip to content

Commit

Permalink
node_contextify: do not incept debug context
Browse files Browse the repository at this point in the history
Currently a debug context is created for various calls to util.

If the node debugger is being run the main context is the debug
context. In this case node_contextify was freeing the debug context
and causing everything to explode.

This change moves around the logic and no longer frees the context.

There is a concern about the dangling pointer

The regression test was adapted from code submitted by @3y3 in nodejs#4815

Fixes: nodejs#4440
Fixes: nodejs#4815
Fixes: nodejs#4597
Fixes: nodejs#4952

PR-URL: nodejs#4815

Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
  • Loading branch information
Myles Borins committed Feb 15, 2016
1 parent f205e99 commit 1b070e4
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 21 deletions.
31 changes: 10 additions & 21 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -239,37 +239,26 @@ class ContextifyContext {


static void RunInDebugContext(const FunctionCallbackInfo<Value>& args) {
// Ensure that the debug context has an Environment assigned in case
// a fatal error is raised. The fatal exception handler in node.cc
// is not equipped to deal with contexts that don't have one and
// can't easily be taught that due to a deficiency in the V8 API:
// there is no way for the embedder to tell if the data index is
// in use.
struct ScopedEnvironment {
ScopedEnvironment(Local<Context> context, Environment* env)
: context_(context) {
const int index = Environment::kContextEmbedderDataIndex;
context->SetAlignedPointerInEmbedderData(index, env);
}
~ScopedEnvironment() {
const int index = Environment::kContextEmbedderDataIndex;
context_->SetAlignedPointerInEmbedderData(index, nullptr);
}
Local<Context> context_;
};

Local<String> script_source(args[0]->ToString(args.GetIsolate()));
if (script_source.IsEmpty())
return; // Exception pending.
Local<Context> debug_context = Debug::GetDebugContext();
Environment* env = Environment::GetCurrent(args);
if (debug_context.IsEmpty()) {
// Force-load the debug context.
Debug::GetMirror(args.GetIsolate()->GetCurrentContext(), args[0]);
debug_context = Debug::GetDebugContext();
CHECK(!debug_context.IsEmpty());
// Ensure that the debug context has an Environment assigned in case
// a fatal error is raised. The fatal exception handler in node.cc
// is not equipped to deal with contexts that don't have one and
// can't easily be taught that due to a deficiency in the V8 API:
// there is no way for the embedder to tell if the data index is
// in use.
const int index = Environment::kContextEmbedderDataIndex;
debug_context->SetAlignedPointerInEmbedderData(index, env);
}
Environment* env = Environment::GetCurrent(args);
ScopedEnvironment env_scope(debug_context, env);

Context::Scope context_scope(debug_context);
Local<Script> script = Script::Compile(script_source);
if (script.IsEmpty())
Expand Down
4 changes: 4 additions & 0 deletions test/fixtures/debugger-util-regression-fixture.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use strict';
const util = require('util');
const payload = util.inspect({a: 'b'});
console.log(payload);
69 changes: 69 additions & 0 deletions test/parallel/test-debugger-util-regression.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
'use strict';
const path = require('path');
const spawn = require('child_process').spawn;
const assert = require('assert');

const common = require('../common');

const fixture = path.join(
common.fixturesDir,
'debugger-util-regression-fixture.js'
);

const args = [
'debug',
fixture
];

const proc = spawn(process.execPath, args, { stdio: 'pipe' });
proc.stdout.setEncoding('utf8');
proc.stderr.setEncoding('utf8');

function fail() {
common.fail('the program should not hang');
}

const timer = setTimeout(fail, common.platformTimeout(4000));

let stdout = '';
let stderr = '';

let nextCount = 0;

proc.stdout.on('data', (data) => {
stdout += data;
if (stdout.includes('> 1') && nextCount < 1 ||
stdout.includes('> 2') && nextCount < 2 ||
stdout.includes('> 3') && nextCount < 3 ||
stdout.includes('> 4') && nextCount < 4) {
nextCount++;
proc.stdin.write('n\n');
}
else if (stdout.includes('{ a: \'b\' }')) {
clearTimeout(timer);
proc.stdin.write('.exit\n');
}
else if (stdout.includes('program terminated')) {
// Catch edge case present in v4.x
// process will terminate after call to util.inspect
common.fail('the program should not terminate');
}
});

proc.stderr.on('data', (data) => stderr += data);

// FIXME
// This test has been periodically failing on certain systems due to
// uncaught errors on proc.stdin. This will stop the process from
// exploding but is still not an elegant solution. Likely a deeper bug
// causing this problem.
proc.stdin.on('error', (err) => {
console.error(err);
});

process.on('exit', (code) => {
assert.equal(code, 0, 'the program should exit cleanly');
assert.equal(stdout.includes('{ a: \'b\' }'), true,
'the debugger should print the result of util.inspect');
assert.equal(stderr, '', 'stderr should be empty');
});

0 comments on commit 1b070e4

Please sign in to comment.