lib: guard inspector console using process var #15008

danbev · 2017-08-24T10:31:28Z

Currently the inspector module is always loaded and if it does not
return anything the inspector console setup is skipped.

This commit uses the process.config.variables.v8_enable_inspector
variable to only load the inspector module if it is enabled.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

lib

danbev · 2017-08-24T10:32:25Z

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

BridgeAR · 2017-09-01T00:15:36Z

lib/internal/bootstrap_node.js

-      console.timeEnd;
-      console.trace;
-      console.warn;
+      if (process.config.v8_enable_inspector) {


It would be nice to move that condition up in the former if.

Sounds good, I'll move it. Thanks

danbev · 2017-09-01T05:47:08Z

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

BridgeAR · 2017-09-01T05:53:38Z

lib/internal/bootstrap_node.js

@@ -54,7 +54,7 @@
    NativeModule.require('internal/process/warning').setup();
    NativeModule.require('internal/process/next_tick').setup();
    NativeModule.require('internal/process/stdio').setup();
-    if (browserGlobals) {
+    if (browserGlobals && process.config.v8_enable_inspector) {


I think the name is actually process.config.variables.v8_enable_inspector?

You are right, can't believe I missed that. 😞 Will update now. Thanks again!

BridgeAR · 2017-09-01T06:26:56Z

You asked for the reason - If I am correct NativeModule.require('module') would have to be called first and that is not the case anymore.

danbev · 2017-09-01T07:36:05Z

If I am correct NativeModule.require('module') would have to be called first and that is not the case anymore.

Thanks! Should we then perhaps do something like the following to still allow for the eager instantiation to happen:

    if (browserGlobals) {
      if (!process.config.variables.v8_enable_inspector) {
        NativeModule.require('module');
      }
      // Instantiate eagerly in case the first call is under stack overflow
      // conditions where instantiation doesn't work.
      const console = global.console;
      console.assert;
      console.clear;
      console.count;
      console.countReset;
      console.dir;
      console.error;
      console.log;
      console.time;
      console.timeEnd;
      console.trace;
      console.warn;
    }

BridgeAR · 2017-09-02T06:42:06Z

I would say let us wait for #15111. It should actually also solve the issue here and allow the eager instantiation. I do like the variable part a lot though and I guess it makes sense to switch the inspector check in the mentioned PR to this check instead.

BridgeAR · 2017-09-19T23:22:44Z

@danbev the mentioned PR just landed. I guess the issue itself should already be fixed but right now the inspector is always "loaded" and it is checked if it actually returns anything or not. Therefore it would still be nice to use process.config.variables.v8_enable_inspector instead. I did not add that to my PR as I forgot about that.

danbev · 2017-09-25T06:28:59Z

@BridgeAR Sorry about the late reply (was at a f2f last week and had limited time). I'll take another look into this issue now. Thanks

danbev · 2017-09-25T07:18:05Z

Rebased and updated CI: https://ci.nodejs.org/job/node-test-pull-request/10249/

jasnell · 2017-09-25T15:41:35Z

lib/internal/bootstrap_node.js

@@ -319,10 +319,10 @@
  }

  function setupInspector(originalConsole, wrappedConsole, Module) {
-    const { addCommandLineAPI, consoleCall } = process.binding('inspector');
-    if (!consoleCall) {
+    if (!process.config.variables.v8_enable_inspector) {


Depending on process.config is dangerous because there are userland modules that completely replace it. During bootstrap it should be safe but problems still could come up.

If it ever becomes an issue, we could cache it before user code runs.

Or use process.binding('config') as a stable alternative.

@jasnell @cjihrig Thanks, I had not thought about that being an issue and I'm trying to understand how this might happen.

Using process.binding('config') will give access to the builtin module config, but as far as I can tell the information from config.gypi is provided as a native module (via node_js2c). The process object is then configured with that information in setupConfig which is called by node_bootstrap.js start function.

setupConfig deletes the config from the _source and then sets these properties on the process object. Since it deletes the _source this function cannot be called multiple times, for example if it did not we could have done the same thing again.

What I'm having some difficulties understand is that how could a userland module be able to replace this at this stage. Would someone be able to shed some light on this?

Userland code might not be able to replace process.config by this stage, I haven't tested it. What I do know is that we've had issues depending on process.config in later stages elsewhere in the code and we if we are going to use it here then we need to be certain that it won't be a problem. If there is no userland code that can be run before bootstrap gets to this point, then it should be ok.

This is still very early in the bootstrap process and I do not think it is possible to change it at that stage. To be on the safe side we could just add a test for it though.

@jasnell @BridgeAR Sounds good, I'll add a test for this. Thanks

BridgeAR · 2017-10-02T00:11:59Z

test/parallel/test-overwrite-config.js

+const assert = require('assert');
+assert.strictEqual(process.config.variables.v8_enable_inspector, 1);
+process.config.variables.v8_enable_inspector = 100;
+assert.strictEqual(process.config.variables.v8_enable_inspector, 100);


I am not sure if this is a appropriate test.
I would have expected the v8_enable_inspector variable to be set to something falsy because we test it to be truthy and afterwards to check if the inspector is set up nevertheless.
Another approach would be to test that inspector is set without any variable manipulation. That should already verify that manipulating it has no impact on this anymore.

@BridgeAR Thanks, I've updated the test. When going through the code I looks like any user land code will be call by Module.runMain, at which point the checking of process.config.variables.v8_enable_inspector done by bootstrap_node.js has already been performed. This also holds true for any preloaded modules. So as far as I can tell the check should be safe unless there are other ways this could happen that I've not taken into account.

danbev · 2017-10-05T07:22:22Z

@BridgeAR Would you mind taking another look at this now and see what you thing? Thanks

danbev · 2017-10-17T05:53:45Z

@BridgeAR ping

BridgeAR · 2017-10-19T05:31:23Z

test/parallel/test-overwrite-config.js

+const assert = require('assert');
+assert.strictEqual(process.config.variables.v8_enable_inspector, 1);
+process.config.variables.v8_enable_inspector = null;
+assert.strictEqual(process.config.variables.v8_enable_inspector, null);


How do we know here that the inspector is indeed set up properly (I would have expected to test for inspector specific functions or similar)? That is the most important part out of my perspective. As soon as we know that the inspector is set up, we do not even have to manipulate the variable anymore, because it would not have any effect afterwards.

@BridgeAR Thanks for the feedback. I'm not working for a few days but will take another look at this next week.

jasnell · 2017-10-20T17:18:08Z

Thank you for adding the test, but I'm not quite sure it's enough. The earliest user code that can run would be a preload module using -r. The test could likely be improved by having it run with a preload module that sets process.config = {} then checking that the inspector code still works as required.

danbev · 2017-10-24T12:19:12Z

@BridgeAR @jasnell Sorry about the delay on this. I've taken another stab at this now, please take a look when you get a chance and let me know what you think.

danbev · 2017-11-09T13:51:22Z

@jasnell Would you able to take another look at the test for this and see what you think?

bnoordhuis

Still LGTM.

Currently the inspector module is always loaded and if it does not return anything the inspector console setup is skipped. This commit uses the process.config.variables.v8_enable_inspector variable to only load the inspector module if it is enabled.

danbev · 2017-11-13T06:30:12Z

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

danbev · 2017-11-13T07:32:30Z

Landed in 1aac4c1

Currently the inspector module is always loaded and if it does not return anything the inspector console setup is skipped. This commit uses the process.config.variables.v8_enable_inspector variable to only load the inspector module if it is enabled. PR-URL: #15008 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 24, 2017

jasnell requested a review from bnoordhuis August 24, 2017 15:19

BridgeAR reviewed Sep 1, 2017

View reviewed changes

danbev force-pushed the guard-eager-console-init branch from 17a73ac to b17c383 Compare September 1, 2017 05:46

BridgeAR reviewed Sep 1, 2017

View reviewed changes

BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Sep 13, 2017

BridgeAR mentioned this pull request Sep 15, 2017

lib: refactor console bootstrap #15111

Closed

4 tasks

BridgeAR removed the blocked PRs that are blocked by other issues or PRs. label Sep 19, 2017

danbev force-pushed the guard-eager-console-init branch from 436dd13 to 87b1b97 Compare September 25, 2017 07:16

danbev changed the title ~~lib: guard console instantiation --wihtout-ssl~~ lib: guard inspector console using process var Sep 25, 2017

bnoordhuis approved these changes Sep 25, 2017

View reviewed changes

jasnell reviewed Sep 25, 2017

View reviewed changes

danbev force-pushed the guard-eager-console-init branch from 87b1b97 to 5a94460 Compare September 29, 2017 07:46

BridgeAR reviewed Oct 2, 2017

View reviewed changes

BridgeAR reviewed Oct 19, 2017

View reviewed changes

danbev force-pushed the guard-eager-console-init branch from 6c47862 to 9395fca Compare October 24, 2017 11:47

bnoordhuis approved these changes Nov 9, 2017

View reviewed changes

jasnell approved these changes Nov 10, 2017

View reviewed changes

danbev force-pushed the guard-eager-console-init branch from 9395fca to 87f3f91 Compare November 13, 2017 06:29

danbev closed this Nov 13, 2017

danbev deleted the guard-eager-console-init branch November 13, 2017 07:51

evanlucas mentioned this pull request Nov 13, 2017

v9.2.0 proposal #16992

Merged

MylesBorins added dont-land-on-v6.x labels Nov 17, 2017

gibfahn mentioned this pull request Nov 21, 2017

v8.9.2 proposal #17204

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: guard inspector console using process var #15008

lib: guard inspector console using process var #15008

danbev commented Aug 24, 2017 •

edited

Loading

danbev commented Aug 24, 2017

BridgeAR Sep 1, 2017

danbev Sep 1, 2017

danbev commented Sep 1, 2017

BridgeAR Sep 1, 2017

danbev Sep 1, 2017

BridgeAR commented Sep 1, 2017

danbev commented Sep 1, 2017

BridgeAR commented Sep 2, 2017 •

edited

Loading

BridgeAR commented Sep 19, 2017

danbev commented Sep 25, 2017

danbev commented Sep 25, 2017

jasnell Sep 25, 2017

cjihrig Sep 25, 2017

jasnell Sep 25, 2017

danbev Sep 26, 2017

jasnell Sep 26, 2017

BridgeAR Sep 27, 2017

danbev Sep 28, 2017

BridgeAR Oct 2, 2017

danbev Oct 2, 2017

danbev commented Oct 5, 2017

danbev commented Oct 17, 2017

BridgeAR Oct 19, 2017

danbev Oct 19, 2017

jasnell commented Oct 20, 2017

danbev commented Oct 24, 2017

danbev commented Nov 9, 2017

bnoordhuis left a comment

danbev commented Nov 13, 2017

danbev commented Nov 13, 2017

lib: guard inspector console using process var #15008

lib: guard inspector console using process var #15008

Conversation

danbev commented Aug 24, 2017 • edited Loading

Checklist

Affected core subsystem(s)

danbev commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Sep 1, 2017

danbev commented Sep 1, 2017

BridgeAR commented Sep 2, 2017 • edited Loading

BridgeAR commented Sep 19, 2017

danbev commented Sep 25, 2017

danbev commented Sep 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented Oct 5, 2017

danbev commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Oct 20, 2017

danbev commented Oct 24, 2017

danbev commented Nov 9, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

danbev commented Nov 13, 2017

danbev commented Nov 13, 2017

danbev commented Aug 24, 2017 •

edited

Loading

BridgeAR commented Sep 2, 2017 •

edited

Loading