Skip to content

Commit

Permalink
src: Only allow --debug-brk w/ --inspect
Browse files Browse the repository at this point in the history
  • Loading branch information
Jan Krems committed May 4, 2017
1 parent 3426b93 commit 50ecfe4
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 4 deletions.
6 changes: 6 additions & 0 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3955,6 +3955,12 @@ static void ParseArgs(int* argc,
exit(9);
}

if (debug_options.debug_brk_used() && !debug_options.inspector_enabled()) {
fprintf(stderr,
"%s: Using --debug-brk without --inspect is no longer supported\n", argv[0]);
exit(9);
}

// Copy remaining arguments.
const unsigned int args_left = nargs - index;

Expand Down
7 changes: 5 additions & 2 deletions src/node_debug_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ DebugOptions::DebugOptions() :
#if HAVE_INSPECTOR
inspector_enabled_(false),
#endif // HAVE_INSPECTOR
debug_brk_used_(false),
wait_connect_(false), http_enabled_(false),
host_name_("127.0.0.1"), port_(-1) { }

Expand All @@ -79,10 +80,12 @@ bool DebugOptions::ParseOption(const std::string& option) {

if (option_name == "--inspect") {
enable_inspector = true;
} else if (option_name == "--inspect-brk" || option_name == "--debug-brk") {
debugger_enabled_ = true;
} else if (option_name == "--inspect-brk") {
enable_inspector = true;
wait_connect_ = true;
} else if (option_name == "--debug-brk") {
debug_brk_used_ = true;
wait_connect_ = true;
} else if (option_name == "--inspect-port" || option_name == "--debug-port") {
if (!has_argument) return false;
} else {
Expand Down
4 changes: 4 additions & 0 deletions src/node_debug_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ class DebugOptions {
return false;
#endif // HAVE_INSPECTOR
}
bool debug_brk_used() const {
return debug_brk_used_;
}
bool ToolsServerEnabled();
bool wait_for_connect() const { return wait_connect_; }
std::string host_name() const { return host_name_; }
Expand All @@ -26,6 +29,7 @@ class DebugOptions {
private:
#if HAVE_INSPECTOR
bool inspector_enabled_;
bool debug_brk_used_;
#endif // HAVE_INSPECTOR
bool wait_connect_; // --inspect-brk
bool http_enabled_;
Expand Down
4 changes: 2 additions & 2 deletions test/inspector/inspector-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -442,9 +442,9 @@ Harness.prototype.kill = function() {
};

exports.startNodeForInspectorTest = function(callback,
inspectorFlag = '--inspect-brk',
inspectorFlags = ['--inspect-brk'],
opt_script_contents) {
const args = [inspectorFlag];
const args = [].concat(inspectorFlags);
if (opt_script_contents) {
args.push('-e', opt_script_contents);
} else {
Expand Down
13 changes: 13 additions & 0 deletions test/inspector/test-inspector-debug-brk-only.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
'use strict';
const assert = require('assert');
const execFile = require('child_process').execFile;
const path = require('path');

const common = require('../common');

const mainScript = path.join(common.fixturesDir, 'loop.js');

execFile(process.execPath, [ '--debug-brk', mainScript ], common.mustCall((error, stdout, stderr) => {
assert.equal(error.code, 9);
assert.notEqual(stderr.indexOf('Using --debug-brk without --inspect is no longer supported', -1));
}));
57 changes: 57 additions & 0 deletions test/inspector/test-inspector-debug-brk.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
'use strict';
const common = require('../common');
common.skipIfInspectorDisabled();
const assert = require('assert');
const helper = require('./inspector-helper.js');

function setupExpectBreakOnLine(line, url, session, scopeIdCallback) {
return function(message) {
if ('Debugger.paused' === message['method']) {
const callFrame = message['params']['callFrames'][0];
const location = callFrame['location'];
assert.strictEqual(url, session.scriptUrlForId(location['scriptId']));
assert.strictEqual(line, location['lineNumber']);
scopeIdCallback &&
scopeIdCallback(callFrame['scopeChain'][0]['object']['objectId']);
return true;
}
};
}

function testBreakpointOnStart(session) {
const commands = [
{ 'method': 'Runtime.enable' },
{ 'method': 'Debugger.enable' },
{ 'method': 'Debugger.setPauseOnExceptions',
'params': {'state': 'none'} },
{ 'method': 'Debugger.setAsyncCallStackDepth',
'params': {'maxDepth': 0} },
{ 'method': 'Profiler.enable' },
{ 'method': 'Profiler.setSamplingInterval',
'params': {'interval': 100} },
{ 'method': 'Debugger.setBlackboxPatterns',
'params': {'patterns': []} },
{ 'method': 'Runtime.runIfWaitingForDebugger' }
];

session
.sendInspectorCommands(commands)
.expectMessages(setupExpectBreakOnLine(0, session.mainScriptPath, session));
}

function testWaitsForFrontendDisconnect(session, harness) {
console.log('[test]', 'Verify node waits for the frontend to disconnect');
session.sendInspectorCommands({ 'method': 'Debugger.resume'})
.expectStderrOutput('Waiting for the debugger to disconnect...')
.disconnect(true);
}

function runTests(harness) {
harness
.runFrontendSession([
testBreakpointOnStart,
testWaitsForFrontendDisconnect
]).expectShutDown(55);
}

helper.startNodeForInspectorTest(runTests, ['--inspect', '--debug-brk']);

1 comment on commit 50ecfe4

@jkrems
Copy link
Owner

@jkrems jkrems commented on 50ecfe4 May 10, 2017

Choose a reason for hiding this comment

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

but actual behavior of --debug-brk will have changed, so we should just print a message and exit.

/cc @ChALkeR FYI - this was my idea for solving the compatibility issue around the change in behavior: Only allowing --debug-brk if it's used w/ --inspect and failing otherwise.

Please sign in to comment.