This bug is part of the effort to implement a C++ interface for the debugger API. See the parent bug 1271641 for more context.

Comment on attachment 8766401 [details] [diff] [review] Implement a DebuggerFrame class. Review of attachment 8766401 [details] [diff] [review]: ----------------------------------------------------------------- This looks mostly good but the variable renaming shouldn't happen. ::: js/src/vm/Debugger.cpp @@ +719,4 @@ > const ScriptFrameIter* maybeIter, MutableHandleValue vp) > { > + MOZ_ASSERT_IF(maybeIter, maybeIter->abstractFramePtr() == referent); > + MOZ_ASSERT(!referent.script()->selfHosted()); Why rename frame -> referent? @@ +732,3 @@ > > + RootedNativeObject frame(cx, DebuggerFrame::create(cx, proto, referent, maybeIter, > + debugger)); So that you can use `frame` as a name for this one? I think `frameobj` was just fine, and this renaming is making this patch hard to follow.

Comment on attachment 8766402 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.getCallee. Review of attachment 8766402 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +7036,5 @@ > +{ > + AbstractFramePtr referent = frame->referent(); > + Debugger* dbg = frame->owner(); > + > + if (!referent.isFunctionFrame()) { Nit: move this check right after defining `referent`, and get the owner Debugger after we know we aren't taking the early abort path. I think it reads better this way and is more similar to the usual spidermonkey early abort style. AbstractFramePtr referent = frame->referent(); if (!referent.isFunctionFrame()) { result.set(nullptr); return true; } Debugger* dbg = frame->owner(); ...

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #6) > Comment on attachment 8766401 [details] [diff] [review] > Implement a DebuggerFrame class. > > Review of attachment 8766401 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks mostly good but the variable renaming shouldn't happen. > > ::: js/src/vm/Debugger.cpp > @@ +719,4 @@ > > const ScriptFrameIter* maybeIter, MutableHandleValue vp) > > { > > + MOZ_ASSERT_IF(maybeIter, maybeIter->abstractFramePtr() == referent); > > + MOZ_ASSERT(!referent.script()->selfHosted()); > > Why rename frame -> referent? > > @@ +732,3 @@ > > > > + RootedNativeObject frame(cx, DebuggerFrame::create(cx, proto, referent, maybeIter, > > + debugger)); > > So that you can use `frame` as a name for this one? I think `frameobj` was > just fine, and this renaming is making this patch hard to follow. I've been using the same naming scheme in the Debugger.Object patches: - object for the Debugger.Object instance - referent for the JSObject it refers to And the Debugger.Environment patches: - environment for the Debugger.Environment instance - referent for the Env it refers to The reason I did it like this is that I felt that 'dbgobj', which we used for Debugger.Object, is misleading, since Debugger instances also have an associated object. The above naming scheme felt more consistent, and didn't lead to misleading name. To my knowledge, Jim never commented on it, so I assumed he was ok with the change. If it's ok with you, I'd like to continue on the same foot for the Debugger.Frame patches.

Comment on attachment 8766403 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.getIsConstructing. Review of attachment 8766403 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +7063,5 @@ > + Maybe<ScriptFrameIter>& result) > +{ > + AbstractFramePtr referent = frame->referent(); > + if (referent.isScriptFrameIterData()) { > + result.emplace(*(ScriptFrameIter::Data*)(referent.raw())); reinterpret_cast<ScriptFrameIter::Data*>(referent.raw()); @@ +7072,5 @@ > + ++iter; > + AbstractFramePtr data = iter.copyDataAsAbstractFramePtr(); > + if (!data) > + return false; > + frame->setPrivate(data.raw()); Since you've put the guts of THIS_FRAME_ITER into this method, make THIS_FRAME_ITER call this method so we don't have two copies floating around.

Comment on attachment 8766404 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.getEnvironment. Review of attachment 8766404 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +7113,5 @@ > + env = GetDebugScopeForFrame(cx, iter.abstractFramePtr(), iter.pc()); > + if (!env) > + return false; > + } > + Nit: trailing whitespace on this line.

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/95006e936e44 Implement a DebuggerFrame class;r=fitzgen

backedout for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=31055868&repo=mozilla-inbound

(In reply to Carsten Book [:Tomcat] from comment #12) > backedout for bustage like > https://treeherder.mozilla.org/logviewer.html#?job_id=31055868&repo=mozilla- > inbound Why did this not show up in my local build? I compile with --enable-optimize and --warnings-as-errors for good measure.

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c59c06b26e78 Backed out changeset 95006e936e44 for bustage

Try push for the patch to implement DebuggerFrame class to the patch to implement DebuggerFrame.isGenerator: https://treeherder.mozilla.org/#/jobs?repo=try&revision=649b84496ae9

Comment on attachment 8767136 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.isLive. Review of attachment 8767136 [details] [diff] [review]: ----------------------------------------------------------------- I remember asking you to define {Rooted,Handle,MutableHandle}Debugger{Frame,Object, etc} etc in js/src/gc/Rooting.h and use those typedefs to be consistent with the usual sm style, but I notice that still hasn't happened. Please do it.

Comment on attachment 8767139 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.type. Review of attachment 8767139 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +7414,5 @@ > + str = cx->names().call; > + break; > + case DebuggerFrameType::Module: > + str = cx->names().module; > + break; Does the compiler make a warning that we treat as an error and fail a build if you drop one of these cases? If not, let's add a `default: MOZ_CRASH("...")` branch or initialize str to nullptr and then assert that it is not nullptr after the switch.

Comment on attachment 8767140 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.implementation. Review of attachment 8767140 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +7246,5 @@ > + if (referent().isBaselineFrame()) > + return DebuggerFrameImplementation::Baseline; > + else if (referent().isRematerializedFrame()) > + return DebuggerFrameImplementation::Ion; > + return DebuggerFrameImplementation::Interpreter; Nit: Add MOZ_ASSERT(referent().isInterpreterFrame()); right before the return. @@ +7447,2 @@ > s = "interpreter"; > + break; Ditto regarding assertions about every case if removing a case doesn't cause the build to fail.

@fitzgen @jimb: I've spent today working on a proof of concept of a C++ interface for DebuggerFrame.onStep. This accessor is more complicated than the others, because it involves a callback object. The general idea is that for the C++ interface, the callback is a pointer to an interface with a strongly typed handle method. This allows consumers of the C++ interface to implement callbacks without being tied down to using a JS function, while still giving us the flexibility to implement JS function callbacks on top of it. I've attached the patch (including some comments/questions) to the bug for you to give feedback on. Before moving forward, I feel like it would be a good idea if we go over the patch together, and make sure we all agree on the approach to take. I will request a meeting to that end shortly after this.

Attached a slightly updated patch.

(In reply to Eddy Bruel [:ejpbruel] from comment #20) > Try push for the patch to implement DebuggerFrame class to the patch to > implement DebuggerFrame.isGenerator: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=649b84496ae9 Previous try push has test failures due to using the wrong prototype. Here is a new try push with that issue addressed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=064fa0f1a9fb

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/37f1b9d6f522 Implement a DebuggerFrame class;r=fitzgen https://hg.mozilla.org/integration/mozilla-inbound/rev/14212ea063c6 Implement a C++ interface for DebuggerFrame.getCallee;r=fitzgen https://hg.mozilla.org/integration/mozilla-inbound/rev/b93deff3205a Implement a C++ interface for DebuggerFrame.getIsConstructing;r=fitzgen https://hg.mozilla.org/integration/mozilla-inbound/rev/8698c60d265d Implement a C++ interface for DebuggerFrame.getEnvironment;r=fitzgen https://hg.mozilla.org/integration/mozilla-inbound/rev/359a15f3afea Implement a C++ interface for DebuggerFrame.isGenerator;r=fitzgen

Backed out for leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=31204437&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/a706323c0073b7cd72dc0c41a6601a91f15dea9b https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a0676a43f2bb8b0693591ab75cd9cad36edbf1 https://hg.mozilla.org/integration/mozilla-inbound/rev/7fbd728b1b01806d25fdea9c8dbb91fa7ef49b12 https://hg.mozilla.org/integration/mozilla-inbound/rev/2c7c2bbd7101fc24667f6db81858b07818388014 https://hg.mozilla.org/integration/mozilla-inbound/rev/358e634a8fd0d7cbd7f321ba80c3097986d004a2

Comment on attachment 8767701 [details] [diff] [review] Proof of concept for a C++ interface for DebuggerFrame.onStep. Review of attachment 8767701 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +1947,5 @@ > + // XXX: Could we makes DebuggerFrames a GCVector of DebuggerFrame > + // instances? (as opposed to NativeObjects). When I tried this out I ran > + // into obscure template error messages. I suspect we have a template > + // specialization for GCVector<NativeObject*> somwhere, but have not > + // been able to find it so far. Yeah that would definitely be nicer, although I don't know much about the specialization failures without seeing them. Hopefully they point at missing trait methods for GCPolicy<T> or BarrierMethods<T> that should be straightforward to add. @@ +7253,5 @@ > +OnStepHandler* > +DebuggerFrame::onStepHandler() const > +{ > + Value value = getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER); > + return !value.isUndefined() ? static_cast<OnStepHandler*>(value.toPrivate()) : nullptr; Super nitty nitty nit nit: personally, I think this would read a little more naturally if you removed the negation in the condition and swapped the consequent and alternative. @@ +7317,5 @@ > +static void > +DebuggerFrame_trace(JSTracer* tracer, JSObject* obj) > +{ > + OnStepHandler* handler = obj->as<DebuggerFrame>().onStepHandler(); > + if (handler) trailing whitespace @@ +7745,5 @@ > +class OnStepHandlerFunction final : public OnStepHandler { > + public: > + OnStepHandlerFunction(JSObject* object) : object_(object) > + { > + } Other code below relies on object being non-null (eg the trace method) so I think we should assert that we always construct valid instances: MOZ_ASSERT(object); @@ +7847,5 @@ > + // implements the OnStepHandler interface. > + // > + // Since the Debugger.Frame instance frees the onStep handler if necessary, > + // it must be allocated with cx->new_. Is this a reasonable restriction if > + // we want the allocated object to interface with Rust code? We could make it call some generic `destroy` method instead of actually using `js_delete` and destructors. In general, yes this looks how I imagined. I think we should also provide a way for Rust/embedders to unset the hook and take back ownership. ::: js/src/vm/Debugger.h @@ +393,5 @@ > + * return handleUncaughtException(ac, vp, callHook). > + */ > + JSTrapStatus parseResumptionValue(mozilla::Maybe<AutoCompartment>& ac, bool OK, const Value& rv, > + AbstractFramePtr frame, jsbytecode* pc, MutableHandleValue vp, > + bool callHook = true); Did this change at all or is this just code motion? @@ +1152,5 @@ > > +class Handler { > + public: > + virtual ~Handler() {} > + virtual void trace(JSTracer* trc) = 0; Need lots of docs for this stuff; it's going to be exposed to embedders and they should know how to use it without knowing spidermonkey internals. GC integration in particular is something that should be documented for folks who don't know the GC implementation. @@ +1157,5 @@ > +}; > + > +class OnStepHandler : public Handler { > + public: > + virtual JSTrapStatus handle(JSContext* cx, const ScriptFrameIter& iter, I think we should name this `handleStep` or `onStep` or something on the off chance someone makes a single handler handle multiple things so we won't get any method name collisions. @@ +1192,5 @@ > static MOZ_MUST_USE bool getThis(JSContext* cx, Handle<DebuggerFrame*> frame, > MutableHandleValue result); > > + Debugger* owner() const; > + AbstractFramePtr referent() const; This is just code motion?

Comment on attachment 8767701 [details] [diff] [review] Proof of concept for a C++ interface for DebuggerFrame.onStep. Review of attachment 8767701 [details] [diff] [review]: ----------------------------------------------------------------- To summarize our discussion this morning: Apparently, the simpler Rust apps embedding SpiderMonkey will usually be using a different malloc for Rust objects that SpiderMonkey uses for its own, so we need to support the case where the handler pointer is an inter-heap pointer. This means that the Handler base class must provide a trace method and a drop method, where the drop method does everything necessary for the Debugger to stop using the pointer, including freeing the Handler's storage if appropriate. That way, the Handler implementation can decide which malloc to use. You have the destructor as a virtual method of Handler; perhaps the destructor itself need not be part of the abstract base class, because Debugger will only ever call the drop method. It will be interesting to see how to turn a class with virtual methods into Rust types. Apparently bindgen doesn't do this now, but for an implementation Foo of Handler, I'd expect something like: struct Foo { vtable: &'static Foo_Vtable; ... data members of Foo ... } // This must match the ABI-specified vtable layout for Handler. I'm omitting some fields. struct Foo_Vtable { delete: fn(this: *mut Foo), trace: trace(this: *mut Foo, tracer: *mut JSTracer) } trait Handler { fn delete(&mut self); fn trace(&mut self, tracer: *mut JSTracer); } impl Handler for Foo { fn delete(&mut self) { self.vtable.delete(self); } fn trace(&mut self, tracer: *mut JSTracer) { self.vtable.trace(self, tracer); } } ::: js/src/vm/Debugger.cpp @@ +1944,5 @@ > > // Call onStep for frames that have the handler set. > for (size_t i = 0; i < frames.length(); i++) { > + // XXX: Could we makes DebuggerFrames a GCVector of DebuggerFrame > + // instances? (as opposed to NativeObjects). When I tried this out I ran Sure, that seems like the right thing. ::: js/src/vm/Debugger.h @@ +393,5 @@ > + * return handleUncaughtException(ac, vp, callHook). > + */ > + JSTrapStatus parseResumptionValue(mozilla::Maybe<AutoCompartment>& ac, bool OK, const Value& rv, > + AbstractFramePtr frame, jsbytecode* pc, MutableHandleValue vp, > + bool callHook = true); This method shouldn't be public. It would be better to have a common base class for those Handler implementations that invoke JS functions, and make that a friend of Debugger.

Yet another try push, with the memory leak that caused the previous backout hopefully resolved: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a52a6a7f6af2

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/49d59741e736 Implement a DebuggerFrame class;r=fitzgen https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4fb2446f1f Implement a C++ interface for DebuggerFrame.getCallee;r=fitzgen https://hg.mozilla.org/integration/mozilla-inbound/rev/276a5e166738 Implement a C++ interface for DebuggerFrame.getIsConstructing;r=fitzgen https://hg.mozilla.org/integration/mozilla-inbound/rev/763679990ba8 Implement a C++ interface for DebuggerFrame.getEnvironment;r=fitzgen https://hg.mozilla.org/integration/mozilla-inbound/rev/25c1b2a66590 Implement a C++ interface for DebuggerFrame.isGenerator;r=fitzgen

Try push from the patch to implement a C++ interface for DebuggerFrame.isLive to the patch to implement a C++ interface for DebuggerFrame.implementation: https://treeherder.mozilla.org/#/jobs?repo=try&revision=79340c5f8067

Comment on attachment 8768729 [details] [diff] [review] DebuggerFrameVector should be a GCVector<DebuggerFrame*>. Review of attachment 8768729 [details] [diff] [review]: ----------------------------------------------------------------- \o/ slightly more type safe!

Comment on attachment 8768750 [details] [diff] [review] Add GC typedefs for commonly used debugger types. Review of attachment 8768750 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: js/src/vm/Debugger.h @@ +258,5 @@ > +typedef JS::Handle<DebuggerObject*> HandleDebuggerObject; > + > +typedef JS::MutableHandle<DebuggerEnvironment*> MutableHandleDebuggerEnvironment; > +typedef JS::MutableHandle<DebuggerFrame*> MutableHandleDebuggerFrame; > +typedef JS::MutableHandle<DebuggerObject*> MutableHandleDebuggerObject; These belong in js/src/gc/Rooting.h, with forward decls as necessary.

https://hg.mozilla.org/mozilla-central/rev/49d59741e736 https://hg.mozilla.org/mozilla-central/rev/aa4fb2446f1f https://hg.mozilla.org/mozilla-central/rev/276a5e166738 https://hg.mozilla.org/mozilla-central/rev/763679990ba8 https://hg.mozilla.org/mozilla-central/rev/25c1b2a66590

I've rewritten the patch to implement a C++ interface for DebuggerFrame.onStep based on your and Jim's feedback. The patch is ready for review, but we probably need one or two iterations before we are both happy with it. Things I'd like to get feedback on in particular: 1. Did I miss anything in the interface? You mentioned in your feedback that we need a way for Rust to take back ownership of the handler. If we treat the handlers as a reference to a trait object, rather than the trait object itself, the delete_ method should allow us to do this, no? 2. Are you happy with the documentation? Is there anything you would like to add or phrase differently? 3. How do you feel about the cast in DebuggerFrame::onStepGetter? That cast is only safe if the handler was created by DebuggerFrame::onStepSetter. Any attempt to get a handler from JS that was created by C++ (and does not wrap a JSObject*) will cause a crash. That's arguably not something you would ever do, but you never know.

On the off chance that you're happy with the previous patch as is, I'm putting up the next patch for r? as well. Feel free to cancel this r? if you've r-'d the previous patch.

Try push for the patch to turn DebuggerFrameVector into a GCVector<DebuggerFrame> and the patch to add GC typedefs for commonly used Debugger types: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eb36acd94a1

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bda8deb45e63 Implement a C++ interface for DebuggerFrame.isLive;r=fitzgen https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa204468133 Implement a C++ interface for DebuggerFrame.getOffset;r=fitzgen https://hg.mozilla.org/integration/mozilla-inbound/rev/2879d03b9ce4 Implement a C++ interface for DebuggerFrame.getOlder;r=fitzgen https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f55b62daef Implement a C++ interface for DebuggerFrame.type;r=fitzgen https://hg.mozilla.org/integration/mozilla-inbound/rev/c374964ed8db Implement a C++ interface for DebuggerFrame.implementation;r=fitzgen

backed out for hazard failures like : https://treeherder.mozilla.org/logviewer.html#?job_id=31607995&repo=mozilla-inbound sorry eddy!

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/dc423f4ee148 Backed out changeset c374964ed8db https://hg.mozilla.org/integration/mozilla-inbound/rev/94272d8934e4 Backed out changeset b1f55b62daef https://hg.mozilla.org/integration/mozilla-inbound/rev/89fc1e2778d8 Backed out changeset 2879d03b9ce4 https://hg.mozilla.org/integration/mozilla-inbound/rev/52b6ce7d9819 Backed out changeset bfa204468133 https://hg.mozilla.org/integration/mozilla-inbound/rev/020f8723193f Backed out changeset bda8deb45e63 for hazard failures

(In reply to Carsten Book [:Tomcat] from comment #43) > backed out for hazard failures like : > https://treeherder.mozilla.org/logviewer.html#?job_id=31607995&repo=mozilla- > inbound > > sorry eddy! Rooting hazards eh? That's a new one. Can I somehow run this analysis locally?

SpiderMonkey has the convention that a method should not be static if it cannot GC. I didn't (and in fact I still don't) feel comfortable with this rule, because it forces you to think about whether your method can GC or not, potentially leading to subtle GC hazards if you get it wrong. Because of these worries, we ended up settling on the compromise that a method should not be static if it does not take a JSContext* as parameter. The assumption was that if a method does not take a JSContext* as parameter, then it cannot GC, so it is always safe to make it non-static. As it now turns out, that assumption does not hold. Since it does not take a JSContext* as parameter, at first glance it may look like DebuggerFrame::referent cannot GC. However, the GC hazard analysis, which we run on inbound, thinks that it does (Shu had a good explanation for why it must, but I don't remember it). Because of this, the previous 5 patches got backed out. To fix the issue, we have to change DebuggerFrame::referent into a static function, and then rebase the other patches on top of this change.

Rebased patch to implement a C++ interface for DebuggerFrame.isLive. Patch is essentially unchanged, so carrying over r+.

Rebased patch to implement a C++ interface for DebuggerFrame.getOffset. Patch is essentially unchanged, so carrying over r+.

Rebased patch to implement a C++ interface for DebuggerFrame.getOlder. Patch is essentially unchanged, so carrying over r+.

Rebased patch to implement a C++ interface for DebuggerFrame.getType. This patch has changed due to the changes in DebuggerFrame::referent, so re-requesting review.

Rebased patch to implement a C++ interface for DebuggerFrame.getType. This patch has changed due to the changes in Debugger::referent, so re-requesting review.

Rebased patch to turn DebuggerFrameVector into GCVector<DebuggerFrame*>. Patch is essentially unchanged, so carrying over r+.

Rebased patch to add GC typedefs for commonly used debugger types. Had to resolve a few merge conflicts, and change some additional types introduced in the earlier rebased patches, so re-requesting review.

At the moment, DebuggerGenericEval, which is used by DebuggerFrame_eval, DebuggerFrame_evalWithBindings and DebuggerObject::executeInGlobal, returns a JSValue representing a completion value. For the C++ interface, we prefer strong types over weak types wherever possible. A completion value consists of a status and value. By making DebuggerGenericEval return these two values directly we obtain an interface that is more strongly typed. Similarly, DebuggerObject::executeInGlobal, which belongs to the C++ interface of DebuggerObject, should also return these two values directly. DebuggerFrame_eval and DebuggerFrame_evalWithBindings don't yet have a C++ equivalent, so these will be addressed in an upcoming patch.

This patch moves EvaluateInEnv and DebuggerGenericEval to near the C++ interface functions for DebuggerFrame, so I will be able to define DebuggerFrame::eval and DebuggerFrame::evalInBindings (which depend on these functions) near the other C++ interface functions in the next patch.

I'm still waiting for your review of the onStep/onPop handler methods, so I went ahead and implemented the remaining C++ interface functions for Debugger.Frame. Note that these patches do not depend on the onStep/onPop handler patches, so feel free to hold off on those if you need more time to think.

(In reply to Eddy Bruel [:ejpbruel] from comment #46) > Created attachment 8770144 [details] [diff] [review] > DebuggerFrame::referent should be a static method. > > SpiderMonkey has the convention that a method should not be static if it > cannot GC. I didn't (and in fact I still don't) feel comfortable with this > rule, because it forces you to think about whether your method can GC or > not, potentially leading to subtle GC hazards if you get it wrong. > > Because of these worries, we ended up settling on the compromise that a > method should not be static if it does not take a JSContext* as parameter. > The assumption was that if a method does not take a JSContext* as parameter, > then it cannot GC, so it is always safe to make it non-static. > > As it now turns out, that assumption does not hold. Since it does not take a > JSContext* as parameter, at first glance it may look like > DebuggerFrame::referent cannot GC. However, the GC hazard analysis, which we > run on inbound, thinks that it does (Shu had a good explanation for why it > must, but I don't remember it). Because of this, the previous 5 patches got > backed out. > > To fix the issue, we have to change DebuggerFrame::referent into a static > function, and then rebase the other patches on top of this change. I'm left with a couple questions: (1) Which operation(s) inside referent() can GC? (2) Which pointers get invalidated? Is it pointers inside referent() calls or GC pointers across referent() calls? In general though I'm a bit preoccupied with getting ready for my ECOOP presentation and feel like I'm not able to give these patches the attention they deserve and so I'm bumping review back to jimb.

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #57) > (1) Which operation(s) inside referent() can GC? > (2) Which pointers get invalidated? Is it pointers inside referent() calls > or GC pointers across referent() calls? The stack iterator uses JSPrincipal subsumption to decide which frames to show. Subsumption is controlled by an embedding-provided callback, which the rooting analysis conservatively assumes could do anything at all, including cause a GC. (This is known as the "Scumbag JSPrincipal Subsumption Check" problem.) So, we don't know of any reasons that any actual pointers would really be changed by calling 'referent'. But to err on the side of caution and keep the static analysis happy, we'll just treat it as if it could GC.

(In reply to Jim Blandy :jimb from comment #58) > (In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #57) > > (1) Which operation(s) inside referent() can GC? > > (2) Which pointers get invalidated? Is it pointers inside referent() calls > > or GC pointers across referent() calls? > > The stack iterator uses JSPrincipal subsumption to decide which frames to > show. Subsumption is controlled by an embedding-provided callback, which the > rooting analysis conservatively assumes could do anything at all, including > cause a GC. (This is known as the "Scumbag JSPrincipal Subsumption Check" > problem.) > > So, we don't know of any reasons that any actual pointers would really be > changed by calling 'referent'. But to err on the side of caution and keep > the static analysis happy, we'll just treat it as if it could GC. Thank you for explaining it better than I could Jim.

Comment on attachment 8770144 [details] [diff] [review] DebuggerFrame::referent should be a static method. Review of attachment 8770144 [details] [diff] [review]: ----------------------------------------------------------------- Wow, that's too bad. Well, seems necessary.

Eddy, could you write a patch that changes /* static */ bool DebuggerFrame::getScriptFrameIter(JSContext* cx, Handle<DebuggerFrame*> frame, Maybe<ScriptFrameIter>& result) to: /* static */ Maybe<ScriptFrameIter> DebuggerFrame::getScriptFrameIter(JSContext* cx, Handle<DebuggerFrame*> frame) It's sort of ugly to have the returned bool carry exactly the same information as the discriminant of the Maybe. In other similar cases we've returned Maybe by value, so doing so here would be more consistent. And since structs returned by value are actually implemented by constructing the return value in a buffer provided by the caller, the actual machine code is going to be very similar. (This is something Nick tells me he decided not to nit, but it bugs me quite a bit.)

Comment on attachment 8770151 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.getType. Review of attachment 8770151 [details] [diff] [review]: ----------------------------------------------------------------- I think there are missing changes here: ::: js/src/vm/Debugger.cpp @@ +7509,5 @@ > return true; > } > > +/* static */ bool > +DebuggerFrame::thisGetter(JSContext* cx, unsigned argc, Value* vp) Why doesn't DebuggerFrame::thisGetter use DebuggerFrame::getThis?

Looks like I accidentally merged two patches during rebase, but only partially :S Here is the patch again, with the missing parts added.

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #9) > Since you've put the guts of THIS_FRAME_ITER into this method, make > THIS_FRAME_ITER call this method so we don't have two copies floating around. Eddy, I'd like to re-iterate this request. It's a distraction to have to wonder why there are two copies and carefully check them over for differences.

Comment on attachment 8770736 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.getType. Review of attachment 8770736 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, much better.

Comment on attachment 8770159 [details] [diff] [review] DebuggerGenericEval should return both trap status and value. Review of attachment 8770159 [details] [diff] [review]: ----------------------------------------------------------------- If I'm understanding this correctly, in the old arrangement, receiveCompletionValue carries out three steps: - call resultToCompletion - destruct the AutoCompartment - call newCompletionValue In the new arrangement, the same steps get carried out in the same order: - DebuggerGenericEval calls resultToCompletion directly. - The AutoCompartment is destructed by returning from DebuggerGenericEval. - The caller then calls newCompletion value, if needed. It worries me that DebuggerGenericEval is now returning a debuggee value directly, without calling wrapDebuggeeValue on it. That seems wrong. So I think you're going to need to reintroduce the explicit AutoCompartment destruction in DebuggerGenericEval, and change newCompletionValue to assert that its argument is already in the debugger's compartment, rather than wrapping it itself. ::: js/src/vm/Debugger.cpp @@ +7895,5 @@ > static bool > DebuggerGenericEval(JSContext* cx, const mozilla::Range<const char16_t> chars, > + HandleObject bindings, const EvalOptions& options, JSTrapStatus& status, > + MutableHandleValue value, Debugger* dbg, HandleObject scope, > + ScriptFrameIter* iter) Nit: put status and value on their own line, with bindings and options above, and dbg, scope and iter below.

Comment on attachment 8770161 [details] [diff] [review] Move EvaluateInEnv and DebuggerGenericEval. Review of attachment 8770161 [details] [diff] [review]: ----------------------------------------------------------------- This is the wrong patch.

Comment on attachment 8769657 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.onStep. Review of attachment 8769657 [details] [diff] [review]: ----------------------------------------------------------------- Still reviewing, but I wanted to publish what I had to this point: I think the design of Handler is pretty close to what we want. There are two remaining bits of dissonance to resolve. First, when we have C++ set a Debugger.Frame's onStep handler to some custom OnStepHandler subclass, and then JavaScript retrieves that handler, what should it see? There's no JavaScript representation for a random OnStepHandler. I suppose we could wrap it up as a JavaScript native function, but that doesn't seem worth it. So I think we need to return undefined or something. But the immediate point is, Debugger needs some way to know whether a Handler is JS or not. Perhaps there should be a "retrieveJSFunction" virtual method, with a default definition that returns nullptr, which OnStepHandlerFunction overrides to return the actual function? I don't remember what our original expectations were, but now it's clear that reflection objects (Debugger, Debugger.Frame, etc.) own *pointers* to Handlers, not Handlers themselves. Since Handler is an abstract base class, the reflection objects have no idea in general what size the concrete classes will be, so they can only point to them. If reflection objects only own pointers to Handlers, then the only finalization-related method that Handler needs to offer is a "drop" method. It doesn't need to mention a destructor, nor a delete method. That is, I think we need something like this: struct Handler { virtual void drop() = 0; virtual void trace(JSTracer* tracer) = 0; virtual JSFunction* maybeJSFunction() { return nullptr; } protected: ~Handler() { } } The comments for drop should definitely continue to mention that the Handler may be using a different allocator. That much is great. But it should talk about dropping pointers to Handlers, not about deleting and freeing their storage. Making the destructor protected, in Handler and in derived abstract classes like OnStepHandler, ensures that reflection objects that know only the base class can never destruct the instances, only drop them. We're going to need to be super-careful about dropping these things properly; self-assignment hazards, the whole nine yards. The disadvantage of storing these things in NativeObject slots is that we can't easily use a smart pointer type to gather all those rules into one place. ::: js/src/vm/Debugger.h @@ +34,5 @@ > JSTRAP_THROW, > JSTRAP_LIMIT > }; > > +class OnStepHandlerFunction; I would expect these to be in the same namespace as DebuggerObject, etc. Also, by placing "Function" in the name, you mean to indicate that this is a handler that calls a JS function. But there are "Functions" all over the place, in all languages. OnStepHandlerScripted might be a better name. @@ +1158,5 @@ > + * this data (see below). > + * > + * Finally, because handlers are implemented as interfaces, they need to be > + * allocated on the heap. Since we generally don't know what allocator was used > + * to allocate handelr, each handler also needs to provide a method to delete "handelr" @@ +1166,5 @@ > + * each handler (i.e. trace and delete). Handlers for specific events inherit > + * from this base class and define the method to be called when that event > + * happens. > + */ > +class Handler { I think abstract base classes should just be 'struct', since their whole purpose is to provide public methods.

Comment on attachment 8769657 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.onStep. Review of attachment 8769657 [details] [diff] [review]: ----------------------------------------------------------------- Okay, this is all I have. I'd like to review the revised patch as well. ::: js/src/vm/Debugger.cpp @@ -1950,5 @@ > - Debugger* dbg = Debugger::fromChildJSObject(frame); > - EnterDebuggeeNoExecute nx(cx, *dbg); > - > - Maybe<AutoCompartment> ac; > - ac.emplace(cx, dbg->object); This patch moves the EnterDebuggeeNoExecute and AutoCompartment into OnStepHandlerFunction::onStep, but I don't see why different subclasses of OnStepHandler should run in different dynamic contexts this way. This code should stay where it is. @@ +7230,5 @@ > + } > + > + if (prior && handler != prior) { > + prior->delete_(); > + } nit: let's delete / drop prior after we've replaced it with handler. It feels strange to have the slot referring to a deleted handler... @@ +7796,5 @@ > + // Calling the handler function may cause the frame's referent to be > + // changed into an ion frame. parseResumptionValue expects the referent > + // to be either an interpreter or baseline frame, so we need to copy it > + // before calling the handler function below. > + AbstractFramePtr referent = frame->referent(); AbstractFramePtr is incapable of referring to an Ion frame. If the referent really has turned into an Ion frame, then the `referent` value saved here is certainly invalid by the time we try to use it in parseResumptionValue. I have forgotten the details of how Debugger.Frame instances refer to the various sorts of frames. But there's nothing in this patch that should have any effect on the behavior of the JITs, so this seems like evidence of a bug elsewhere.

(In reply to Eddy Bruel [:ejpbruel] from comment #40) > On the off chance that you're happy with the previous patch (Just to be clear, I'm very happy with the previous patch! I rejected it because we need to go around on the design once more.)

(In reply to Jim Blandy :jimb from comment #69) > I have forgotten the details of how Debugger.Frame instances refer to the > various sorts of frames. But there's nothing in this patch that should have > any effect on the behavior of the JITs, so this seems like evidence of a bug > elsewhere. I checked up with Shu; there's no way the frame whose onStep handler we're calling could have turned into an Ion frame. It's marked as a debuggee frame, and those are never Ion. So there's some other bug here.

Comment on attachment 8769661 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.onPop. Review of attachment 8769661 [details] [diff] [review]: ----------------------------------------------------------------- Same general thoughts here as with onStep. I'd like to re-review after it's been changed. ::: js/src/vm/Debugger.cpp @@ -893,5 @@ > /* For each Debugger.Frame, fire its onPop handler, if any. */ > for (size_t i = 0; i < frames.length(); i++) { > - HandleDebuggerFrame frameobj = frames[i]; > - Debugger* dbg = Debugger::fromChildJSObject(frameobj); > - EnterDebuggeeNoExecute nx(cx, *dbg); Ideally this would stay here, in the code that invokes the onPopHandler. @@ -901,5 @@ > - { > - RootedValue handler(cx, frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER)); > - > - Maybe<AutoCompartment> ac; > - ac.emplace(cx, dbg->object); Similarly. @@ +7271,5 @@ > +OnPopHandler* > +DebuggerFrame::onPopHandler() const > +{ > + Value value = getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER); > + return value.isUndefined() ? nullptr : static_cast<OnPopHandler*>(value.toPrivate()); I believe private values can store nullptr, too. I think it would be a bit cleaner if we *always* stored a private value in the slots that refer to Handlers, and then checked for nullptr after calling toPrivate. This would become, simply: return static_cast<OnPopHandler*>(value.toPrivate()); @@ +7280,5 @@ > +{ > + OnPopHandler* prior = onPopHandler(); > + if (prior && handler != prior) { > + prior->delete_(); > + } Same slight nit as before: it would make me a little more comfortable if we deleted the prior handler *after* we install the new one. @@ +7347,5 @@ > + onStepHandler->delete_(); > + > + OnPopHandler* onPopHandler = obj->as<DebuggerFrame>().onPopHandler(); > + if (onPopHandler) > + onPopHandler->delete_(); Nit: why not have: DebuggerFrame &frame = obj->as<DebuggerFrame>(); and then use that everywhere? We're going to be adding more and more cases here. @@ +7355,5 @@ > DebuggerFrame_trace(JSTracer* trc, JSObject* obj) > { > + OnStepHandler* onStepHandler = obj->as<DebuggerFrame>().onStepHandler(); > + if (onStepHandler) > + onStepHandler->trace(trc); Similarly. @@ +7872,5 @@ > + js_delete(this); > + } > + > + virtual void trace(JSTracer* tracer) override { > + TraceEdge(tracer, &object_, "OnPopHandlerFunction.object"); Here and in the onStep patch, I think it might make more sense to label this edge "Debugger.Frame onPop handler". Since Debugger.Frame's trace function calls the OnPopHandler's trace function directly, there's no record of that edge; so tools will see the name given here attributed to Debugger.Frame objects. @@ +7892,5 @@ > + // Calling the handler function may cause the frame's referent to be > + // changed into an ion frame. parseResumptionValue expects the referent > + // to be either an interpreter or baseline frame, so we need to copy it > + // before calling the handler function below. > + AbstractFramePtr referent = frame->referent(); Here, too, this suggests a bug elsewhere. ::: js/src/vm/Debugger.h @@ +35,5 @@ > JSTRAP_LIMIT > }; > > class OnStepHandlerFunction; > +class OnPopHandlerFunction; This should be in the same namespace as DebuggerObject et al. @@ +1216,5 @@ > > +class OnPopHandler : public Handler { > + public: > + virtual JSTrapStatus onPop(JSContext* cx, HandleDebuggerFrame frame, JSTrapStatus status, > + HandleValue value, jsbytecode* pc, MutableHandleValue result) = 0; Why does an OnPopHandler need a `pc` pointer?

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/904a6eb36f1b DebuggerFrame::referent should be a static method. r=jimb

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ae502e2271f2 Implement a C++ interface for DebuggerFrame.isLive. r=fitzgen

https://hg.mozilla.org/mozilla-central/rev/904a6eb36f1b https://hg.mozilla.org/mozilla-central/rev/ae502e2271f2

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/496ab46d92e3 Implement a C++ interface for DebuggerFrame.getOffset. r=fitzgen

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa0163913570 Implement a C++ interface for DebuggerFrame.getOlder. r=fitzgen

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb2a15891d3 Implement a C++ interface for DebuggerFrame.getType. r=jimb

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/52057d746d72 Implement a C++ interface for DebuggerFrame.getImplementation. r=jimb

https://hg.mozilla.org/mozilla-central/rev/496ab46d92e3 https://hg.mozilla.org/mozilla-central/rev/fa0163913570 https://hg.mozilla.org/mozilla-central/rev/3cb2a15891d3 https://hg.mozilla.org/mozilla-central/rev/52057d746d72

Try push for the patches to change DebuggerFrameVector into GCVector<DebuggerFrame*> and add GC typedefs for commonly used debugger types: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be5b7095d395

Apologies for the delay in this bug. I suffered a catastrophic crash of my Linux VM last week, and lost several days of work in the process. As a result, I find myself back on OS X again, where even basic stuff like running a jit-test with lldb doesn't work. Good times.

New patch with comments by Jim addressed.

(In reply to Eddy Bruel [:ejpbruel] from comment #82) > I suffered a catastrophic crash of my > Linux VM last week, and lost several days of work in the process. Wow, that sucks!!!

Comment on attachment 8778839 [details] [diff] [review] DebuggerGenericEval should return both trap status and value. Review of attachment 8778839 [details] [diff] [review]: ----------------------------------------------------------------- The change to newCompletionValue is right, but this doesn't make the change to DebuggerGenericEval that I asked for. The patch as written has DebuggerGenericEval return a (JSTrapStatus, unwrapped debuggee value) pair, leaving its callers to call wrapDebuggeeValue on the value. Instead, DebuggerGenericEval should explicitly destruct `ac` by calling `ac.reset()` to leave the debuggee compartment, and then call wrapDebuggeeValue itself, returning a (JSTrapStatus, properly wrapped debuggee value) pair ready for newCompletionValue to consume.

New patch with review comments by jimb addressed that were unaddressed by the previous patch with review comments by jimb addressed (I think).

In the patch to implement a C++ interface for DebuggerFrame.onStep, I ran into a problem where the referent of a Debugger.Frame would change after calling its onStep handler. At the time, I thought this was due to the referent being changed to an Ion frame as a result of the call. However, after reviewing the patch, Jim pointed out that this shouldn't be happening. This made me worried that we had a serious bug somewhere, so I wanted to know what exactly caused the referent to change. After some debugging, I came up with the following stack trace, which illustrates the problem: https://pastebin.mozilla.org/8890749 As you can see, in this particular test, the onStep handler removes the global to which the frame belongs as debuggee. Doing that will cause the Debugger.Frame to be invalidated, and *that* causes it referent to be changed. Now that we know this, it should be enough to simply create a copy of the referent before calling the onStep handler, like I did in the r-'d patch.

Comment on attachment 8779241 [details] [diff] [review] DebuggerGenericEval should return both trap status and value. Review of attachment 8779241 [details] [diff] [review]: ----------------------------------------------------------------- This has it all. Thanks!

Comment on attachment 8779244 [details] [diff] [review] Move EvaluateInEnv and DebuggerGenericEval. Review of attachment 8779244 [details] [diff] [review]: ----------------------------------------------------------------- This patch is just pure code motion, right? Thanks for splitting it out.

Moving the stack trace from pastebin into an attachment.

(In reply to Eddy Bruel [:ejpbruel] from comment #88) > This made me worried that we had a serious bug somewhere, so I wanted to > know what exactly caused the referent to change. After some debugging, I > came up with the following stack trace, which illustrates the problem: > https://pastebin.mozilla.org/8890749 > > As you can see, in this particular test, the onStep handler removes the > global to which the frame belongs as debuggee. Doing that will cause the > Debugger.Frame to be invalidated, and *that* causes it referent to be > changed. Which test case is this, again? (Sorry, I did look through the bug, but I didn't see you mentioning the test case that motivates the change.) I think I see the bug in the patch. The code now in Mozilla Central initializes `iter` at the top of onSingleStep, finds all the Debugger.Frames that refer to that frame, and then iterates over them, using the original `iter` value to produce the arguments parseResumptionValue needs. With the patch, the initial steps are the same: initialize `iter`, find all the Debugger.Frames that refer to that frame, and then iterate over them. However, the patch tries to retrieve the referent from each Debugger.Frame, instead of using the `iter` captured at the top of the function. A test case that causes Debugger.Frames to be invalidated in their onStep handlers will certainly leave the Debugger.Frame unable to provide the information parseResumptionValue needs. However, I think the patch is still incorrect: if there are two Debugger.Frames referring to a frame, and the first D.F's onStep handler removes the frame's global from second D.F's debuggee set, then the second iteration through the loop will invoke OnStepHandlerFunction::onStep, passing nothing but an invalidated Debugger.Frame. That function will have no way to reach the actual stack frame at all. I think there's a bigger problem here with the OnStepHandler API. Clearly, only an OnStepHandler subclass that invokes a JavaScript handler should have to call parseResumptionValue. But parseResumptionValue requires types like AbstractFramePtr that shouldn't appear in a public interface like OnStepHandler::onStep.

It looks to me like bug 1232685 has entangled Debugger::parseResumptionValue with a bunch of stuff it shouldn't be concerned with. All the OnStepHandler really wants to do is take a JavaScript value representing a resumption value and convert it to <JSTrapStatus, Value> form, and return that. And that's what parseResumptionValue is supposed to do. But now it's taking care of a bunch of ES6 concerns with correcting forced return values for derived class constructors and I-don't-know-what, that require an AbstractFramePtr. All of that extra garbage *must* be handled outside OnStepHandler, not within implementations of OnStepHandler::onStep. Whatever it is, it certainly looks like stuff which needs to be handled in exactly the same way no matter what sort of handler is being invoked. As it stands, it seems like what OnStepHandlerFunction really wants to call is something simpler, like ParseResumptionValueAsObject. All the other handling in parseResumptionValue should take place in OnStepHandler::onStep's caller. So we need to refactor parseResumptionValue before we can get this right.

(In reply to Jim Blandy :jimb from comment #93) > It looks to me like bug 1232685 has entangled Debugger::parseResumptionValue > with a bunch of stuff it shouldn't be concerned with. All the OnStepHandler > really wants to do is take a JavaScript value representing a resumption > value and convert it to <JSTrapStatus, Value> form, and return that. And > that's what parseResumptionValue is supposed to do. But now it's taking care > of a bunch of ES6 concerns with correcting forced return values for derived > class constructors and I-don't-know-what, that require an AbstractFramePtr. > > All of that extra garbage *must* be handled outside OnStepHandler, not > within implementations of OnStepHandler::onStep. Whatever it is, it > certainly looks like stuff which needs to be handled in exactly the same way > no matter what sort of handler is being invoked. > > As it stands, it seems like what OnStepHandlerFunction really wants to call > is something simpler, like ParseResumptionValueAsObject. All the other > handling in parseResumptionValue should take place in > OnStepHandler::onStep's caller. > > So we need to refactor parseResumptionValue before we can get this right. I was working on a new version of the patch to implement a C++ interface for DebuggerFrame.onStep with your review comments addressed, but given your comments above, we should probably take a step back and briefly discuss over Vidyo how we want this code to behave before I continue.

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3b6d54ce5fa4 DebuggerFrameVector should be a GCVector<DebuggerFrame*>. r=fitzgen

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1d9554d44cbc Add GC typedefs for commonly used debugger types. r=fitzgen

Try push for the patch to make DebuggerGenericEval return both trap status and value, to the patch to implement a C++ interface for DebuggerFrame.eval(WithBindings): https://treeherder.mozilla.org/#/jobs?repo=try&revision=66f4d6d6d8c9

https://hg.mozilla.org/mozilla-central/rev/3b6d54ce5fa4 https://hg.mozilla.org/mozilla-central/rev/1d9554d44cbc

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c3b20f9afa DebuggerGenericEval should return both trap status and value. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/0b5962eff1ad Move EvaluateInEnv and DebuggerGenericEval. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/837ca629be91 Implement a C++ interface for DebuggerFrame.eval(WithBindings). r=jimb

In bug 1294013, you said you wanted to hold off on reviewing the patch to factor our GetThisValueForCheck, because you expect Debugger::onSingleStep to use the same functions for checking and applying resumption values that other hook invocation spots do. That's not exactly true: processHandlerResult gets the this value for the check and then passes that to processHandlerResultHelper. We don't want to use processHandlerResult, because we want to do parse the result ourselves in the on step handler. We then do the remaining steps such as unwrapping the return value and checking the resumption value ourselves. We have separate functions for each of these steps, but we don't have a separate function for getting the this value (remember, we can't use processHandlerValue). I've attached a WIP patch to implement a C++ interface for DebuggerFrame.onStep. It's not quite ready for review yet, but hopefully it will help to make clear why we need the patch in bug 1288084. Let me know if you agree, so I can re-request review.

https://hg.mozilla.org/mozilla-central/rev/c3c3b20f9afa https://hg.mozilla.org/mozilla-central/rev/0b5962eff1ad https://hg.mozilla.org/mozilla-central/rev/837ca629be91

Comment on attachment 8781150 [details] [diff] [review] WIP patch to implement a C++ interface for DebuggerFrame.onStep. Review of attachment 8781150 [details] [diff] [review]: ----------------------------------------------------------------- The goal of the refactoring is to separate parsing of resumption values into (JSTrapStatus, wrapped Debuggee value) pairs, which is what Handler subclass methods like onStep should return, from the further processing. But there should still be a single function that handles all that further processing; the return value check-and-fix should still be internal to that function. Remember that most handlers can return resumption values, so every one of those needs the same uncaught exception handling and return value checking/fixing. If you change Debugger::onSingleStep in the way proposed here, then all those handler invocation sites will need the same GetThisValueForCheck/unwrapDebuggeeValue/CheckResumptionValue/handleUncaughtException process you're writing out here. That's a step backwards. Instead, processHandlerResult (originally parseResumptionValue) needs to continue to be the single call that a handler invocation site must make to get all that machinery. The only difference is that it should take a parsed resumption value (the pair), instead of the unparsed rv value it takes now. Basically, you want another overload of processHandlerResult that expects the (JSTrapStatus, debuggee value) pair: JSTrapStatus Debugger::parseResumptionValue(Maybe<AutoCompartment>& ac, bool ok, JSTrapStatus resumptionStatus, HandleValue resumptionV, AbstractFramePtr frame, jsbytecode* pc, MutableHandleValue vp, bool callHook) and then the extant overload just calls ParseResumptionValue, and then calls the new overload.

(In reply to Jim Blandy :jimb from comment #102) > Comment on attachment 8781150 [details] [diff] [review] > WIP patch to implement a C++ interface for DebuggerFrame.onStep. > > Review of attachment 8781150 [details] [diff] [review]: > ----------------------------------------------------------------- > > The goal of the refactoring is to separate parsing of resumption values into > (JSTrapStatus, wrapped Debuggee value) pairs, which is what Handler subclass > methods like onStep should return, from the further processing. But there > should still be a single function that handles all that further processing; > the return value check-and-fix should still be internal to that function. > > Remember that most handlers can return resumption values, so every one of > those needs the same uncaught exception handling and return value > checking/fixing. If you change Debugger::onSingleStep in the way proposed > here, then all those handler invocation sites will need the same > GetThisValueForCheck/unwrapDebuggeeValue/CheckResumptionValue/ > handleUncaughtException process you're writing out here. That's a step > backwards. > > Instead, processHandlerResult (originally parseResumptionValue) needs to > continue to be the single call that a handler invocation site must make to > get all that machinery. The only difference is that it should take a parsed > resumption value (the pair), instead of the unparsed rv value it takes now. > > Basically, you want another overload of processHandlerResult that expects > the (JSTrapStatus, debuggee value) pair: > > JSTrapStatus > Debugger::parseResumptionValue(Maybe<AutoCompartment>& ac, bool ok, > JSTrapStatus resumptionStatus, HandleValue > resumptionV, > AbstractFramePtr frame, jsbytecode* pc, > MutableHandleValue vp, > bool callHook) > > and then the extant overload just calls ParseResumptionValue, and then calls > the new overload. Ok, that seems like a reasonable approach to me. New patch coming up tomorrow.

I now have two helper functions for processing a handler result. The first one, processHandlerResultHelper, takes an unparsed handler result (that is, a success value and a resumption value). The second one, processParsedHandlerResultHelper, takes a parsed handler result (that is, a success value, a trap status, and a value to be returned). The first helper function is used by the existing JS style hooks. It simply parses the handler result, and then forwards to the second helper function to do the rest of the processing. The new C++ style hooks can call the second helper function directly, since they always return parsed resumption values. Neither of these helper functions can be called directly, because they also need a frame pointer and an optional this value to check the resumption value against. There are two ways to obtain the this value, so each helper function has two non-helper overloads: one that takes a this value directly, and one that takes a program counter that can be used to obtain the this value indirectly via GetThisValueForDebuggerMaybeOptimizedOut. Since the code to obtain the this value via GetThisValueForDebuggerMaybeOptimizedOut is still duplicated between these two overloads, it might still be nice to factor that code out into GetThisValueForCheck, like I did before. On the other hand, these are now the only two functions between where this code is duplicated (as opposed to for every C++ style hook, as would have been the case in my original approach), so we could also leave it out. Jim, are you happy with this approach? Or are there still things that you would like to be done differently?

I just noticed the comment in Debugger.h that says: /* * When we run the onEnterFrame hook, the |this| slot hasn't been fully * initialized, because the initialzation happens in the function's * prologue. To combat this, we pass the this for the primitive return * check directly. When bug 1249193 is fixed, this overload should be * removed. */ JSTrapStatus parseResumptionValue(mozilla::Maybe<AutoCompartment>& ac, bool OK, const Value& rv, const Value& thisVForCheck, AbstractFramePtr frame, MutableHandleValue vp, bool callHook = true); Bug 1249193 is marked RESOLVED FIXED, but Debugger::fireEnterFrame is still using this overload. What's the change needed to Debugger::fireEnterFrame to let us delete this overload?

Asking because I think it will halve the number of overloads Eddy needs.

Comment on attachment 8781974 [details] [diff] [review] WIP patch to implement a C++ interface for DebuggerFrame.onStep (v2). Review of attachment 8781974 [details] [diff] [review]: ----------------------------------------------------------------- This looks more like it. Note my needinfo? above --- we may be able to drop a bunch of variations here. ::: js/src/vm/Debugger.cpp @@ -2004,5 @@ > // Call onStep for frames that have the handler set. > for (size_t i = 0; i < frames.length(); i++) { > - HandleDebuggerFrame frame = frames[i]; > - if (frame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined()) > - continue; nit: I don't think changing this no-handler check into an `if` around the handler call is a clarification. (I'm guessing that's the intent?) In the old version, it's easier to see that the loop iteration has no effect if the frame has no handler. You can read the rest of the loop body under the assumption that we're going to invoke a handler.

(In reply to Jim Blandy :jimb from comment #105) > I just noticed the comment in Debugger.h that says: > > /* > * When we run the onEnterFrame hook, the |this| slot hasn't been fully > * initialized, because the initialzation happens in the function's > * prologue. To combat this, we pass the this for the primitive return > * check directly. When bug 1249193 is fixed, this overload should be > * removed. > */ > JSTrapStatus parseResumptionValue(mozilla::Maybe<AutoCompartment>& ac, > bool OK, const Value& rv, > const Value& thisVForCheck, > AbstractFramePtr frame, > MutableHandleValue vp, bool callHook = > true); > > Bug 1249193 is marked RESOLVED FIXED, but Debugger::fireEnterFrame is still > using this overload. What's the change needed to Debugger::fireEnterFrame to > let us delete this overload? It can be deleted now. We forgot about it after bug 1249193 landed. The original issue was that to accommodate classes, the 'this' binding is now initialized in bytecode and treated like a var binding, but this meant the 'this' value was uninitialized on the frame in onEnterFrame. Bug 1249193 added special logic to initialize the 'this' binding should a frame be inspected before any code has executed. I removed that overload locally, passing in |frame.script()->code()|, i.e., the first bytecode, to parseResumptionValue, and all debug tests pass.

Okay, thanks very much. So it's now possible to derived the `this` value from a (frame, pc) pair just as all the other parseResumptionValue sites do. Eddy, if we could get this small patch landed, it would simplify your patch nicely!

This patch removes one of the two overloads of processHandlerResultHelper, as suggested by Jim, and merged the remaining helper function back into processHandlerResult.

This patch adds a new version of processHandlerResult, that can be used by those callers that already have a parsed version of the handler result, and which I've named processParsedHandlerResult. Note that I've introduced a helper function, processParsedHandlerResultHelper, that is called by both processParsedHandlerResult and processHandlerResult. Both functions first obtain the this value to be used to to check the resumption value. In addition, the latter then parses the unparsed handler result. Finally, both functions pass the parsed handler and the this value for the check to the helper function.

This seems about ready for review. Essentially the same patch on which you gave feedback on, but with comments added, and some of the work split off into the patches I r?'d you on earlier.

Review ping.

Thanks for the ping!

Comment on attachment 8783513 [details] [diff] [review] Remove processHandlerResultHelper. Review of attachment 8783513 [details] [diff] [review]: ----------------------------------------------------------------- The changes to processHandlerResult and processHandlerResultHelper look great. But why are there changes to slowPathOnEnterFrame and fireEnterFrame here? Those are extraneous to the patch. Constructing a ScriptFrameIter is not terribly cheap; it may involve reading Ion frame snapshots and various other bits of hair.

Comment on attachment 8783514 [details] [diff] [review] Factor out processParsedHandlerResult(Helper). Review of attachment 8783514 [details] [diff] [review]: ----------------------------------------------------------------- I feel like this is more complicated than it needs to be, but it seems like it should work.

Comment on attachment 8783535 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.onStep. Review of attachment 8783535 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +2030,5 @@ > #endif > > // Call onStep for frames that have the handler set. > for (size_t i = 0; i < frames.length(); i++) { > + RootedDebuggerFrame frame(cx, frames[i]); Is there a reason this changes the HandleDebuggerFrame to a RootedDebuggerFrame? @@ -2031,5 @@ > // Call onStep for frames that have the handler set. > for (size_t i = 0; i < frames.length(); i++) { > - HandleDebuggerFrame frame = frames[i]; > - if (frame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined()) > - continue; The nit I mentioned in comment 107 is still here; is that on purpose?

(In reply to Jim Blandy :jimb from comment #115) > Comment on attachment 8783513 [details] [diff] [review] > Remove processHandlerResultHelper. > > Review of attachment 8783513 [details] [diff] [review]: > ----------------------------------------------------------------- > > The changes to processHandlerResult and processHandlerResultHelper look > great. > > But why are there changes to slowPathOnEnterFrame and fireEnterFrame here? > Those are extraneous to the patch. Constructing a ScriptFrameIter is not > terribly cheap; it may involve reading Ion frame snapshots and various other > bits of hair. We removed the version of processHandlerResult that takes an AbstractFramePtr and a JSValue, and replaced it with the version that takes a AbstractFramePtr and a jsbytecode* (from which we can then obtain the JSValue). My assumption was that the only way to obtain an AbstractFramePtr *and* a jsbytecode* is with a ScriptFrameIter. Is this not the case? If so, please let me know what the correct approach should be. On the other hand, if this approach is correct, we no longer need to pass an AbstractFramePtr to fireEnterFrame, because we already obtain it from the ScriptFrameIter. Note that this is consistent with what the other fire* methods do.

Okay, I see: the patch just makes fireEnterFrame look just like all the other hook invocations, that create a ScriptFrameIter and use that to find the Debugger.Frame and pc to pass to processHandlerResult. Okay, now it all makes more sense - thanks for the explanation.

Try push for the following patches: - Remove processResumptionValueHelper. - Factor our processParsedResumptionValue(Helper). - Implement a C++ interface for DebuggerFrame.onStep. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e49cccb98001

Previous try patch had the following issues: - Compile errors in browser builds due to missing 'explicit' keyword. New try push for the following patches, with above issues addressed: - Remove processResumptionValueHelper. - Factor our processParsedResumptionValue(Helper). - Implement a C++ interface for DebuggerFrame.onStep. https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f3e53a3f53a

Comment on attachment 8787598 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.onPop Review of attachment 8787598 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +905,2 @@ > { > + OnPopHandler* handler = frameobj->onPopHandler(); There's something that makes me a little nervous here - and it applies to the onStep handler as well. This is a non-owning pointer to the handler; if anything stores a new handler on the frame, this onPopHandler will be destructed. This didn't happen with the old code, because we stored the handler in our very own RootedValue. I can't see anything between here and the use of `handler` that could change it, though, so I guess it's okay. It's just a little precarious. It'd make me happier if we actually just dropped the `handler` variable entirely, and called it like this: bool success = frameobj->onPopHandler()->onPop(...) @@ +7794,5 @@ > + if (onStepHandler) > + onStepHandler->trace(trc); > + OnPopHandler* onPopHandler = obj->as<DebuggerFrame>().onPopHandler(); > + if (onPopHandler) > + onPopHandler->trace(trc); FWIW, the cool kids these days just say: if (OnPopHandler* onPopHandler = obj->as<DebuggerFrame>().onPopHandler()) onPopHandler->trace(trc); If you like that style, you could use it for onStepHandler, and in _finalize as well. @@ +8251,5 @@ > + > + OnPopHandler* handler = frame->onPopHandler(); > + RootedValue value(cx, handler ? ObjectValue(*handler->object()) : UndefinedValue()); > + MOZ_ASSERT(IsValidHook(value)); > + args.rval().set(value); Does this mean that we get JS `null` if there's a non-Scripted handler installed, but JS `undefined` if there's no handler at all? Isn't this going to fail the IsValidHook assertion? It seems to me we should either return some JS value that really represents the (non-Scripted) handler, so you could save it, replace it with a JS function, and then put the original back and get back the same handler as before --- which would be a lot of work for something I really doubt anyone's going to want to do --- or we should just make non-Scripted handlers look like no handler at all (i.e. undefined) to JS. ::: js/src/vm/Debugger.h @@ +1241,5 @@ > +}; > + > +class ScriptedOnPopHandler final : public OnPopHandler { > + public: > + ScriptedOnPopHandler(JSObject* object); This is going to need an 'explicit' keyword, right?

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0ecfb7ad9c Remove processResumptionValueHelper. r=jimb

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f82d03a8de0c Factor our processParsedResumptionValue(Helper).

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb39c7c98dd Implement a C++ interface for DebuggerFrame.onStep.

(In reply to Pulsebot from comment #126) > Pushed by ejpbruel@mozilla.com: > https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb39c7c98dd > Implement a C++ interface for DebuggerFrame.onStep. I had to back this out for ASAN leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=35865989&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/056e578d03bb

(In reply to Wes Kocher (:KWierso) from comment #127) > I had to back this out for ASAN leaks Thanks, Wes! I wish I'd caught whatever the problem is when I reviewed. Eddy, I'm very curious to know what the problem was once you've analyzed; if you could post for re-review, that'd be great.

https://hg.mozilla.org/mozilla-central/rev/6f0ecfb7ad9c https://hg.mozilla.org/mozilla-central/rev/f82d03a8de0c

I'm having a hard time actually analysing the ASAN leak reported in comment 127, because I cannot reproduce it locally. I managed to create an ASAN build in my Linux VM, but running it against the failing mochitest (i.e. browser_dbg_breakpoints-reload.js) causes it to crash with the following stack trace: https://pastebin.mozilla.org/ Note that this happens even without the failing patch applied. I don't know if this is a problem with my VM, with my ASAN build, or with something else.

Here's a try push for the fix I proposed in comment 130: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97e99df4fbe6&selectedJob=28067489 The previous ASAN failures are gone, but they are replaced with use-after-free errors. I don't understand how these can happen: if the prior onStep handler is dropped, then the new handler must be null. Since we always replace the onStep handler slot with the new handler after that, the prior handler should no longer be accessible after it has been freed.

Comment on attachment 8783535 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.onStep. Review of attachment 8783535 [details] [diff] [review]: ----------------------------------------------------------------- Some further comments on this. Very sorry for dropping the ball in the initial review. ::: js/src/vm/Debugger.cpp @@ +7152,5 @@ > + > +void > +ScriptedOnStepHandler::drop() > +{ > +} As your try push shows, this isn't right. This is the only possible place the memory allocated by the `cx->new_<ScriptedOnStepHandler>` call in onStepSetter could possibly be freed, but we don't do it. @@ +7409,5 @@ > return DebuggerFrameImplementation::Interpreter; > } > > +/* static */ bool > +DebuggerFrame::setOnStepHandler(JSContext* cx, HandleDebuggerFrame frame, OnStepHandler* handler) Although we're not documenting the C++ API, I think we still need a comment here that this function promises to either: 1) succeed and take ownership of `handler`, or 2) fail and leave ownership of `handler` in the hands of the caller but never anything else. @@ +8129,5 @@ > { > + THIS_DEBUGGER_FRAME(cx, argc, vp, "get onStep", args, frame); > + > + OnStepHandler* handler = frame->onStepHandler(); > + RootedValue value(cx, handler ? ObjectValue(*handler->object()) : UndefinedValue()); For what it's worth, this assertion is going to fire the first time we ever have a non-scripted onStepHandler. Its `object` method will return nullptr, which ObjectValue will barf on. @@ +8155,4 @@ > } > > + if (!DebuggerFrame::setOnStepHandler(cx, frame, handler)) > + return false; Demonstrating the importance of that comment on setOnStepHandler, this function leaks `handler` if this call fails.

(In reply to Eddy Bruel [:ejpbruel] from comment #131) > Here's a try push for the fix I proposed in comment 130: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=97e99df4fbe6&selectedJob=28067489 > > The previous ASAN failures are gone, but they are replaced with > use-after-free errors. I don't understand how these can happen: if the prior > onStep handler is dropped, then the new handler must be null. Since we > always replace the onStep handler slot with the new handler after that, the > prior handler should no longer be accessible after it has been freed. As it turns out, the use-after-free error happened because even though we freed the ScriptedOnStepHandler, we never called its destructor in that patch. ScriptedOnStepHandler stores a JSObject in a HeapPtr. If we don't call the destructor for that HeapPtr, the GC can end up trying to access the HeapPtr after we freed the ScriptedOnStepHandler, thus causing the use-after-free bug. I've resolved that problem, and also addressed Jim's additional review comments in comment 132. Here is a try push for the new patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d44498feceb

This patch makes me want to turn in my badge.

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ddeaf72c3abe Implement a C++ interface for DebuggerFrame.onStep. r=jimb

Try push for the patch to implement a C++ interface for DebuggerFrame.onPop: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4923e3a77574e38dc8fc2efed2ddf7c86ca68e2b

Note that this patch depends on the patch in bug 1271654.

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3da8afdc2497 Implement a C++ interface for DebuggerFrame.onPop. r=jimb

Comment on attachment 8814915 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.getArguments. Review of attachment 8814915 [details] [diff] [review]: ----------------------------------------------------------------- Could I see this one again, with these revisions made? ::: js/src/vm/Debugger.cpp @@ +7571,5 @@ > + MutableHandleDebuggerArguments result) > +{ > + Value argumentsv = frame->getReservedSlot(JSSLOT_DEBUGFRAME_ARGUMENTS); > + if (!argumentsv.isUndefined()) { > + result.set(argumentsv.isObject() ? &argumentsv.toObject().as<DebuggerArguments>() : nullptr); If I'm reading right, dropping the MOZ_ASSERT here silently turns bogus slot values into nullptr. Unless I've misread, let's retain the MOZ_ASSERT from the original. @@ +7575,5 @@ > + result.set(argumentsv.isObject() ? &argumentsv.toObject().as<DebuggerArguments>() : nullptr); > + return true; > + } > + > + AbstractFramePtr referent = DebuggerFrame::getReferent(frame); Before this patch, the code does the equivalent of getReferent, written out in the THIS_FRAME macro's expansion, once. With this patch applied, the code does it twice: once here, and then again in DebuggerArguments::create. Unfortunately, the getReferent operation isn't super-cheap: if you look at the definition of FrameIter::FrameIter(const Data& data) (linked), it sometimes has to construct a js::jit::InlineFrameIterator and then walk it in a few frames, which entails decoding IonMonkey snapshots, which can be expensive. http://searchfox.org/mozilla-central/rev/0c055ccbcf96846044fc9a71421bd9b7978686f7/js/src/vm/Stack.cpp#633 There are argument both ways, but I think DebuggerArguments::create should take an argument count, in addition to the Debugger.Frame. That seems to be the only thing it actually needs from the referent. Then we can extract that here, given the AbstractFramePointer we already have, and just pass it in. @@ +7579,5 @@ > + AbstractFramePtr referent = DebuggerFrame::getReferent(frame); > + > + RootedDebuggerArguments arguments(cx); > + if (referent.hasArgs()) { > + Rooted<GlobalObject*> global(cx, &frame->global()); Could you add the comment: // Allocate our DebuggerArguments object in the same compartment as its Debugger.Frame object. The "frame->global()" expression freaked me out for a bit, because variables named `frame` sometimes refer to the stack frame itself, which is, of course, in the wrong compartment for this. But here it refers to the D.F, which is correct. @@ +8175,2 @@ > { > + AbstractFramePtr referent = DebuggerFrame::getReferent(frame); Let's add, just inside the opening brace: assertSameCompartment(cx, frame, proto); and then, after the declaration of referent: MOZ_ASSERT(referent.hasArgs());

New patch with comments by jimb addressed.

Comment on attachment 8816474 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.arguments (v2). Review of attachment 8816474 [details] [diff] [review]: ----------------------------------------------------------------- This patch seems to have been combined with "Implement a C++ interface for DebuggerArgument.getArgument", but that's fine, I'll review them both together. The DebuggerArguments object should probably have a `length` method as well.

Comment on attachment 8816474 [details] [diff] [review] Implement a C++ interface for DebuggerFrame.arguments (v2). Review of attachment 8816474 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +8186,2 @@ > > + SetReservedSlot(obj, JSSLOT_DEBUGARGUMENTS_FRAME, ObjectValue(*frame)); Shouldn't this be, simply, FRAME_SLOT?

(In reply to Jim Blandy :jimb from comment #145) > This patch seems to have been combined with "Implement a C++ interface for > DebuggerArgument.getArgument", but that's fine, I'll review them both > together. Oh, just Debugger.h was folded in, I guess.

Comment on attachment 8814917 [details] [diff] [review] Implement a C++ interface for DebuggerArgument.getArgument Review of attachment 8814917 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +8111,3 @@ > { > CallArgs args = CallArgsFromVp(argc, vp); > + int32_t index = args.callee().as<JSFunction>().getExtendedSlot(0).toInt32(); If I had my druthers, I would rather get renames as separate patches. For your own sake, I think I'm faster at reviews when there are fewer lines changed due to renames and code motion. @@ +8119,2 @@ > return false; > + if (thisobj->getClass() != &DebuggerArguments::class_) { I think you can write this: if (!thisobj->is<DebuggerArguments>()) { @@ +8136,5 @@ > RootedNativeObject obj(cx, NewNativeObjectWithGivenProto(cx, &DebuggerArguments::class_, proto)); > if (!obj) > return nullptr; > > + SetReservedSlot(obj, DebuggerArguments::FRAME_SLOT, ObjectValue(*frame)); You can just write FRAME_SLOT here, no need for DebuggerArguments::. @@ +8150,5 @@ > > Rooted<jsid> id(cx); > for (unsigned i = 0; i < fargc; i++) { > RootedFunction getobj(cx); > + getobj = NewNativeFunction(cx, DebuggerArguments::getArgumentMethod, 0, nullptr, Similarly: no need for DebuggerArguments::.

Try push for DebuggerFrame.arguments: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3781686422189d660d7e27129414e25aa5c2e6f0

Try push for DebuggerArguments.getArgument: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53180dc35bc3516421208b6511f651f53b62b4c2

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0b58022dda33 Implement a C++ interface for DebuggerFrame.arguments. r=jimb

Jim, what's next here?

I'll take over finishing up this bug. It seems that some of the patches written here have not landed. There may also be remaining work yet to be done. From a preliminary look, it seems that the following patches above haven't been landed: Implement a C++ interface for DebuggerFrame.getType. Stack trace of Debugger.Frame getting its ScriptFrameIter::Data freed. Remove processHandlerResultHelper. Factor out processParsedHandlerResult(Helper). Implement a C++ interface for DebuggerArgument.getArgument Implement a C++ interface for DebuggerFrame.script. These may have been subsumed by other patches; I'll look into this in more detail tomorrow.

It looks like this is almost done. All that's missing is a C++ getter for Debugger.Frame.prototype.script. Rather than looking through the patches in the bug, I looked through the code to see which Debugger.Frame properties and methods lacked corresponding well-typed methods in the js::DebuggerFrame C++ type.

Attachment 8814935 [details] [diff] implements a C++ analogue to the `script` accessor, and is blocked on bug 1271654, a C++ API for Debugger.Script, which has not landed.

The leave-open keyword is there and there is no activity for 6 months. :jimb, maybe it's time to close this bug?

Thanks for the nudge, Release mgmt bot!

I do intend to finish this at some point, though.