Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit 1b070e4

Browse filesBrowse files
author
Myles Borins
committed
node_contextify: do not incept debug context
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 #4815 Fixes: #4440 Fixes: #4815 Fixes: #4597 Fixes: #4952 PR-URL: #4815 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent f205e99 commit 1b070e4
Copy full SHA for 1b070e4

File tree

Expand file treeCollapse file tree

3 files changed

+83
-21
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

3 files changed

+83
-21
lines changed
Open diff view settings
Collapse file

‎src/node_contextify.cc‎

Copy file name to clipboardExpand all lines: src/node_contextify.cc
+10-21Lines changed: 10 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -239,37 +239,26 @@ class ContextifyContext {
239239

240240

241241
static void RunInDebugContext(const FunctionCallbackInfo<Value>& args) {
242-
// Ensure that the debug context has an Environment assigned in case
243-
// a fatal error is raised. The fatal exception handler in node.cc
244-
// is not equipped to deal with contexts that don't have one and
245-
// can't easily be taught that due to a deficiency in the V8 API:
246-
// there is no way for the embedder to tell if the data index is
247-
// in use.
248-
struct ScopedEnvironment {
249-
ScopedEnvironment(Local<Context> context, Environment* env)
250-
: context_(context) {
251-
const int index = Environment::kContextEmbedderDataIndex;
252-
context->SetAlignedPointerInEmbedderData(index, env);
253-
}
254-
~ScopedEnvironment() {
255-
const int index = Environment::kContextEmbedderDataIndex;
256-
context_->SetAlignedPointerInEmbedderData(index, nullptr);
257-
}
258-
Local<Context> context_;
259-
};
260-
261242
Local<String> script_source(args[0]->ToString(args.GetIsolate()));
262243
if (script_source.IsEmpty())
263244
return; // Exception pending.
264245
Local<Context> debug_context = Debug::GetDebugContext();
246+
Environment* env = Environment::GetCurrent(args);
265247
if (debug_context.IsEmpty()) {
266248
// Force-load the debug context.
267249
Debug::GetMirror(args.GetIsolate()->GetCurrentContext(), args[0]);
268250
debug_context = Debug::GetDebugContext();
269251
CHECK(!debug_context.IsEmpty());
252+
// Ensure that the debug context has an Environment assigned in case
253+
// a fatal error is raised. The fatal exception handler in node.cc
254+
// is not equipped to deal with contexts that don't have one and
255+
// can't easily be taught that due to a deficiency in the V8 API:
256+
// there is no way for the embedder to tell if the data index is
257+
// in use.
258+
const int index = Environment::kContextEmbedderDataIndex;
259+
debug_context->SetAlignedPointerInEmbedderData(index, env);
270260
}
271-
Environment* env = Environment::GetCurrent(args);
272-
ScopedEnvironment env_scope(debug_context, env);
261+
273262
Context::Scope context_scope(debug_context);
274263
Local<Script> script = Script::Compile(script_source);
275264
if (script.IsEmpty())
Collapse file
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
'use strict';
2+
const util = require('util');
3+
const payload = util.inspect({a: 'b'});
4+
console.log(payload);
Collapse file
+69Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
'use strict';
2+
const path = require('path');
3+
const spawn = require('child_process').spawn;
4+
const assert = require('assert');
5+
6+
const common = require('../common');
7+
8+
const fixture = path.join(
9+
common.fixturesDir,
10+
'debugger-util-regression-fixture.js'
11+
);
12+
13+
const args = [
14+
'debug',
15+
fixture
16+
];
17+
18+
const proc = spawn(process.execPath, args, { stdio: 'pipe' });
19+
proc.stdout.setEncoding('utf8');
20+
proc.stderr.setEncoding('utf8');
21+
22+
function fail() {
23+
common.fail('the program should not hang');
24+
}
25+
26+
const timer = setTimeout(fail, common.platformTimeout(4000));
27+
28+
let stdout = '';
29+
let stderr = '';
30+
31+
let nextCount = 0;
32+
33+
proc.stdout.on('data', (data) => {
34+
stdout += data;
35+
if (stdout.includes('> 1') && nextCount < 1 ||
36+
stdout.includes('> 2') && nextCount < 2 ||
37+
stdout.includes('> 3') && nextCount < 3 ||
38+
stdout.includes('> 4') && nextCount < 4) {
39+
nextCount++;
40+
proc.stdin.write('n\n');
41+
}
42+
else if (stdout.includes('{ a: \'b\' }')) {
43+
clearTimeout(timer);
44+
proc.stdin.write('.exit\n');
45+
}
46+
else if (stdout.includes('program terminated')) {
47+
// Catch edge case present in v4.x
48+
// process will terminate after call to util.inspect
49+
common.fail('the program should not terminate');
50+
}
51+
});
52+
53+
proc.stderr.on('data', (data) => stderr += data);
54+
55+
// FIXME
56+
// This test has been periodically failing on certain systems due to
57+
// uncaught errors on proc.stdin. This will stop the process from
58+
// exploding but is still not an elegant solution. Likely a deeper bug
59+
// causing this problem.
60+
proc.stdin.on('error', (err) => {
61+
console.error(err);
62+
});
63+
64+
process.on('exit', (code) => {
65+
assert.equal(code, 0, 'the program should exit cleanly');
66+
assert.equal(stdout.includes('{ a: \'b\' }'), true,
67+
'the debugger should print the result of util.inspect');
68+
assert.equal(stderr, '', 'stderr should be empty');
69+
});

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.