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: https://github.com/nodejs/node/issues/4440
Fixes: https://github.com/nodejs/node/issues/4815
Fixes: https://github.com/nodejs/node/issues/4597
Fixes: https://github.com/nodejs/node/issues/4952

PR-URL: https://github.com/nodejs/node/issues/4815

Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>

3 files changed, 83 insertions, 21 deletions

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index bfd45a22862..59a90c88c99 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -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())
diff --git a/test/fixtures/debugger-util-regression-fixture.js b/test/fixtures/debugger-util-regression-fixture.js
new file mode 100644
index 00000000000..d397f3d641e
--- /dev/null
+++ b/test/fixtures/debugger-util-regression-fixture.js
@@ -0,0 +1,4 @@
+'use strict';
+const util = require('util');
+const payload = util.inspect({a: 'b'});
+console.log(payload);
diff --git a/test/parallel/test-debugger-util-regression.js b/test/parallel/test-debugger-util-regression.js
new file mode 100644
index 00000000000..cf32ec3fa66
--- /dev/null
+++ b/test/parallel/test-debugger-util-regression.js
@@ -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');
+});