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

inspector,vm: remove --eval wrapper #25832

Closed
wants to merge 1 commit into from
Closed

inspector,vm: remove --eval wrapper #25832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions lib/internal/process/execution.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,26 +33,30 @@ function tryGetCwd() {
}
}

function evalScript(name, body, breakFristLine) {
function evalScript(name, body, breakFirstLine) {
const CJSModule = require('internal/modules/cjs/loader');
if (breakFristLine) {
const fn = `function() {\n\n${body};\n\n}`;
body = `process.binding('inspector').callAndPauseOnStart(${fn}, {})`;
}
const { kVmBreakFirstLineSymbol } = require('internal/util');

const cwd = tryGetCwd();

const module = new CJSModule(name);
module.filename = path.join(cwd, name);
module.paths = CJSModule._nodeModulePaths(cwd);
const script = `global.__filename = ${JSON.stringify(name)};\n` +
'global.exports = exports;\n' +
'global.module = module;\n' +
'global.__dirname = __dirname;\n' +
'global.require = require;\n' +
'return require("vm").runInThisContext(' +
`${JSON.stringify(body)}, { filename: ` +
`${JSON.stringify(name)}, displayErrors: true });\n`;
global.kVmBreakFirstLineSymbol = kVmBreakFirstLineSymbol;
const script = `
global.__filename = ${JSON.stringify(name)};
global.exports = exports;
global.module = module;
global.__dirname = __dirname;
global.require = require;
const { kVmBreakFirstLineSymbol } = global;
delete global.kVmBreakFirstLineSymbol;
return require("vm").runInThisContext(
${JSON.stringify(body)}, {
filename: ${JSON.stringify(name)},
displayErrors: true,
[kVmBreakFirstLineSymbol]: ${!!breakFirstLine}
});\n`;
Copy link
Member

Choose a reason for hiding this comment

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

@addaleax The kVmBreakFirstLineSymbol looks pretty handy. Is there a reason to keep it hidden and not exposed as a user facing option like a boolean breakFirstLineSymbol options property?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jdalton Err, it’s less work for this PR? 😄 I’m not sure, but I guess it also only works while the inspector is enabled, similar to debugger; statements?

Somebody from @nodejs/v8-inspector might know more.

const result = module._compile(script, `${name}-wrapper`);
if (require('internal/options').getOptionValue('--print')) {
console.log(result);
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,5 +418,6 @@ module.exports = {
// Used by the buffer module to capture an internal reference to the
// default isEncoding implementation, just in case userland overrides it.
kIsEncodingSymbol: Symbol('kIsEncodingSymbol'),
kExpandStackSymbol: Symbol('kExpandStackSymbol')
kExpandStackSymbol: Symbol('kExpandStackSymbol'),
kVmBreakFirstLineSymbol: Symbol('kVmBreakFirstLineSymbol')
};
6 changes: 4 additions & 2 deletions lib/vm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const {
validateUint32,
validateString
} = require('internal/validators');
const { kVmBreakFirstLineSymbol } = require('internal/util');
const kParsingContext = Symbol('script parsing context');

const ArrayForEach = Function.call.bind(Array.prototype.forEach);
Expand Down Expand Up @@ -175,7 +176,8 @@ function getRunInContextArgs(options = {}) {

const {
displayErrors = true,
breakOnSigint = false
breakOnSigint = false,
[kVmBreakFirstLineSymbol]: breakFirstLine = false,
} = options;

if (typeof displayErrors !== 'boolean') {
Expand All @@ -189,7 +191,7 @@ function getRunInContextArgs(options = {}) {

return {
breakOnSigint,
args: [timeout, displayErrors, breakOnSigint]
args: [timeout, displayErrors, breakOnSigint, breakFirstLine]
};
}

Expand Down
27 changes: 24 additions & 3 deletions src/node_contextify.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,7 +795,9 @@ void ContextifyScript::RunInThisContext(
TRACE_EVENT_NESTABLE_ASYNC_BEGIN0(
TRACING_CATEGORY_NODE2(vm, script), "RunInThisContext", wrapped_script);

CHECK_EQ(args.Length(), 3);
// TODO(addaleax): Use an options object or otherwise merge this with
// RunInContext().
CHECK_EQ(args.Length(), 4);

CHECK(args[0]->IsNumber());
int64_t timeout = args[0]->IntegerValue(env->context()).FromJust();
Expand All @@ -806,8 +808,16 @@ void ContextifyScript::RunInThisContext(
CHECK(args[2]->IsBoolean());
bool break_on_sigint = args[2]->IsTrue();

CHECK(args[3]->IsBoolean());
bool break_on_first_line = args[3]->IsTrue();

// Do the eval within this context
EvalMachine(env, timeout, display_errors, break_on_sigint, args);
EvalMachine(env,
timeout,
display_errors,
break_on_sigint,
break_on_first_line,
args);

TRACE_EVENT_NESTABLE_ASYNC_END0(
TRACING_CATEGORY_NODE2(vm, script), "RunInThisContext", wrapped_script);
Expand All @@ -819,7 +829,7 @@ void ContextifyScript::RunInContext(const FunctionCallbackInfo<Value>& args) {
ContextifyScript* wrapped_script;
ASSIGN_OR_RETURN_UNWRAP(&wrapped_script, args.Holder());

CHECK_EQ(args.Length(), 4);
CHECK_EQ(args.Length(), 5);

CHECK(args[0]->IsObject());
Local<Object> sandbox = args[0].As<Object>();
Expand All @@ -843,12 +853,16 @@ void ContextifyScript::RunInContext(const FunctionCallbackInfo<Value>& args) {
CHECK(args[3]->IsBoolean());
bool break_on_sigint = args[3]->IsTrue();

CHECK(args[4]->IsBoolean());
bool break_on_first_line = args[4]->IsTrue();

// Do the eval within the context
Context::Scope context_scope(contextify_context->context());
EvalMachine(contextify_context->env(),
timeout,
display_errors,
break_on_sigint,
break_on_first_line,
args);

TRACE_EVENT_NESTABLE_ASYNC_END0(
Expand All @@ -859,6 +873,7 @@ bool ContextifyScript::EvalMachine(Environment* env,
const int64_t timeout,
const bool display_errors,
const bool break_on_sigint,
const bool break_on_first_line,
const FunctionCallbackInfo<Value>& args) {
if (!env->can_call_into_js())
return false;
Expand All @@ -874,6 +889,12 @@ bool ContextifyScript::EvalMachine(Environment* env,
PersistentToLocal::Default(env->isolate(), wrapped_script->script_);
Local<Script> script = unbound_script->BindToCurrentContext();

#if HAVE_INSPECTOR
if (break_on_first_line) {
env->inspector_agent()->PauseOnNextJavascriptStatement("Break on start");
}
#endif

MaybeLocal<Value> result;
bool timed_out = false;
bool received_signal = false;
Expand Down
1 change: 1 addition & 0 deletions src/node_contextify.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ class ContextifyScript : public BaseObject {
const int64_t timeout,
const bool display_errors,
const bool break_on_sigint,
const bool break_on_first_line,
const v8::FunctionCallbackInfo<v8::Value>& args);

inline uint32_t id() { return id_; }
Expand Down
4 changes: 2 additions & 2 deletions test/sequential/test-inspector-async-hook-setup-at-inspect-brk.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ setTimeout(() => {
`;

async function skipBreakpointAtStart(session) {
await session.waitForBreakOnLine(3, '[eval]');
await session.waitForBreakOnLine(1, '[eval]');
await session.send({ 'method': 'Debugger.resume' });
}

async function checkAsyncStackTrace(session) {
console.error('[test]', 'Verify basic properties of asyncStackTrace');
const paused = await session.waitForBreakOnLine(4, '[eval]');
const paused = await session.waitForBreakOnLine(2, '[eval]');
assert(paused.params.asyncStackTrace,
`${Object.keys(paused.params)} contains "asyncStackTrace" property`);
assert(paused.params.asyncStackTrace.description, 'Timeout');
Expand Down
10 changes: 5 additions & 5 deletions test/sequential/test-inspector-async-stack-traces-promise-then.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,18 @@ async function runTests() {
{ 'method': 'Runtime.runIfWaitingForDebugger' }
]);

await session.waitForBreakOnLine(2, '[eval]');
await session.waitForBreakOnLine(0, '[eval]');
await session.send({ 'method': 'Debugger.resume' });

console.error('[test] Waiting for break1');
debuggerPausedAt(await session.waitForBreakOnLine(6, '[eval]'),
'break1', 'runTest:5');
debuggerPausedAt(await session.waitForBreakOnLine(4, '[eval]'),
'break1', 'runTest:3');

await session.send({ 'method': 'Debugger.resume' });

console.error('[test] Waiting for break2');
debuggerPausedAt(await session.waitForBreakOnLine(9, '[eval]'),
'break2', 'runTest:8');
debuggerPausedAt(await session.waitForBreakOnLine(7, '[eval]'),
'break2', 'runTest:6');

await session.runToCompletion();
assert.strictEqual((await instance.expectShutdown()).exitCode, 0);
Expand Down
4 changes: 2 additions & 2 deletions test/sequential/test-inspector-async-stack-traces-set-interval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ const script = 'setInterval(() => { debugger; }, 50);';

async function skipFirstBreakpoint(session) {
console.log('[test]', 'Skipping the first breakpoint in the eval script');
await session.waitForBreakOnLine(2, '[eval]');
await session.waitForBreakOnLine(0, '[eval]');
await session.send({ 'method': 'Debugger.resume' });
}

async function checkAsyncStackTrace(session) {
console.error('[test]', 'Verify basic properties of asyncStackTrace');
const paused = await session.waitForBreakOnLine(2, '[eval]');
const paused = await session.waitForBreakOnLine(0, '[eval]');
assert(paused.params.asyncStackTrace,
`${Object.keys(paused.params)} contains "asyncStackTrace" property`);
assert(paused.params.asyncStackTrace.description, 'Timeout');
Expand Down
2 changes: 1 addition & 1 deletion test/sequential/test-inspector-break-e.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ async function runTests() {
{ 'method': 'Debugger.enable' },
{ 'method': 'Runtime.runIfWaitingForDebugger' }
]);
await session.waitForBreakOnLine(2, '[eval]');
await session.waitForBreakOnLine(0, '[eval]');
await session.runToCompletion();
assert.strictEqual((await instance.expectShutdown()).exitCode, 0);
}
Expand Down
10 changes: 5 additions & 5 deletions test/sequential/test-inspector-scriptparsed-context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,28 +51,28 @@ async function runTests() {
{ 'method': 'Debugger.enable' },
{ 'method': 'Runtime.runIfWaitingForDebugger' }
]);
await session.waitForBreakOnLine(4, '[eval]');
await session.waitForBreakOnLine(2, '[eval]');

await session.send({ 'method': 'Runtime.enable' });
await getContext(session);
await session.send({ 'method': 'Debugger.resume' });
const childContext = await getContext(session);
await session.waitForBreakOnLine(13, '[eval]');
await session.waitForBreakOnLine(11, '[eval]');

console.error('[test]', 'Script is unbound');
await session.send({ 'method': 'Debugger.resume' });
await session.waitForBreakOnLine(17, '[eval]');
await session.waitForBreakOnLine(15, '[eval]');

console.error('[test]', 'vm.runInContext associates script with context');
await session.send({ 'method': 'Debugger.resume' });
await checkScriptContext(session, childContext);
await session.waitForBreakOnLine(20, '[eval]');
await session.waitForBreakOnLine(18, '[eval]');

console.error('[test]', 'vm.runInNewContext associates script with context');
await session.send({ 'method': 'Debugger.resume' });
const thirdContext = await getContext(session);
await checkScriptContext(session, thirdContext);
await session.waitForBreakOnLine(23, '[eval]');
await session.waitForBreakOnLine(21, '[eval]');

console.error('[test]', 'vm.runInNewContext can contain debugger statements');
await session.send({ 'method': 'Debugger.resume' });
Expand Down