Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: add guard to originalConsole #12881

Closed
wants to merge 1 commit into from

Conversation

@danbev
Copy link
Member

commented May 7, 2017

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

This comment has been minimized.

Copy link
Member Author

commented May 7, 2017

lib/internal/bootstrap_node.js Outdated
try {
inspector = process.binding('inspector');
} catch (ignored) {
return wrappedConsole;

This comment has been minimized.

Copy link
@refack

refack May 7, 2017

Member

You skipping over loop in L280.

@refack

This comment has been minimized.

Copy link
Member

commented May 7, 2017

@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

@refack refack requested a review from eugeneo May 7, 2017

@mscdex mscdex added the inspector label May 7, 2017

lib/internal/bootstrap_node.js Outdated
inspector = process.binding('inspector');
} catch (ignored) {
return wrappedConsole;
}
const config = {};
for (const key of Object.keys(wrappedConsole)) {
if (!globalConsole.hasOwnProperty(key))

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye May 8, 2017

Contributor

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

This comment has been minimized.

Copy link
@danbev

danbev May 8, 2017

Author Member

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

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

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

Thanks, will take a closer at this now.

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

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

lib/internal/bootstrap_node.js Outdated
@@ -261,7 +261,8 @@
enumerable: true,
get: function() {
if (!console) {
console = installInspectorConsole(originalConsole);
console = originalConsole ? installInspectorConsole(originalConsole) :

This comment has been minimized.

Copy link
@refack

refack May 9, 2017

Member

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

@eugeneo
eugeneo approved these changes May 9, 2017
@cjihrig
cjihrig approved these changes May 9, 2017
Copy link
Contributor

left a comment

LGTM with a comment.

lib/internal/bootstrap_node.js Outdated
@@ -261,7 +261,8 @@
enumerable: true,
get: function() {
if (!console) {
console = installInspectorConsole(originalConsole);
console = originalConsole ? installInspectorConsole(originalConsole) :

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 9, 2017

Contributor

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

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye May 9, 2017

Contributor

Ya, even I was thinking about.

This comment has been minimized.

Copy link
@danbev

danbev May 10, 2017

Author Member

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

@refack
refack approved these changes May 9, 2017
lib: add guard to originalConsole
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 danbev force-pushed the danbev:bootstrap-without-ssl branch to 96d228c May 10, 2017

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 10, 2017

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

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:
danbev added a commit to danbev/node that referenced this pull request May 11, 2017
lib: add guard to originalConsole
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

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

Landed in 54d3318

@danbev danbev closed this May 11, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
lib: add guard to originalConsole
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>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

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.

@danbev danbev deleted the danbev:bootstrap-without-ssl branch Jun 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.