Purpose of double Debugger.setPauseOnExceptions

[Delapouite]

Hi

During the init phase: https://github.com/nodejs/node-inspect/blob/master/lib/internal/inspect_repl.js#L1041

  function initAfterStart() {
    const setupTasks = [
      Runtime.enable(),
      Profiler.enable(),
      Profiler.setSamplingInterval({ interval: 100 }),
      Debugger.enable(),
      Debugger.setPauseOnExceptions({ state: 'none' }),
      Debugger.setAsyncCallStackDepth({ maxDepth: 0 }),
      Debugger.setBlackboxPatterns({ patterns: [] }),
      Debugger.setPauseOnExceptions({ state: pauseOnExceptionState }),
      restoreBreakpoints(),
      Runtime.runIfWaitingForDebugger(),
    ];
    return Promise.all(setupTasks);
  }

What's the purpose of doing Debugger.setPauseOnExceptions twice?

Debugger.setPauseOnExceptions({ state: 'none' }),
Debugger.setPauseOnExceptions({ state: pauseOnExceptionState }),

[jkrems]

Hm, I don't think this is on purpose. I assume that the first got copied from somewhere and then I later added the 2nd line without looking properly. Nice find! I assume the first one could just be removed.

[alexkozy]

It looks like pauseOnExceptionState always 'none' in second invocation. In any case from protocol point of view there is no sense to call Debugger.setPauseOnExceptions twice. Implementation of this method just changes a flag inside V8.

[jkrems]

@ak239 Thanks for confirming!

Btw, we don't reset it before connecting/restarting. So it might not be 'none' when connecting after restart. So the hardcoded 'none' is definitely wrong and shouldn't be there.

[alexkozy]

I think we should support this case on backend side. When at least one protocol client requests pause on all exceptions - debugger should issue Debugger.paused event for each exception, if at least one client request pause on uncaught exception - issue event for uncaught exception.
I filled bug against V8: issue

[jkrems]

Fixed link, edited: https://bugs.chromium.org/p/chromium/issues/detail?id=1246881

[jkrems]

New location:

node/lib/internal/debugger/inspect_repl.js

Lines 1125 to 1128 in 4fbe9e5

	await Debugger.setPauseOnExceptions({ state: 'none' });
	await Debugger.setAsyncCallStackDepth({ maxDepth: 0 });
	await Debugger.setBlackboxPatterns({ patterns: [] });
	await Debugger.setPauseOnExceptions({ state: pauseOnExceptionState });

[jkrems]

The fix here may be to just remove the first call with the hard-coded 'none'. If the tests pass, that can likely just land.