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.

Sign up for GitHub

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

Jump to bottom

inspector: fix crash on exception #13455

Merged
merged 0 commits into from
Jun 9, 2017
Merged

inspector: fix crash on exception #13455

merged 0 commits into from
Jun 9, 2017

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Jun 4, 2017

Fixes #13438.

cc @eugeneo

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol labels Jun 4, 2017
addaleax
addaleax approved these changes Jun 4, 2017
View reviewed changes
test/inspector/test-inspector.js Outdated
return function(result) {
const expected = helper.mainScriptSource(mainScript);
const source = result['scriptSource'];
assert(source && (source.includes(expected)),
Copy link
Member

Choose a reason for hiding this comment

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

The “middle” parentheses are unnecessary here

refack
refack approved these changes Jun 4, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Wanted to do this myself 😃

test/inspector/inspector-helper.js Outdated
const args = [].concat(inspectorFlags);
if (opt_script_contents) {
if (!mainScript.endsWith('.js')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a guarantee that typeof mainScript.endsWith === 'function'?
Maybe try !String.prototype.endsWith(mainScript, '.js')
Also flip if/else and remove !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an undocumented internal function. I think it's safe to assume that whoever wants to add a new test will look at the code to figure out how it works (just like I did).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack.
But I'm still +1(non-blocking) on inverting

@refack
Copy link
Contributor

refack commented Jun 4, 2017 •
edited by gibfahn
Loading

I would like to see a regression test in /parallel/

var child = spawn(process.argv[0], ['inspect', `${common.fixturesDir}/throws_error2.js`])
child.stdin.write('c\n');

that should cause is to explode (before this fix)

@refack
Copy link
Contributor

refack commented Jun 5, 2017

Two approvals, so it's CI time: https://ci.nodejs.org/job/node-test-commit/10357/

@refack
Copy link
Contributor

refack commented Jun 5, 2017

Regression is in inspector-helper.js:502

/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/inspector/inspector-helper.js:502
    args.push('-e', opt_script_contents);
                    ^

ReferenceError: opt_script_contents is not defined
    at Object.exports.startNodeForInspectorTest (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/inspector/inspector-helper.js:502:21)
    at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/inspector/test-off-no-session.js:10:8)
    at Module._compile (module.js:569:30)

@seishun
Copy link
Contributor Author

seishun commented Jun 5, 2017

Fixed the regression and flipped the if/else.

Adding a test for the "inspect" command seems redundant (and less direct).

@refack
Copy link
Contributor

refack commented Jun 5, 2017

Fixed the regression and flipped the if/else.

👍

Adding a test for the "inspect" command seems redundant (and less direct).

🤷‍♂️

@refack
Copy link
Contributor

refack commented Jun 5, 2017

Quick CI (linuxone): https://ci.nodejs.org/job/node-test-commit-linuxone/6390/
3
2
1
and it's ✔️
I just love it that it finishes in less than 3 minutes

@seishun seishun requested a review from eugeneo June 7, 2017 19:17
@seishun
Copy link
Contributor Author

seishun commented Jun 7, 2017

Rebased on master, reworked the test a bit. PTAL again.

@seishun seishun closed this Jun 9, 2017
@seishun seishun merged commit 9991e27 into nodejs:master Jun 9, 2017
@seishun seishun deleted the inspector-crash branch June 9, 2017 12:01
addaleax pushed a commit that referenced this pull request Jun 10, 2017
@seishun @addaleax
inspector: fix crash on exception
6a696d1 
Fixes: #13438
PR-URL: #13455
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@addaleax addaleax mentioned this pull request Jun 10, 2017
Merged
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@addaleax addaleax addaleax approved these changes

@refack refack refack approved these changes

@eugeneo eugeneo Awaiting requested review from eugeneo

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Exception after "break on start" causes hard crash
7 participants
@seishun @refack @addaleax @TimothyGu @cjihrig @MylesBorins @nodejs-github-bot