bmeck / proposal-debugger-operands

Adding an optional operand to the DebuggerStatement production of JS
13 stars 0 forks source link

Reentrancy #2

Open bmeck opened 6 years ago

bmeck commented 6 years ago

Need to define limitations of debugger evaluating code that reaches another debugger.

CC: @syg

syg commented 6 years ago

From my experience on SpiderMonkey's debugger implementation, a "debuggee no-execute" invariant while paused is very important for correctness. Having event loop turns while paused is, for example, really bad.

bmeck commented 6 years ago

I think ignoring "nested" debugger statements is fine, should it be required? Also it could be that:

debugger { test() { return true; } }

~ becomes

if ({ test() { return true; } }.test()) {
  debugger;
}

Which would avoid nesting, but doesn't address the concerns of userland tooling such as ReactDevTools.

syg commented 6 years ago

I don't think nesting debugger statements should be disallowed, but attempts to re-enter debuggee code while the debugger is already paused needs to be limited in some fashion.

The hard-line options are either to disallow completely, or to allow. My gut reaction is to disallow completely.

OTOH people have brought up legit use cases for nesting debuggers: trying to debug the code you've attached to the debugger statement itself, etc. If this proposal should like to support this nested debugging sessions, then the complexity trade-off is greater. Will the spec require pending exceptions to be in a stack according to the "debugger pause stack"? If a generator escapes while the debuggee is paused and is resumed later, does it also resume with its original "debugger pause stack"? The most consistent behavior would be to close over the debugger pause stack, as it were, but the implementation burden is very large.

ljharb commented 6 years ago

How can it be disallowed completely, given that i can debugger { eval('debugger') }?

syg commented 6 years ago

@ljharb What are you envisioning that does?

ljharb commented 6 years ago

@syg ideally the same thing that debugger { debugger; } does. I just mean that it can't be an early error because of the eval. A runtime error would be fine, but what does a runtime error do inside a debugger block? (which i think you mentioned in your earlier comment)

bmeck commented 6 years ago

Short engine check:

// run debugger REPL with the following while paused:
/*
test:{
function reenter() {
  debugger;
  window.staticDebugger = true;
  console.log('static reentrance ok');

  eval('debugger');
  window.evalDebugger = true;
  console.log('eval reentrance ok');
};
reenter();
}
*/
debugger;
\ SM V8 JSC
static silently ignored silently ignored silently ignored[1]
eval silently ignored silently ignored silently ignored[1]
  1. Console does not log but you can still assign side effects.

It seems like nested debugger already is ignored so maybe we can just state this behavior explicitly.

syg commented 6 years ago

@ljharb Oh yeah I agree it shouldn't be an early error. I meant like it could throw at runtime or something.

@bmeck Great data point. I'm totally okay with no-opping.