The means for requesting variations to argument validation in CallData::ToNative implementations in Debugger object classes are getting baroque, and require information to be stored far away from where it's known.

In DebuggerFrame::ToNative, we have code:

  // These methods do not require liveness.
  bool checkLive = MyMethod != &CallData::liveGetter &&
                   MyMethod != &CallData::onStepGetter &&
                   MyMethod != &CallData::onStepSetter &&
                   MyMethod != &CallData::onPopGetter &&
                   MyMethod != &CallData::onPopSetter;

These are JSNative implementations of methods of Debugger.Frame that do not require the this value to refer to a live stack frame. Future patches add more exceptions and make finer-grained distinctions between what different methods need (on stack? suspended okay? Dead okay?).

It might be better to have different classes that do different degrees of validation, and have the JS_DEBUG_ macros let you actually specify the CallData class appropriate to this method. That way, each method's definition in the methods_ array has the complete description of how it should be called, and each implementation indicates its needs by the class it's a method of.

Since this validation is the last step done before calling the main callback, could we do something like

const JSPropertySpec DebuggerFrame::properties_[] = {
    JS_DEBUG_PSG("arguments", ValidateState<argumentsGetter, true>),

or assuming something like https://phabricator.services.mozilla.com/D54490 lands

const JSPropertySpec DebuggerFrame::properties_[] = {
    JS_DEBUG_PSG("arguments", ValidateState<argumentsGetter, MinState::OnStack>),

with ValidateState being a new templatized method on CallData?

I haven't actually tried this so hopefully not missing anything obvious.

Alternatively, I don't really have a problem with validating the state at the top of the individual methods themselves since it would be a function-call + early return that we could also easily make into a macro.

(In reply to Logan Smyth [:loganfsmyth] from comment #1)

Alternatively, I don't really have a problem with validating the state at the top of the individual methods themselves since it would be a function-call + early return that we could also easily make into a macro.

Well, that's exactly what we used to have. The macros were a pain, and Brian removed them as part of the dbg-impl-cleanup initiative in bug 1564167, introducing CallData as the alternative.

Personally, I think each function explicitly doing

if (!frame->ensureIsOnStack()) {
  return false;
}

or with my current patches even doing

if (frame->isOnStack()) {

} else if (frame->hasGenerator()) {

} else {
  return errorFrameRequiresOnStackOrSuspended();
}

to define its expectations about the state of the frame is totally fine. I'm really happy with CallData overall as a way to assert 100% common logic, but I think the fact that we have all of this special-casing is an indication that this logic doesn't belong inside ToNative or the CallData infrastructure to begin with. Having different classes for different levels of validation complicates the structure and moves the assertions for onStack and so on even farther from the code that actually depends on those assertions.

Just submitting to keep the conversation going. I think there's a lot to be said for not making things complex in this case in favor if a little repetition.

https://hg.mozilla.org/mozilla-central/rev/464a15bfedb8