vm: support parsing a script in a specific context #14888

TimothyGu · 2017-08-17T07:09:08Z

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 · 2017-08-17T08:20:30Z

/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 · 2017-08-17T14:18:22Z

src/node_contextify.cc

+      return MaybeLocal<Context>();
+
+    MaybeLocal<Value> maybe_value =
+      options.As<Object>()->Get(


Four space indent.

TimothyGu · 2017-08-17T15:50:25Z

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

bnoordhuis · 2017-08-17T16:36:52Z

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

TimothyGu · 2017-08-18T01:46:29Z

@bnoordhuis Okay, done.

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

bnoordhuis · 2017-08-18T09:13:11Z

lib/vm.js

@@ -21,6 +21,7 @@

 'use strict';

+const { kParsingContext } = require('internal/vm');
 const binding = process.binding('contextify');


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.

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

bnoordhuis · 2017-08-18T09:18:30Z

src/node_contextify.cc

+      return MaybeLocal<Context>();
+
+    // Symbol handle may be empty when initializing.
+    if (env->vm_parsing_context_symbol().IsEmpty())


You won't need this check if you do.

bnoordhuis · 2017-08-18T09:21:12Z

src/node_contextify.cc

+    if (maybe_value.IsEmpty())
+      return MaybeLocal<Context>();
+
+    Local<Value> value = maybe_value.ToLocalChecked();


FWIW, it's possible to condense this to:

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

bnoordhuis · 2017-08-18T09:23:53Z

src/node_contextify.cc

+
+    Local<Context> context = sandbox->context();
+    if (context.IsEmpty())
+      return MaybeLocal<Context>();


This branch cannot realistically be taken, can it?

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 · 2017-08-23T09:42:21Z

@bnoordhuis would you mind taking another look? Thanks!

TimothyGu · 2017-09-01T14:33:27Z

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

bnoordhuis

LGTM but see comments.

bnoordhuis · 2017-09-04T11:35:59Z

src/node_contextify.cc

+    Context::Scope scope(
+        maybe_context.IsEmpty() ?
+            env->context() :
+            maybe_context.ToLocalChecked());


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

bnoordhuis · 2017-09-04T11:37:39Z

test/inspector/test-scriptparsed-context.js

+
+async function getContext(session) {
+  const created =
+    await session.waitForNotification('Runtime.executionContextCreated');


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

PR-URL: nodejs#14888 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>

TimothyGu · 2017-09-05T03:21:51Z

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

TimothyGu · 2017-09-05T03:52:06Z

Landed in d932e80 with @bnoordhuis's comments fixed.

PR-URL: nodejs/node#14888 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>

PR-URL: #14888 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>

MylesBorins · 2017-09-11T08:07:57Z

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 · 2017-09-11T10:17:01Z

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

MylesBorins · 2017-09-11T10:43:10Z

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

TimothyGu · 2017-09-11T10:44:41Z

@MylesBorins Node.js (this PR specifically).

TimothyGu · 2017-09-12T02:05:43Z

@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...

PR-URL: #14888 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>

MylesBorins · 2017-10-11T04:51:59Z

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

bug in traceur compiler polyfill in browserify is causing errors in 8.x and above. This needs to be fixed in browserify. Refs: nodejs/node#14888 (comment)

TimothyGu added inspector Issues and PRs related to the V8 inspector protocol vm Issues and PRs related to the vm subsystem. labels Aug 17, 2017

TimothyGu requested review from bnoordhuis and eugeneo August 17, 2017 07:09

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Aug 17, 2017

TimothyGu force-pushed the vm-inspector-script branch from 9a38ab7 to 1ea23ac Compare August 17, 2017 07:17

jasnell approved these changes Aug 17, 2017

View reviewed changes

bnoordhuis approved these changes Aug 17, 2017

View reviewed changes

eugeneo approved these changes Aug 17, 2017

View reviewed changes

TimothyGu force-pushed the vm-inspector-script branch 3 times, most recently from 330e27b to ce77785 Compare August 18, 2017 01:43

bnoordhuis reviewed Aug 18, 2017

View reviewed changes

bnoordhuis approved these changes Sep 4, 2017

View reviewed changes

vm: support parsing a script in a specific context

d932e80

PR-URL: nodejs#14888 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>

TimothyGu force-pushed the vm-inspector-script branch from f40774e to d932e80 Compare September 5, 2017 02:54

TimothyGu closed this Sep 5, 2017

TimothyGu deleted the vm-inspector-script branch September 5, 2017 03:52

TimothyGu merged commit d932e80 into nodejs:master Sep 5, 2017

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

MylesBorins added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 11, 2017

MylesBorins mentioned this pull request Oct 11, 2017

Revert "vm: support parsing a script in a specific context" #16136

Closed

MylesBorins added a commit to MylesBorins/citgm that referenced this pull request Oct 31, 2017

lookup: skip browserify in 8.x >

24f033a

bug in traceur compiler polyfill in browserify is causing errors in 8.x and above. This needs to be fixed in browserify. Refs: nodejs/node#14888 (comment)

MylesBorins mentioned this pull request Oct 31, 2017

lookup: skip browserify in 8.x > nodejs/citgm#495

Merged

TimothyGu mentioned this pull request Aug 5, 2018

src: remove unused env->vm_parsing_context_symbol #22034

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: support parsing a script in a specific context #14888

vm: support parsing a script in a specific context #14888

TimothyGu commented Aug 17, 2017

TimothyGu commented Aug 17, 2017

bnoordhuis left a comment

bnoordhuis Aug 17, 2017

TimothyGu commented Aug 17, 2017

bnoordhuis commented Aug 17, 2017

TimothyGu commented Aug 18, 2017

bnoordhuis Aug 18, 2017

TimothyGu Aug 19, 2017

bnoordhuis Aug 18, 2017

bnoordhuis Aug 18, 2017

bnoordhuis Aug 18, 2017

TimothyGu Aug 19, 2017

TimothyGu commented Aug 23, 2017

TimothyGu commented Sep 1, 2017

bnoordhuis left a comment

bnoordhuis Sep 4, 2017

bnoordhuis Sep 4, 2017

TimothyGu commented Sep 5, 2017

TimothyGu commented Sep 5, 2017

MylesBorins commented Sep 11, 2017

TimothyGu commented Sep 11, 2017

MylesBorins commented Sep 11, 2017

TimothyGu commented Sep 11, 2017

TimothyGu commented Sep 12, 2017

MylesBorins commented Oct 11, 2017

vm: support parsing a script in a specific context #14888

vm: support parsing a script in a specific context #14888

Conversation

TimothyGu commented Aug 17, 2017

Checklist

Affected core subsystem(s)

TimothyGu commented Aug 17, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Aug 17, 2017

bnoordhuis commented Aug 17, 2017

TimothyGu commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Aug 23, 2017

TimothyGu commented Sep 1, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Sep 5, 2017

TimothyGu commented Sep 5, 2017

MylesBorins commented Sep 11, 2017

TimothyGu commented Sep 11, 2017

MylesBorins commented Sep 11, 2017

TimothyGu commented Sep 11, 2017

TimothyGu commented Sep 12, 2017

MylesBorins commented Oct 11, 2017