Debugger: vm.runInDebugContext change behavior in 0.11.16

[3y3]

var vm = require('vm');
var assert = require('assert');

var proto = vm.runInDebugContext('DebugCommandProcessor.prototype');
proto.option_ = true;
assert(proto.option_ === true);
proto = vm.runInDebugContext('DebugCommandProcessor.prototype');
assert(proto.option_ === true);
proto = vm.runInDebugContext('DebugCommandProcessor.prototype.option_ = true; DebugCommandProcessor.prototype');
assert(proto.option_ === true);

This test passes in 0.11.14 and fails in 0.11.16.
I think this is relative to 947408b
@indutny , please confirm this.

Previous behavior is very important for node-inspector. This is my current way to extend v8 debugger protocol. If new behavior is correct, can you propose any way to extend debugger protocol after 0.11.16 and in io.js

[indutny]

Looking

[indutny]

This is not caused by 947408b for sure, happens on a2a3fd4 too.

[indutny]

I think it was broken during the merge 7ca5af8

[indutny]

Hm... may be not.

[indutny]

Btw, doesn't seem to be working on v0.11.14 for me.

[indutny]

And on v0.11.15 too.

[indutny]

Same on v0.11.16 ... @3y3 am I missing something? This feature was introduced in v0.11.14 and your test case does not appear to be working with any versions after (and including) it.

[3y3]

Hm... Win x64 works on 0.11.14 for me (downloaded from site).

[indutny]

@tjfontaine @misterdjules can we confirm this on windows? (I don't have any machines at hand)

[3y3]

I also backported this feature to previous versions in v8-debug project (small core of node-inspector) link and it works fine excluding to 0.11.15+

[indutny]

@3y3 it is failing on:

var proto = vm.runInDebugContext('DebugCommandProcessor.prototype');
proto.option_ = true;

for me

[indutny]

~/Code/joyent/node $ ./node -p "process.version"
v0.11.16
~/Code/joyent/node $ cat 1.js
var vm = require('vm');
var assert = require('assert');

var proto = vm.runInDebugContext('DebugCommandProcessor.prototype');
assert(proto.option_ === true);
proto = vm.runInDebugContext('DebugCommandProcessor.prototype');
assert(proto.option_ === true);
proto = vm.runInDebugContext('DebugCommandProcessor.prototype.option_ = true; DebugCommandProcessor.prototype');
assert(proto.option_ === true);
~/Code/joyent/node $ ./node 1

assert.js:86
  throw new assert.AssertionError({
        ^
AssertionError: false == true
    at Object.<anonymous> (/Users/indutny/Code/joyent/node/1.js:5:1)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:501:10)
    at startup (node.js:129:16)
    at node.js:814:3

[indutny]

Gosh. I got this line removed somehow, sorry about this! :)

[3y3]

Success

D:\3y3\GIT\v8-debug [master +3 ~5 -0 !]> node -p "process.version + ' ' + process.arch"
v0.11.14 ia32
D:\3y3\GIT\v8-debug [master +3 ~5 -0 !]> cat test.js
var vm = require('vm');
var assert = require('assert');

var proto = vm.runInDebugContext('DebugCommandProcessor.prototype');
proto.option_ = true;
assert(proto.option_ === true);
proto = vm.runInDebugContext('DebugCommandProcessor.prototype');
assert(proto.option_ === true);
proto = vm.runInDebugContext('DebugCommandProcessor.prototype.option_ = true; DebugCommandProcessor.prototype');
assert(proto.option_ === true);
D:\3y3\GIT\v8-debug [master +3 ~5 -0 !]> node test
D:\3y3\GIT\v8-debug [master +3 ~5 -0 !]>

Failure

D:\3y3\GIT\v8-debug [master +3 ~5 -0 !]> node -p "process.version + ' ' + process.arch"
v0.11.15 ia32
D:\3y3\GIT\v8-debug [master +3 ~5 -0 !]> cat test.js
var vm = require('vm');
var assert = require('assert');

var proto = vm.runInDebugContext('DebugCommandProcessor.prototype');
proto.option_ = true;
assert(proto.option_ === true);
proto = vm.runInDebugContext('DebugCommandProcessor.prototype');
assert(proto.option_ === true);
proto = vm.runInDebugContext('DebugCommandProcessor.prototype.option_ = true; DebugCommandProcessor.prototype');
assert(proto.option_ === true);
D:\3y3\GIT\v8-debug [master +3 ~5 -0 !]> node test

assert.js:99
  throw new assert.AssertionError({
        ^
AssertionError: false == true
    at Object.<anonymous> (D:\3y3\GIT\v8-debug\test.js:8:1)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:501:10)
    at startup (node.js:129:16)
    at node.js:814:3
D:\3y3\GIT\v8-debug [master +3 ~5 -0 !]>

Not relative to error but I'm sorry for

Hm... Win x64 works

I don't know why I use x32 arch in 0.11.15, but in 0.11.14 it was important for node-pre-gyp...

[indutny]

Ok, the offending commit seems to be 9116b24

[indutny]

Hah, actually 7a0cfe9 as you initially said :)

This appears to be fixing the problem:

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 6985a33..d4e284d 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -236,6 +236,10 @@ class ContextifyContext {
     Local<String> script_source(args[0]->ToString(args.GetIsolate()));
     if (script_source.IsEmpty())
       return;  // Exception pending.
+    Environment* env = Environment::GetCurrent(args);
+    Local<Context> ctx = Debug::GetDebugContext();
+    if (!ctx.IsEmpty())
+      ctx->SetSecurityToken(env->context()->GetSecurityToken());
     Context::Scope context_scope(Debug::GetDebugContext());
     Local<Script> script = Script::Compile(script_source);
     if (script.IsEmpty())

@3y3 do you have resources to verify it?

[3y3]

I'll try to rebuild my 0.11.16 now

[indutny]

Just opened PR: #9157

[indutny]

@3y3 unfortunately, it does not fix the issue. Going to look into it tomorrow.

[indutny]

Most likely wontfix as per https://code.google.com/p/v8/issues/detail?id=3876&thanks=3876&ts=1423353590 .

[3y3]

Wow! It's a bad news. I subscribed on v8 issue.
So if it will be wontfix, I'll think about pr for debugger_agent. It's a last place where I can inject debugger extensions dynamically.
And now I need to make console tracing and profiling more optionally for node-inspector, because this features based on dinamic injection.

About new debugger_agent - is this a main gateway for @bmeck work on Using webkit debugger protocol?

[indutny]

@3y3 I have no idea :) How are you using this DebugCommandProcessor anyway?

[3y3]

I do the following

DebugCommandProcessor.prototype.processDebugRequest = function WRAPPED_BY_NODE_INSPECTOR(request) {
    return this.extendedProcessDebugJSONRequest_(request)
      || this.processDebugJSONRequest(request);
  };
DebugCommandProcessor.prototype.extendedProcessDebugJSONRequest_ = function(request) {
  // if request exists in extendedProcessDebugJSONRequestHandles_ 
  // then process it
  // else return false
}
DebugCommandProcessor.prototype.extendedProcessDebugJSONRequestHandles_ = {};

This way a hack this line of code. So in current time this line is unhackable =(

I provide small api to write in extendedProcessDebugJSONRequestHandles_ and call commands.

From node-inspector source:

debug.registerCommand('Profiler.stop', function(request, response) {
    var profile = profiler.stopProfiling();
    profilingEnabled = false;
    response.body = {header: profile.getHeader()};

    debug.sendCommand(
      'Profiler.addProfileHeader',
      response.body
    );

    debug.sendCommand(
      'Profiler.setRecordingProfile',
      {isProfiling: false}
    );
  });

So debug.sendCommand is a wrapped v8::Debug::SendCommand.

[3y3]

Ok =) As temporary fix I use next strategy:

Crazy pseudocode

v8::Debug::Call(function(exec_state) {
  var proto = exec_state.debugCommandProcessor().__proto__
  //modify proto
  v8::Debug::SendCommand(/*any command*/);
});

[3y3]

@indutny , where I need to open isues for debugger-agent?

For example current iisue is Unhandled error event
Or where I can contribute to it?

[indutny]

@3y3 you could do it in node.js or io.js repo, depends on what works for you best.

[3y3]

@indutny , is it possible to share _debugAPI with main thread process object?

It will be nice to use _debugAPI.sendCommand as entry point to extensible debugger protocol.
So I'll be glad to use it by way described below:

process._debugAPI.sendCommand = (function(originalSendCommand) {
  return function(command) {
    // do here something
   originalSendCommand();
  }
}(process._debugAPI.sendCommand))

v8 issue was accepted, but I don't know that it means for us (is this "ok, we well fix it in future" or "ok, we see it and we will think to fix it or not" ?)

[indutny]

This is what v8 team has replied, just in case:

When the debugger becomes active, the debug context is loaded and kept via handle. Once the debugger becomes inactive, the handle is cleared. When the debugger becomes active again, a new debug context is loaded.

You can keep the debug context alive by:
- Keep it in a persistent handle so that you can use it. However, internal debugger functionality will still run in fresh debug contexts when it needs to execute something in a debug context, and the debugger has not been active.
- Or keep the debugger active by registering a debug event listener or message listener.

[3y3]

Oh, yes! I'm already implement noop debug event listener in v8-debug module. This solution works fine.

persistent handle doesn't work for me. Maybe I cook it wrong.

I'm not sure that debug event listener needs to be a part of node.
Maybe reasonable to redefine runInDebugContext to private _runInDebugContext, like process._debugProcess? This will block common users from using this suspicious behavior.