debug: unhandled exception from runInDebugContext causes segfault

[ofrobots]

The following test-case causes a segfault:

$ cat test1.js
require('vm').runInDebugContext('*');
$ ~/src/io.js/iojs test1.js
[1]    96829 segmentation fault  ~/src/io.js/iojs test1.js

With a debug build:

$ ~/src/io.js/iojs_g test1.js
FATAL ERROR: v8::Context::GetAlignedPointerFromEmbedderData() Index too large
[1]    97178 abort      ~/src/io.js/iojs_g test1.js

I think that the issue is that node::Environment has not been initialized for the debug context.

Here's a (truncated) stack-trace from the debug build:

* thread #1: tid = 0x1074ac, 0x00007fff96467286 libsystem_kernel.dylib`__pthread_kill + 10, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x00007fff96467286 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff8ab3842f libsystem_pthread.dylib`pthread_kill + 90
    frame #2: 0x00007fff98192b53 libsystem_c.dylib`abort + 129
    frame #3: 0x000000010097200c iojs_g`node::OnFatalError(location=0x0000000100b06df9, message=0x0000000100b0ab07) + 124 at node.cc:2119
    frame #4: 0x00000001001fe002 iojs_g`v8::Utils::ReportApiFailure(location=0x0000000100b06df9, message=0x0000000100b0ab07) + 98 at api.cc:184
    frame #5: 0x00000001002223df iojs_g`v8::Utils::ApiCheck(condition=false, location=0x0000000100b06df9, message=0x0000000100b0ab07) + 47 at api.h:181
    frame #6: 0x00000001001ff627 iojs_g`v8::EmbedderDataFor(context=0x000000010204a3a8, index=32, can_grow=false, location=0x0000000100b06df9) + 279 at api.cc:570
    frame #7: 0x00000001001ff82f iojs_g`v8::Context::SlowGetAlignedPointerFromEmbedderData(this=0x000000010204a3a8, index=32) + 47 at api.cc:602
    frame #8: 0x000000010022321b iojs_g`v8::Context::GetAlignedPointerFromEmbedderData(this=0x000000010204a3a8, index=32) + 27 at v8.h:7440
    frame #9: 0x0000000100957815 iojs_g`node::Environment::GetCurrent(context=Local<v8::Context> at 0x00007fff5fbfe398) + 37 at env-inl.h:147
    frame #10: 0x00000001009572f2 iojs_g`node::Environment::GetCurrent(isolate=0x0000000102007600) + 34 at env-inl.h:142
    frame #11: 0x0000000100972040 iojs_g`node::FatalException(isolate=0x0000000102007600, error=(val_ = v8::Value * = 0x000000010204a380), message=(val_ = v8::Message * = 0x000000010204a370)) + 48 at node.cc:2135
    frame #12: 0x00000001009728e5 iojs_g`node::OnMessage(message=(val_ = v8::Message * = 0x000000010204a370), error=(val_ = v8::Value * = 0x000000010204a380)) + 53 at node.cc:2181
    frame #13: 0x00000001006aa677 iojs_g`v8::internal::MessageHandler::ReportMessage(isolate=0x0000000102007600, loc=0x00007fff5fbfe688, message=Handle<v8::internal::Object> at 0x00007fff5fbfe648) + 727 at messages.cc:117
    frame #14: 0x000000010064944d iojs_g`v8::internal::Isolate::ReportPendingMessages(this=0x0000000102007600) + 413 at isolate.cc:1384
    frame #15: 0x00000001003ef0d9 iojs_g`v8::internal::Compiler::CompileScript(source=Handle<v8::internal::String> at 0x00007fff5fbfe9d0,
...
...

Frame 9 is trying to get the node::Environment from the debug context, but that is going to fail, since we never Set EmbedderData in that Context.

Wrapping the test case in a try/catch works around the issue.

I tested with 1.5.2, 1.1.0 and 0.12.0. All of them crash as above.

[ofrobots]

Here's a patch that fixes the segfault

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 6985a33..1a8bd07 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -233,10 +233,14 @@ class ContextifyContext {


   static void RunInDebugContext(const FunctionCallbackInfo<Value>& args) {
+    // TODO(ofrobots): maybe only do this once
+    Local<Context> context = Debug::GetDebugContext();
+    Environment::GetCurrent(args)->AssignToContext(context);
+
     Local<String> script_source(args[0]->ToString(args.GetIsolate()));
     if (script_source.IsEmpty())
       return;  // Exception pending.
-    Context::Scope context_scope(Debug::GetDebugContext());
+    Context::Scope context_scope(context);
     Local<Script> script = Script::Compile(script_source);
     if (script.IsEmpty())
       return;  // Exception pending.

However, don't go merging it just yet. While the simplified test-case (above) does get fixed, the following slightly bigger test-case (that is closer to what I am trying to do) still segfaults.

var vm = require('vm');
var Debug = vm.runInDebugContext('Debug');

Debug.setListener(function(evt, execState, eventData) {
  console.log('debug event ' + evt);
  if (evt === 1) {
    var mirror = execState.frame(0).evaluate('process.env');
    console.log(mirror.properties());
  }
});

debugger;

This segfaults in a similar way:

$ ~/src/io.js/iojs test2.js
debug event 1
debug event 5
[1]    85147 segmentation fault  ~/src/io.js/iojs ~/src/test/test2.js

I suspect that there is yet another context active at this point.

The crash occurs on mirror.properties(). The interesting thing about process.env is that it has a named interceptor. That's why we end up trying to fetch the Environment from the current Context, but we apparently haven't assigned the Environment to that Context yet (perhaps because it was created by V8).

Here's what I think we need to do:

• Make sure we can Assign the Environment in all Contexts that get created. or
• Figure out a different way of squirreling away the Environment pointer.

@indutny thoughts?

[ofrobots]

This is related to nodejs/node-v0.x-archive#9156. I am trying to come up with a solution.
/cc @indutny.

[ofrobots]

@bnoordhuis Looking at commit 756b622, you might have some, ehm, context about this problem too.

There are a bunch of contexts at play here. At the time RunInDebugContext is called, V8 creates the Debug context, but since there is no listener or message handler installed at this point, the context gets destructed before we return to RunInDebugContext. At the time the listener is installed, a new Debug context will be created by V8.

We never had an opportunity to Assign node::Environment to that context.

Here's my question: wouldn't it make more sense for node to associate node::Environment with the Isolate instead of the Context. Clearly a given node::Environment can be relevant to multiple contexts.

Perhaps the fix is to store the Environment into Isolate::SetData(kIslolateSlot) – where Environment::IsolateData is presently stored, and make IsolateData an immediate field of Environment. I am not familiar enough with the lifetimes of Environment and Environment::IsolateData to know for sure if this matches the current semantics.

Let me know if this makes sense. I can send a PR.
EDIT: syntax.

[bnoordhuis]

Thanks for the bug report, should be fixed by cf081a4.

[ofrobots]

The second testcase I posted still fails even with your fix.

$ ./iojs_g ~/src/test/test2.js
debug event 1
debug event 5
FATAL ERROR: v8::Context::GetAlignedPointerFromEmbedderData() Index too large

The testcase is in #1190 (comment).

[bnoordhuis]

I'm not sure I agree that qualifies as an io.js bug. Let me adapt the test so it's a little easier to talk about:

require('vm').runInDebugContext('Debug').setListener(ondebugevent);

function ondebugevent(evt, exc) {
  if (evt === 1) exc.frame(0).evaluate('process.env').properties();
}

function breakpoint() { debugger; }

breakpoint();

ondebugevent runs in the debug context. exc.frame(0).evaluate('process.env') evaluates the process.env enumerator in the scope of breakpoint but without entering breakpoint's context first. I'm inclined to say that's a V8 bug.

Anyway, that's why it segfaults, because node::EnvEnumerator() is called with the wrong context. It's called with v8::Isolate::GetCurrentContext() == v8::Debug::GetDebugContext().

(v8::Isolate::GetCallingContext() and v8::Isolate::GetEnteredContext() return the right context but have problems of their own. The first is wrong when C++ enters a new scope with v8::Context::Scope, the second is wrong when there is a C++ -> JS -> C++ call sequence on the stack where the JS stack frame changes the active context.)

But whatever, I think we can work around this quirk with a little effort. I'll open a PR in a few.

[bnoordhuis]

Fixed again in 7e88a93. Please file new issues if this turns into a bug whack-a-mole.

[ofrobots]

Awesome! Thanks a lot for fixing.