vm: support parsing a script in a specific context

[TimothyGu]

Differences are only observable through Inspector's Debugger.scriptParsed event (hence the Dev Tools frontend).

Before when executing vm.runInNewContext('debugger'):

After:

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

vm

[TimothyGu]

/cc @nodejs/v8-inspector

[bnoordhuis]

LGTM, I suppose, although it feels a bit like a workaround rather than a proper fix, and since it adds a public property, we might come to regret that later.

[bnoordhuis]

Four space indent.

[TimothyGu]

@bnoordhuis Would you be more satisfied had I made the option an "internal" one through unexposed symbols?

[bnoordhuis]

I think that would be best. We can always make it public later if a good use case presents itself.

[TimothyGu]

@bnoordhuis Okay, done.

CI: https://ci.nodejs.org/job/node-test-commit/11871/

[bnoordhuis]

This seems like over-complicating it. You can create the symbol in InitContextify() or ContextifyScript::Init() and set it on target and then use it directly from the binding object, no need for a new file in lib/internal.

[TimothyGu]

... not sure what got into my head. Will do.

[bnoordhuis]

You won't need this check if you do.

[bnoordhuis]

FWIW, it's possible to condense this to:

Local<Value> value;
if (!obj->Get(context, key).ToLocal(&value)) return MaybeLocal<Context>();

[bnoordhuis]

This branch cannot realistically be taken, can it?

[TimothyGu]

Not sure, but the same check is in a couple of other places in the file so I wouldn't touch them in this PR.

[TimothyGu]

@bnoordhuis would you mind taking another look? Thanks!

[TimothyGu]

I'll apply this early next week if I don't see any comments.

[bnoordhuis]

LGTM but see comments.

[bnoordhuis]

Could be shortened to Context::Scope scope(maybe_context.FromMaybe(env->context()));.

[bnoordhuis]

Teeny tiny style nit but line continuations are indented by four spaces. (I don't know why the linter doesn't enforce that.)

[TimothyGu]

CI: https://ci.nodejs.org/job/node-test-pull-request/9941/

[TimothyGu]

Landed in d932e80 with @bnoordhuis's comments fixed.

[MylesBorins]

It appears that this change is breaking browserify

TAP version 13
    # Subtest: yield
    1..6
(node:71559) Warning: process.on(SIGPROF) is reserved while debugging
    ok 1 - should not error
    not ok 2 - TypeError: Cannot convert undefined or null to object
      ---
      test: yield
      message: 'TypeError: Cannot convert undefined or null to object'
      stack: >
        getOwnPropertyNames (<anonymous>)

        Function.assign (eval at <anonymous>
        (node_modules/traceur/src/node/traceur.js:24:17), <anonymous>:219:19)

        Object.runInNewContext (vm.js:131:22)

        test/yield.js:14:12

        index.js:779:13

        ConcatStream.<anonymous> (node_modules/concat-stream/index.js:36:43)

        finishMaybe
        (node_modules/concat-stream/node_modules/readable-stream/lib/_stream_writable.js:475:14)

        endWritable
        (node_modules/concat-stream/node_modules/readable-stream/lib/_stream_writable.js:485:3)
      ...
    not ok 3 - missing test
    not ok 4 - missing test
    not ok 5 - missing test
    not ok 6 - missing test
    # failed 5 of 6 tests
not ok 1 - yield # time=216.95ms
  ---
  at:
    file: test/yield.js
    line: 5
    column: 1
  results:
    plan:
      start: 1
      end: 6
    count: 6
    pass: 1
    ok: false
    fail: 5
    time: 216.95
  source: |
    test('yield', function (t) {
  ...

1..1
# failed 1 of 1 tests

I'm going to drop this from the 8.5.0 release and add semver-major for now

[TimothyGu]

@MylesBorins That sounds like a bug. I shall create a PR that fixes it.

[MylesBorins]

@TimothyGu to confirm, a bug with node or with browserify / watchify?

[TimothyGu]

@MylesBorins Node.js (this PR specifically).

[TimothyGu]

@MylesBorins Upon a second look, it seems to me that this bug is with Browserify -- or more specifically, an old and buggy version of Traceur's Object.assign polyfill that doesn't handle undefined, null, or multiple sources, which is unconditionally added to the global environment during Browserify's test/yield.js:

    // Object.assign (19.1.3.1)
    function assign(target, source) {
      var props = $getOwnPropertyNames(source);
      var p, length = props.length;
      for (p = 0; p < length; p++) {
        target[props[p]] = source[props[p]];
      }
      return target;
    }

So, not a bug in Node.js after all...

[MylesBorins]

Marking this Semver-Major until it is not longer breaking the browserify test suite