module: improve detection try catch

[GeoffreyBooth]

This refactors the try/catch added in #52093 to happen in C++, so that the rethrown error preserves the same origin, source arrow and source map-translated stack trace. This gets us much closer to being able to unflag --experimental-detect-module and have all the tests pass. The C++ was written by @targos

There’s currently only one test failing on this branch, and I’d love some help getting it to pass. ./node test/parallel/test-debugger-exceptions.js is failing because in node inspect, for some reason the inspector sees the exception as being thrown by the new TryCatchRethrow rather than the rethrown exception. If anyone can figure out why this is and what we can do about it, I would be very grateful. cc @jkrems @nodejs/loaders @nodejs/inspector @nodejs/v8-inspector

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[GeoffreyBooth]

@nodejs/v8 When I run ./node test-debugger-exceptions.js on this branch, it errors on the last line of this section:

      // Next run: With `breakOnUncaught` it only pauses on the 2nd exception.
      await cli.command('breakOnUncaught');
      await cli.stepCommand('r'); // Also, the setting survives the restart.
      await cli.waitForInitialBreak();
      assert.deepStrictEqual(cli.breakInfo, { filename: script, line: 1 });
      await cli.stepCommand('c');
      assert.ok(cli.output.includes(`exception in ${script}:9`));

If I log that cli.output, I see this:

exception in node:internal/modules/run_main:171
 169     const { Module } = cjsLoader;
 170
>171     tryCatchRethrow(() => {
 172       // Module._load is the monkey-patchable CJS module loader.
 173       Module._load(main, null, true);

Which should be the fixture code, not Node internal code. I guess that somehow node inspect is adding its own try_catch_scope or similar that’s a level higher than the one created by tryCatchRethrow, and even when tryCatchRethrow catches an exception, that exception is also caught by the inspector? Can anyone point me to where this code lives, and any ideas on how to avoid it? I don’t want the inspector ever catching on tryCatchRethrow, only on user code exceptions that are rethrown by it.

[joyeecheung]

Calling back from C++ to JS would have a non-trivial performance impact. I think the proper way to deal with this without all the weird error handling should be handling ESM directly in the CJS loader when the compilation fails and the source looks like ESM (and if it's async, simply ignoring the exports of the entry point/making it some kind of dummy object should be fine, because no one really needs that, or if they do weird things like process.mainModule.exports, we can warn/error when the entry point is async ESM).

[joyeecheung]

I have a branch here built on top of #52413 that implements the idea above + require(esm) detection. This moves the reparsing into C++ and also handles ESM entrypoints in-place in the CJS loader (by just doing cascadedLoader.import() and making the CJS exports a proxy that emits a warning, technically I think we can just delete process.mainModule in that case, then it would probably be impossible to reach to the exports of the entry point without some hacky monkey patch that maybe no one really does), so it passes ./node test/parallel/test-debugger-exceptions.js when the flag is flipped because the error doesn't bubble all the way up.

[jkrems]

Sorry for not responding any earlier. Two questions I would have is:

1. How much does it matter to break this in practical terms. E.g. does the error trace at least contain the "real" location or is that completely lost? Also, is this only a problem with errors that happen to be thrown exactly in the entry point or also for errors thrown in its imports?
2. I assume this wouldn't just be a problem in node inspect but the same behavior would also be observable when debugging from any other CDP client like Chrome DevTools or VSCode? Afaik the node inspect tests are just the best proxy we have for regressions in more popular debug clients.

But maybe that's moot if the alternate implementation approach that @joyeecheung mentioned shakes out.

[GeoffreyBooth]

But maybe that's moot if the alternate implementation approach that @joyeecheung mentioned shakes out.

Yes, fortunately I think it's moot now thanks to Joyee's PR.