node_contextify: do not incept debug context #4819

MylesBorins · 2016-01-22T19:20:52Z

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 checks to see if there is currently a debug context
before setting up / tearing down a temporary context.

mscdex · 2016-01-22T19:23:26Z

Can you also add a test for this?

mscdex · 2016-01-22T19:29:49Z

src/node_contextify.cc

-    ScopedEnvironment env_scope(debug_context, env);
+    const int index = Environment::kContextEmbedderDataIndex;
+    if (!debug_context->GetAlignedPointerFromEmbedderData(index)) {
+      ScopedEnvironment env_scope(debug_context, env);


Are you sure this is right? env_scope's destructor will get called as soon as execution moves outside of the if statement. I'm not sure, but we may need to have an if-else and copy the code after the current if into both blocks (one with env_scope and one without)?

Thanks for the heads up. I have not used structs with this pattern before and was unaware of the semantics.

MylesBorins · 2016-01-22T19:46:04Z

@mscdex I've attempted a different approach, does this solve your original concerns?

bnoordhuis · 2016-01-22T19:57:57Z

src/node_contextify.cc

@@ -246,14 +246,22 @@ class ContextifyContext {
    // there is no way for the embedder to tell if the data index is
    // in use.
    struct ScopedEnvironment {
+      bool alreadyInDebugContext = false;


Style: C++ code uses snake_case for locals.

EDIT: And data members use snake_case and end in an underscore: bool already_in_debug_context_ = false; Also, it should go near the definition of context_.

I rebased in those style changes in case we decide to move forward with 803ca9667a8dc608c5271633e42e792dbba66fc0

MylesBorins · 2016-01-22T20:01:42Z

@bnoordhuis I've just pushed another option for handling this problem.

@ofrobots was saying that my second attempt would cause a FATAL ERROR if the index slot didn't exist

The current approach, originally done as a patch shared by ofrobots does away with that, but no longer cleans up the debugger context, which may be undesirable

bnoordhuis · 2016-01-22T20:04:25Z

test.js

@@ -0,0 +1,3 @@
+var util = require('util');


'use strict'

bah... didn't mean to check that in.

killed that file.

bnoordhuis · 2016-01-22T20:04:48Z

LGTM with a nit and assuming the CI likes it.

MylesBorins · 2016-01-22T20:07:23Z

@bnoordhuis can I get you to sign off one more time on the current head. Just want to ensure that is the one you were ok with

bnoordhuis · 2016-01-22T20:09:04Z

LGTM but is there a reason you're not deassigning the context slot afterwards?

MylesBorins · 2016-01-22T20:10:54Z

the deassignment was what was causing the original problem

MylesBorins@724fb28 offers an approach with deassignment but causes the debug errors mentioned above. TBQH I am a little out of my depth here, and not sure the best way to only deassign if there was not already a debugger context

bnoordhuis · 2016-01-22T20:14:00Z

What worries me a little is that you're essentially leaving a dangling pointer. If you have a simple regression test, I can take a look.

ofrobots · 2016-01-22T20:15:53Z

@bnoordhuis See: #4815. The original problem is that there is code in util.js (e.g. inspectPromise) that now keeps a keep a debug context around forever. Once that context has been created, subsequent calls to RunInDebugContext clear the environment from the debug context when they shoudn't.

I am worried about the dangling pointer, but I think Environment lives longer as the debug context so I am not sure it really is dangling?

bnoordhuis · 2016-01-22T21:03:38Z

Not now, no, but it's a future hazard. Change itself LGTM but a regression test would be good.

MylesBorins · 2016-01-22T21:07:28Z

@bnoordhuis are there similar regression tests I can look at to base it off of?

bnoordhuis · 2016-01-22T21:22:11Z

Maybe the test from 25776f3?

MylesBorins · 2016-01-22T22:27:07Z

So the edge case does not seem to show up if debugger is being run in interactive mode.

Not having a ton of luck here

jasnell · 2016-01-23T04:27:47Z

Yeah, this one is a hard one. The change LGTM but it leaves me a bit uneasy. Unfortunately I haven't had any luck coming up with an idea for a reliable regression test either.

MylesBorins · 2016-02-03T20:04:54Z

ping @bnoordhuis

Trott · 2016-02-05T03:55:42Z

test/parallel/test-debugger-util-regression.js

+proc.stderr.setEncoding('utf8');
+
+function fail() {
+  assert(false, 'the program should not hang');


Nit: common.fail()

Trott · 2016-02-05T04:01:51Z

Test LGTM. Would be great if @bnoordhuis or @indutny or someone else knowledgable about the C++ side of things could give an LGTM. (Ben gave one, but it was 13 days ago and I'm pretty sure the code has changed since then.)

indutny · 2016-02-05T21:04:01Z

LGTM if CI is happy.

bnoordhuis · 2016-02-05T22:04:46Z

test/parallel/test-debugger-util-regression.js

+// exploding but is still not an elegant solution. Likely a deeper bug
+// causing this problem.
+proc.stdin.on('error', (err) => {
+  console.error(new Error(err));


err is already an Error, isn't it?

bnoordhuis · 2016-02-05T22:07:12Z

LGTM with teeny tiny nits. Can I suggest you stress-test the test besides doing the regular CI?

MylesBorins · 2016-02-05T22:43:14Z

Stress Test: https://ci.nodejs.org/job/node-stress-single-test/415/

edit: stress test had over 1400 positives and 0 negatives so I stopped it.

@3y3

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 nodejs#4815 Fixes: nodejs#4440 Fixes: nodejs#4815 Fixes: nodejs#4597 Fixes: nodejs#4952 PR-URL: nodejs#4815 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Rich Trott <[email protected]>

MylesBorins · 2016-02-05T23:41:25Z

landed in 4897f94

Notable changes: * buffer: make byteLength work with Buffer correctly (Jackson Tian) - nodejs#4738 * debugger: guard against call from non-node context (Ben Noordhuis) - nodejs#4328 * node_contextify: do not incept debug context (Myles Borins) - nodejs#4819 * deps: update to http-parser 2.5.2 (James Snell) - nodejs#5238

Notable changes: * buffer: make byteLength work with Buffer correctly (Jackson Tian) - #4738 * debugger: guard against call from non-node context (Ben Noordhuis) - #4328 * node_contextify: do not incept debug context (Myles Borins) - #4819 * deps: update to http-parser 2.5.2 (James Snell) - #5238 PR-URL: #5200 (comment)

mscdex reviewed Jan 22, 2016
View reviewed changes

mscdex added debugger vm Issues and PRs related to the vm subsystem. labels Jan 22, 2016

MylesBorins force-pushed the do-not-double-debug branch 2 times, most recently from 6608d57 to c9f8309 Compare January 22, 2016 19:50

bnoordhuis reviewed Jan 22, 2016
View reviewed changes

MylesBorins force-pushed the do-not-double-debug branch from 9dcfa74 to 428ac5f Compare January 22, 2016 19:59

MylesBorins force-pushed the do-not-double-debug branch from 428ac5f to 56d54df Compare January 22, 2016 20:03

bnoordhuis reviewed Jan 22, 2016
View reviewed changes

MylesBorins force-pushed the do-not-double-debug branch from 56d54df to d491ddf Compare January 22, 2016 20:05

MylesBorins force-pushed the do-not-double-debug branch from d491ddf to c0efa65 Compare January 23, 2016 01:43

jasnell mentioned this pull request Jan 23, 2016

In "node debug" in a certain reproducible scenario, typing "c" twice causes node to completley hang #4651

Closed

MylesBorins mentioned this pull request Jan 25, 2016

Broken example for vm.runInDebugContext(code) #4862

Closed

MylesBorins self-assigned this Jan 25, 2016

MylesBorins mentioned this pull request Jan 26, 2016

Debugger protocol hangs in some special situations #4597

Closed

MylesBorins force-pushed the do-not-double-debug branch from c0efa65 to 86a94ac Compare January 27, 2016 01:04

MylesBorins assigned bnoordhuis and unassigned MylesBorins Feb 3, 2016

Trott reviewed Feb 5, 2016
View reviewed changes

MylesBorins mentioned this pull request Feb 5, 2016

util.inspect({level: "info"}) exits the debug session #4440

Closed

mikermcneil mentioned this pull request Feb 5, 2016

Sails 0.12.0-rc6 debug error : Segmentation fault balderdashy/sails#3538

Closed

chrisradek mentioned this pull request Feb 5, 2016

Logger facility causes crash or stall with NodeJs 4.2.5+ aws/aws-sdk-js#897

Closed

bnoordhuis reviewed Feb 5, 2016
View reviewed changes

MylesBorins force-pushed the do-not-double-debug branch 2 times, most recently from 9b84a8c to 54b8693 Compare February 5, 2016 22:41

MylesBorins force-pushed the do-not-double-debug branch from 54b8693 to 86aa7db Compare February 5, 2016 23:40

MylesBorins force-pushed the do-not-double-debug branch from 86aa7db to 4897f94 Compare February 5, 2016 23:41

MylesBorins merged commit 4897f94 into nodejs:master Feb 5, 2016

MylesBorins deleted the do-not-double-debug branch February 8, 2016 18:46

gzzhanghao mentioned this pull request Feb 10, 2016

Complete application crash when console.log()ing an object node-inspector/node-inspector#823

Closed

MylesBorins added lts-watch-v4.x and removed lts-watch-v4.x labels Feb 11, 2016

MylesBorins mentioned this pull request Apr 5, 2016

When debugging, any console logging that outputs an object is terminating the debugger. #6066

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node_contextify: do not incept debug context #4819

node_contextify: do not incept debug context #4819

MylesBorins commented Jan 22, 2016

mscdex commented Jan 22, 2016

mscdex Jan 22, 2016

MylesBorins Jan 22, 2016

MylesBorins commented Jan 22, 2016

bnoordhuis Jan 22, 2016

MylesBorins Jan 22, 2016

MylesBorins commented Jan 22, 2016

bnoordhuis Jan 22, 2016

MylesBorins Jan 22, 2016

MylesBorins Jan 22, 2016

bnoordhuis commented Jan 22, 2016

MylesBorins commented Jan 22, 2016

bnoordhuis commented Jan 22, 2016

MylesBorins commented Jan 22, 2016

bnoordhuis commented Jan 22, 2016

ofrobots commented Jan 22, 2016

bnoordhuis commented Jan 22, 2016

MylesBorins commented Jan 22, 2016

bnoordhuis commented Jan 22, 2016

MylesBorins commented Jan 22, 2016

jasnell commented Jan 23, 2016

MylesBorins commented Feb 3, 2016

Trott Feb 5, 2016

Trott commented Feb 5, 2016

indutny commented Feb 5, 2016

bnoordhuis Feb 5, 2016

MylesBorins Feb 5, 2016

bnoordhuis commented Feb 5, 2016

MylesBorins commented Feb 5, 2016

MylesBorins commented Feb 5, 2016

node_contextify: do not incept debug context #4819

node_contextify: do not incept debug context #4819

Conversation

MylesBorins commented Jan 22, 2016

mscdex commented Jan 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins commented Jan 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins commented Jan 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Jan 22, 2016

MylesBorins commented Jan 22, 2016

bnoordhuis commented Jan 22, 2016

MylesBorins commented Jan 22, 2016

bnoordhuis commented Jan 22, 2016

ofrobots commented Jan 22, 2016

bnoordhuis commented Jan 22, 2016

MylesBorins commented Jan 22, 2016

bnoordhuis commented Jan 22, 2016

MylesBorins commented Jan 22, 2016

jasnell commented Jan 23, 2016

MylesBorins commented Feb 3, 2016

Choose a reason for hiding this comment

Trott commented Feb 5, 2016

indutny commented Feb 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Feb 5, 2016

MylesBorins commented Feb 5, 2016

MylesBorins commented Feb 5, 2016