lib: add guard to originalConsole by danbev · Pull Request #12881 · nodejs/node

danbev · 2017-05-07T12:42:49Z

Currently when building --without-ssl or --without-inspector there will
be an error when trying to set up the console in bootstrap_node.js:

Can't determine the arch of: 'out/Release/node'
bootstrap_node.js:276
      if (!globalConsole.hasOwnProperty(key))
                        ^

TypeError: Cannot read property 'hasOwnProperty' of undefined
    at installInspectorConsole (bootstrap_node.js:276:25)
    at get (bootstrap_node.js:264:21)
    at evalScript (bootstrap_node.js:395:30)
    at startup (bootstrap_node.js:125:9)
    at bootstrap_node.js:537:3

I think this issue was introduced in commit
3f48ab3 ("inspector: do not add
'inspector' property").

This commit attempts to fix this.

Checklist

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

Affected core subsystem(s)

danbev · 2017-05-07T12:58:08Z

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

refack · 2017-05-07T15:13:40Z

lib/internal/bootstrap_node.js

You skipping over loop in L280.

refack · 2017-05-07T15:18:23Z

@danbev IMHO you need to do the bypass higher up the call stack...

Ping @eugeneo, if there's no inspector there should be no need to wrap global.console at all. Essentially bypass all the changes to node_bootstrap from 3f48ab3#diff-cc63475f116262e9cbea02374e344c01

thefourtheye · 2017-05-08T15:32:27Z

lib/internal/bootstrap_node.js

The error message suggests that globalConsole is undefined, right? Shouldn't the fix be installing a guard for it?

Yep, that is correct. I'm not sure now what I was thinking as that sounds much more sensible (or if I ran into some different issue doing that) but I'll take a closer look at this tomorrow.

danbev · 2017-05-09T08:43:25Z

@danbev IMHO you need to do the bypass higher up the call stack...

Thanks, will take a closer at this now.

danbev · 2017-05-09T10:30:55Z

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

refack · 2017-05-09T13:14:04Z

lib/internal/bootstrap_node.js

this makes much more sense 👍
Is this the right check? i.e. if inspector is "on" global.console will be true?

cjihrig

LGTM with a comment.

cjihrig · 2017-05-09T16:47:20Z

lib/internal/bootstrap_node.js

Can the originalConsole check be stricter? Such as checking explicitly against undefined.

Ya, even I was thinking about.

Yeah, that sounds better. I'll add a more strict check.

Currently when building --without-ssl or --without-inspector there will be an error when trying to set up the console in bootstrap_node.js: Can't determine the arch of: 'out/Release/node' bootstrap_node.js:276 if (!globalConsole.hasOwnProperty(key)) ^ TypeError: Cannot read property 'hasOwnProperty' of undefined at installInspectorConsole (bootstrap_node.js:276:25) at get (bootstrap_node.js:264:21) at evalScript (bootstrap_node.js:395:30) at startup (bootstrap_node.js:125:9) at bootstrap_node.js:537:3 I think this issue was introduced in commit 3f48ab3 ("inspector: do not add 'inspector' property"). This commit attempts to fix this.

danbev · 2017-05-10T06:10:26Z

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

danbev · 2017-05-11T05:12:35Z

test/windows-fanned looks unrelated to this PR:

ERROR: Error fetching remote repo 'jenkins_tmp'
hudson.plugins.git.GitException: Failed to fetch from git@github.com:janeasystems/node_binary_tmp.git
	at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:809)
	at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1076)
	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1107)
	at hudson.scm.SCM.checkout(SCM.java:496)
	at hudson.model.AbstractProject.checkout(AbstractProject.java:1281)
	at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:604)
	at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:529)
	at hudson.model.Run.execute(Run.java:1728)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
	at hudson.model.ResourceController.execute(ResourceController.java:98)
	at hudson.model.Executor.run(Executor.java:405)
Caused by: hudson.plugins.git.GitException: Command "git fetch --tags --progress git@github.com:janeasystems/node_binary_tmp.git +refs/heads/jenkins-node-test-commit-windows-fanned-8962-binary-windows/*:refs/remotes/jenkins_tmp/jenkins-node-test-commit-windows-fanned-8962-binary-windows/*" returned status code 128:

Currently when building --without-ssl or --without-inspector there will be an error when trying to set up the console in bootstrap_node.js: Can't determine the arch of: 'out/Release/node' bootstrap_node.js:276 if (!globalConsole.hasOwnProperty(key)) ^ TypeError: Cannot read property 'hasOwnProperty' of undefined at installInspectorConsole (bootstrap_node.js:276:25) at get (bootstrap_node.js:264:21) at evalScript (bootstrap_node.js:395:30) at startup (bootstrap_node.js:125:9) at bootstrap_node.js:537:3 I think this issue was introduced in commit 3f48ab3 ("inspector: do not add 'inspector' property"). This commit attempts to fix this. PR-URL: nodejs#12881 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

danbev · 2017-05-11T05:32:59Z

Landed in 54d3318

Currently when building --without-ssl or --without-inspector there will be an error when trying to set up the console in bootstrap_node.js: Can't determine the arch of: 'out/Release/node' bootstrap_node.js:276 if (!globalConsole.hasOwnProperty(key)) ^ TypeError: Cannot read property 'hasOwnProperty' of undefined at installInspectorConsole (bootstrap_node.js:276:25) at get (bootstrap_node.js:264:21) at evalScript (bootstrap_node.js:395:30) at startup (bootstrap_node.js:125:9) at bootstrap_node.js:537:3 I think this issue was introduced in commit 3f48ab3 ("inspector: do not add 'inspector' property"). This commit attempts to fix this. PR-URL: nodejs#12881 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

gibfahn · 2017-06-20T23:12:01Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 7, 2017

refack suggested changes May 7, 2017

View reviewed changes

lib/internal/bootstrap_node.js Outdated

Copy link

Contributor

refack May 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You skipping over loop in L280.

refack requested a review from eugeneo May 7, 2017 15:18

mscdex added the inspector Issues and PRs related to the V8 inspector protocol label May 7, 2017

thefourtheye reviewed May 8, 2017

View reviewed changes

danbev changed the title ~~lib: add try/catch to inspector binding~~ lib: add guard to originalConsole May 9, 2017

refack reviewed May 9, 2017

View reviewed changes

eugeneo approved these changes May 9, 2017

View reviewed changes

cjihrig approved these changes May 9, 2017

View reviewed changes

thefourtheye approved these changes May 9, 2017

View reviewed changes

refack approved these changes May 9, 2017

View reviewed changes

danbev force-pushed the bootstrap-without-ssl branch from bfe87c1 to 96d228c Compare May 10, 2017 06:00

danbev closed this May 11, 2017

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added the backport-requested-v6.x label Jun 20, 2017

danbev deleted the bootstrap-without-ssl branch June 28, 2017 11:35

Uh oh!

Conversation

danbev commented May 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

danbev commented May 7, 2017

Uh oh!

refack May 7, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented May 7, 2017

Uh oh!

thefourtheye May 8, 2017

Choose a reason for hiding this comment

Uh oh!

danbev May 8, 2017

Choose a reason for hiding this comment

Uh oh!

danbev commented May 9, 2017

Uh oh!

danbev commented May 9, 2017

Uh oh!

refack May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig May 9, 2017

Choose a reason for hiding this comment

Uh oh!

thefourtheye May 9, 2017

Choose a reason for hiding this comment

Uh oh!

danbev May 10, 2017

Choose a reason for hiding this comment

Uh oh!

danbev commented May 10, 2017

Uh oh!

danbev commented May 11, 2017

Uh oh!

danbev commented May 11, 2017

Uh oh!

gibfahn commented Jun 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

danbev commented May 7, 2017 •

edited

Loading

refack May 9, 2017 •

edited

Loading