nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.49k stars 29.02k forks source link

Pause on uncaught exceptions stops at wrong position with ES modules #38439

Open connor4312 opened 3 years ago

connor4312 commented 3 years ago

What steps will reproduce the bug?

Here's a repo with an easy reproduction:

  1. Open the repo in VS Code (or you can use another tool/any debugger)
  2. In the debug view (play button on the left hand side):
    • Check the [x] Uncaught Exceptions box in the breakpoints view.
    • Select "No Bug" and hit the play button. This runs no-bug/index.js.
  3. The debugger pauses at the location of the throw
  4. Now select "Repro Bug", and hit the play button. This runs bug/index.js: the same code in an ES module.
  5. The debugger pauses inside internal Node.js module code:

Under the hood this is just calling Debugger.setPauseOnExceptions({ state: 'uncaught' }), so it should be reproducable with any tool. In both cases this leads to a Debugger.pause, with the latter in the wrong location.

I'm guessing there's some inspector ✨magic that happens with commonjs that isn't happening for ES modules.

How often does it reproduce? Is there a required condition?

100%

What is the expected behavior?

In both cases, the debugger should pause on the throw

What do you see instead?

In ES modules, it pauses in module_job.js.

Additional information

This was originally reported in https://github.com/microsoft/vscode/issues/122246

Trott commented 3 years ago

I haven't looked into this issue, but I do know that inspect_repl.js has a isNativeUrl() function. I'm not sure but I think it is used to exclude internal Node.js module code. Maybe that function works only with CommonJS and tests need to be added for ESM.

@nodejs/diagnostics

Trott commented 3 years ago

I haven't looked into this issue, but I do know that inspect_repl.js has a isNativeUrl() function. I'm not sure but I think it is used to exclude internal Node.js module code. Maybe that function works only with CommonJS and tests need to be added for ESM.

@nodejs/diagnostics

Oh, wait, that would probably only affect the CLI debugger, not debugging in VS Code....

jkrems commented 3 years ago

It looks like the error stack is correct but we may have to compare how exactly we're calling the V8 APIs for modules. It looks like the inspector integration gets confused by the JS wrapping around module evaluation, maybe..?

MrJithil commented 3 years ago

I have tested the code and debugged the issue.

@connor4312 , From the CommonJS -, dependencies, instantiation, and code get evaluated all at once, without any breaks in between. But, unlike the CommonJS, in the ESModule, the code will be evaluated for syntax errors first. Here is your example, the throw is syntactically correct so that the evaluation will start the process. So, to get a breakpoint in the throw, you might need to use a try-catch like below.

try{
    throw new Error('oh no')
}
catch(e){
    console.log(e);
}

It will correctly pause on the throw statement because the code will evaluate correctly from ESM, but the error is thrown during the interpretation.

connor4312 commented 3 years ago

@MrJithil I'm not sure how what you said is relevant to the issue. Files are always checked for syntax errors prior to evaluation regardless of the module type. In neither example is a syntax error present.

MrJithil commented 3 years ago

@MrJithil I'm not sure how what you said is relevant to the issue. Files are always checked for syntax errors prior to evaluation regardless of the module type. In neither example is a syntax error present.

In my statement, I said since the code is syntactically correct, the module will start to evaluate it. And the exception caught there.