loader: fix --inspect-brk

[devsnek]

Fixes: #18948

Checklist

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

Affected core subsystem(s)

loader

[devsnek]

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

[targos]

Could you add a test?

[devsnek]

side question to anyone who can answer,

why can't we just do something like pauseOnStart() which only calls PauseOnNextJavascriptStatement then calling some function like:

void PauseOnStart(const FunctionCallbackInfo<v8::Value>& args) {
  Environment* env = Environment::GetCurrent(args);
  env->inspector_agent()->PauseOnNextJavascriptStatement("Break on start");
}

if (this.inspectBrk)
  process.binding('inspector').pauseOnStart();
this.module.instantiate();

[TimothyGu]

This was actually the original version of the PR submitted by @eugeneo in #17360, before #18194 changed the approach.

Changes LGTM. Testing is necessary however.

[devsnek]

@targos how does one test inspect-brk? i can't find any similar tests

edit: i found test-inspector-esm.js 👍

[TimothyGu]

@devsnek Because that would break on this.module.instantiate() rather than the user module.

[guybedford]

Looks good - I just copied the approach from #17360 exactly when doing this initially to get this feature moving. Edit: ah, I see I tried to move the wrapper to avoid an instance property which was the issue here!

I did experiment wrapping the break on the execution function itself to no success, this only seems to be working when wrapping instantiate specifically. If there's a way to get a pauseOnStart going there'd be no reason not to though surely.

[cjihrig]

LGTM with a test.

[eugeneo]

why can't we just do something like pauseOnStart() which only calls PauseOnNextJavascriptStatement then calling some function like:

We can't do it because next JS instruction will be before calling the function. I.e. instead of breaking inside the function (that is a user code) we will arrive to a function callsite, which is framework code. That will confuse the users a lot.

[devsnek]

i've been messing around with the tests but i just can't figure out this inspector stuff :(

basically the test just needs to be modified to import one other module to ensure nothing is called out of band. i tried to make these changes but i started getting assertion errors from the test inspector wrapper about urls

[guybedford]

@devsnek could you share where exactly the test is giving you issues? It should just be adding another case https://github.com/nodejs/node/blob/master/test/parallel/test-inspector-esm.js#L107, or modifying that existing case to have a dependency right?

[devsnek]

@guybedford i did that earlier but but it keeps giving me an assertion error here: https://github.com/nodejs/node/blob/master/test/common/inspector-helper.js#L248

and i still really don't understand all this stuff but i've been slowly making my way through the devtools docs

[guybedford]

@devsnek what are the expected and actual values in that case that are failing? If it's pathing issues that may be a test bug.

[devsnek]

@guybedford i added uh

const depUrl = UrlResolve(session.scriptURL(), 'message.mjs');
session.waitForBreakOnLine(0, depUrl);

which throws an assertion error for module.js === /home/snek/misc/nodejs/node/test/fixtures/es-modules/message.mjs

i also changed loop.mjs to import { message } from './message' and to break on line 6 instead of 5 because of the added line

[guybedford]

@devsnek does the module.js there mean that it's breaking in Node code? And it's definitely working in the Chome devtools correctly?

[devsnek]

@guybedford i have no idea which module.js it refers to, but yea these changes work perfectly

[devsnek]

@guybedford it seems the test i made is working now, i'm running a full test run on my laptop then i'll spin up a ci and if that passes hopefully land this 🙏

[devsnek]

@addaleax if i land this tonight or tomorrow we can still squeeze it into 9.7.0 right?

[devsnek]

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

[devsnek]

cc @nodejs/build
both windows builds failed around the same time in the middle of what they were doing with FATAL: command execution failed java.nio.channels.ClosedChannelException

[addaleax]

@devsnek Sorry, I wasn’t around when you posted that comment – if you think something is important enough to let a release wait for it, commenting in the release proposal might be best…

Then again, I’d be happy to also prepare another 9.x release next week.

[devsnek]

landed in abd0d79

[jasnell]

Should this be backported to 8.x?