It would be great if we could set stepping hooks for dead generator frames. The use case I have in mind is async stepping which roughly goes like this:

1. user pauses at an await/yield
2. user invokes step over, which sets an onPop & onStep handler in the background
3. the program pauses before the initial promise has resolved triggering an unexpected pause.

At this point, it would be great if the server could clear the initial frame's stepping handlers. Unfortunately this frame has temporarily died and is currently unaccessible.

Jim, this should be probably under your radar for 71
Honza

Should this be handed over?

One might expect this to be trivial, but it's not quite.

Looking at DebuggerFrame::setOnStepHandler, one can see that setting and clearing the onStep handler is not just a matter of setting or or clearing a slot on the DebuggerFrame object. Stepping requires instrumentation in the JIT code, which we don't want to include in the ~100% of scripts not being debugged. So changing the onStep handler requires coordinating with the JIT.

The script's stepper count needs to be incremented or decremented (or left alone, if one is just replacing one handler with another!), and if we're toggling stepping on, then we need to make sure the script's old JIT code is disposed of (this is the call to Debugger::ensureExecutionObservabilityOfScript), to be re-jitted with debug instrumentation.

And both Wasm and JS frames need to be dealt with.

@jimb I've looked into this a bit and wanted to run my thoughts by you.

As for the implementation, I assume what you were thinking is something along the lines of

  if (!isLive() && hasGenerator()) {
    RootedScript script(cx, generatorInfo()->generatorScript());
    AutoRealm ar(cx, script);
    if (!Debugger::ensureExecutionObservabilityOfScript(cx, script)) {
      return false;
    }
    if (!DebugScript::incrementStepperCount(cx, script)) {
      return false;
    }
  }

in setOnStepHandler, is that right?

The big next question for me is, are we allowing onStep mutation always now, or just when isLive() || hasGenerator()? I think it should be always mutable, because needing to keep track of multiple conditions is confusing and requiring the .live check in JS currently already caused us to file this bug. That raises the other issue, which is that we clear onStep when the frame finally dies fully. I think if the user sets onStep, it shouldn't change unless they change it. Given that, it seems like the easiest approach would be to add a new Boolean reserved slot to track whether the DebuggerFrame has incremented the stepper count. Right now we use the implicit presence of onStep as this flag, but if we split it off, then we can allow setting and removing onStep while the frame is suspended, and after it has died entirely.

Lastly, I see that onPop is also restricted to isLive() == true. I can't see any reason that it is necessary, so would you be up for me removing that as a second changeset within this bug? It seems strange to have .onStep = never throw but have .onPop = throw sometimes.

Yes, that implementation is something like what I had in mind, with analogous code for clearing the handler. Please look for opportunities to refactor, because this is not neat.

I can certainly see the argument that:

• onStep should always be mutable, regardless of liveness

• its value should be retained even when it's dead

• onPop should also be settable, regardless of liveness

I think the original rationale for the liveness checks was that, if your server is trying to set properties on a dead frame, that's probably a bug that you'd like to be alerted to. But maybe that was a bit too aggressive.

Although, as far as I know, we never did want to set things on dead frames, only on suspended frames.

The behavior of the live property is to some extent an accident. I think it would make more sense to treat a suspended generator frame as live until the generator is closed, and then have a separate predicate running (name open to debate) to distinguish between frames on the stack and frames that are suspended. For non-generator frames, the two would be equivalent.

So there's a middle ground here, which is to permit changes until the frame's final return, and then report attempts to use the frame after that point as errors. Wouldn't that serve our purposes?

So there's a middle ground here, which is to permit changes until the frame's final return, and then report attempts to use the frame after that point as errors. Wouldn't that serve our purposes?

My concern is that it is easy to think through "only live during sync", but as soon as a reference to the frame could be held elsewhere and changed later, the middleground may be more error-prone. I can 100% see the motivation for a warning if you set it after it dies, but throwing seems too aggressive to me since it would be very easy to forget that onStep is only mutable some of the time.

Would you be open to a warning instead?

The behavior of the live property is to some extent an accident. I think it would make more sense to treat a suspended generator frame as live until the generator is closed, and then have a separate predicate running (name open to debate) to distinguish between frames on the stack and frames that are suspended. For non-generator frames, the two would be equivalent.

I think that's reasonable if we preserve the warn/throw when live (isLive() || hasGenerator() in current terms).

We talked with this a bit more on Slack, and the agreement was, it's just weird to have assigning to a property throw. It's not the way people expect objects to behave. So onStep and onPop should always be settable, regardless of the liveness or suspended-ness of the frame.

Since we want to start allowing onStep to persist even after the frame's
internal state has been cleared, we cannot use the presence on onStep to
indicate this flag anymore.

We want to be able to set onStep when generators are suspended, and since that
already requires changes, it is easy to expand that and allow setting onStep
even after a frame is entirely dead. This simplifies the implementation,
and better matches user expectations around object properties not throwing
or vanishing due to actions outside their control.

Depends on D50430

Since we're now allowing onStep when not live, it makes sense to do the same
for onPop, and since it does not have any implementation details preventing
setting it while the frame is dead, we might as well.

Depends on D50431

This isn't checking 'liveness' anymore, and we don't need it to, so dropping
that part from the name of the function.

Depends on D50432

DebugScript::decrementStepperCount() cannot fail and
DebugState::decrementStepperCount() cannot either. Making this clearer allows
us to not seeming like we are silently ignoring the possibility of a 'false'
return value in 'maybeDecrementFrameScriptStepperCount()' since we don't
currently check the return value in there.

Depends on D50433

This should be all set for another review pass. Here's a new test run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6689a2cbf8603482fe4f978e50ca3ff688af3d82

https://hg.mozilla.org/mozilla-central/rev/b012a86d3d86
https://hg.mozilla.org/mozilla-central/rev/caf2d51b0a9d
https://hg.mozilla.org/mozilla-central/rev/912fb103b6e0
https://hg.mozilla.org/mozilla-central/rev/e4fcbd01d994
https://hg.mozilla.org/mozilla-central/rev/0199ecf6c90b