[jsdbg2] DebuggerFoo::CallData argument validation exceptions are getting out of hand
Categories
(Core :: JavaScript Engine, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox73 | --- | fixed |
People
(Reporter: jimb, Assigned: loganfsmyth)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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.
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
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.
Reporter | ||
Comment 2•5 years ago
|
||
(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.
Assignee | ||
Comment 3•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 5•5 years ago
|
||
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.
Comment 7•5 years ago
|
||
bugherder |
Description
•