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

Note: this patch depends on the patches in bug 1271653.

Comment on attachment 8761214 [details] [diff] [review] Implement a DebuggerEnvironment class. Clearing review on this solely because I won't be able to get to it until July. Eddy discussed flagging Shu instead, which seems fine to me.

(To be clear: if for any reason these are still unreviewed on July 3 - vacations, etc - I'd be happy to do them.)

Comment on attachment 8761214 [details] [diff] [review] Implement a DebuggerEnvironment class. Review of attachment 8761214 [details] [diff] [review]: ----------------------------------------------------------------- I'm not qualified to review this. I don't understand devtools JS.

Comment on attachment 8764367 [details] [diff] [review] Implement a DebuggerEnvironment class. Review of attachment 8764367 [details] [diff] [review]: ----------------------------------------------------------------- r=me with changes applied. ::: js/src/vm/Debugger.cpp @@ +131,3 @@ > "Environment", > JSCLASS_HAS_PRIVATE | > JSCLASS_HAS_RESERVED_SLOTS(JSSLOT_DEBUGENV_COUNT), Get rid of JSSLOT_DEBUGENV_{OWNER,COUNT} and use the static constants inside DebuggerEnvironment. @@ +1050,5 @@ > /* Create a new Debugger.Environment for env. */ > RootedObject proto(cx, &object->getReservedSlot(JSSLOT_DEBUG_ENV_PROTO).toObject()); > + RootedNativeObject debugger(cx, object); > + > + envobj = DebuggerEnvironment::create(cx, proto, env, debugger); For more encapsulation, you could move getting the proto to inside DebuggerEnvironment::create, since you pass |debugger| to it already. @@ +10082,5 @@ > objectProto = DebuggerObject::initClass(cx, obj, debugCtor); > if (!objectProto) > return false; > > + envProto = DebuggerEnvironment::initClass(cx, debugCtor, obj); Nit: swap order of debugCtor and obj for uniformity's sake.

Comment on attachment 8761603 [details] [diff] [review] Implement a C++ interface for DebuggerEnvironment.type. Review of attachment 8761603 [details] [diff] [review]: ----------------------------------------------------------------- r=me with type changed to an instance method. ::: js/src/vm/Debugger.cpp @@ +9595,1 @@ > #define THIS_DEBUGENV(cx, argc, vp, fnname, args, envobj, env) \ Is the plan to phase this out in the later patches? ::: js/src/vm/Debugger.h @@ +1056,5 @@ > static NativeObject* initClass(JSContext* cx, HandleObject dbgCtor, HandleObject objProto); > static DebuggerEnvironment* create(JSContext* cx, HandleObject proto, HandleObject referent, > HandleNativeObject debugger); > > + static DebuggerEnvironmentType type(JSContext* cx, Handle<DebuggerEnvironment*> environment); This should be an instance method. It doesn't do any can-GC operations, so no need to root.

Comment on attachment 8761603 [details] [diff] [review] Implement a C++ interface for DebuggerEnvironment.type. Review of attachment 8761603 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.h @@ +1056,5 @@ > static NativeObject* initClass(JSContext* cx, HandleObject dbgCtor, HandleObject objProto); > static DebuggerEnvironment* create(JSContext* cx, HandleObject proto, HandleObject referent, > HandleNativeObject debugger); > > + static DebuggerEnvironmentType type(JSContext* cx, Handle<DebuggerEnvironment*> environment); Forgot to add: no need to take a cx either.

Comment on attachment 8762022 [details] [diff] [review] Implement a C++ interface for DebuggerEnvironment.parent. Review of attachment 8762022 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.h @@ +1064,5 @@ > HandleNativeObject debugger); > > static DebuggerEnvironmentType type(JSContext* cx, Handle<DebuggerEnvironment*> environment); > + static bool parent(JSContext* cx, Handle<DebuggerEnvironment*> environment, > + MutableHandle<DebuggerEnvironment*> result); I'd name this getParent. There's a convention in SpiderMonkey that fallible things that do non-trivial work have a 'get' prefix. I also don't know why this method is static. It's true that Debugger::wrapEnvironment can GC, but it's a tail call in the method.

Comment on attachment 8762023 [details] [diff] [review] Implement a C++ interface for DebuggerEnvironment.object Review of attachment 8762023 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.h @@ +1066,5 @@ > static DebuggerEnvironmentType type(JSContext* cx, Handle<DebuggerEnvironment*> environment); > static bool parent(JSContext* cx, Handle<DebuggerEnvironment*> environment, > MutableHandle<DebuggerEnvironment*> result); > + static bool object(JSContext* cx, Handle<DebuggerEnvironment*> environment, > + MutableHandle<DebuggerObject*> result); getObject, and again, I don't understand why it's static.

Comment on attachment 8762035 [details] [diff] [review] Implement a C++ interface for DebuggerEnvironment.callee. Review of attachment 8762035 [details] [diff] [review]: ----------------------------------------------------------------- Ditto comments as before. getCallee and why is the method static?

Comment on attachment 8763915 [details] [diff] [review] Implement a C++ interface for DebuggerEnvironment.isDebuggee. Review of attachment 8763915 [details] [diff] [review]: ----------------------------------------------------------------- isDebuggee should be an instance method too. Cancelling r? here since the rest of the patch series needs similar issues address. ::: js/src/vm/Debugger.cpp @@ +9603,5 @@ > } > > +#define THIS_DEBUGENVIRONMENT(cx, argc, vp, fnname, args, environment) \ > + CallArgs args = CallArgsFromVp(argc, vp); \ > + Rooted<DebuggerEnvironment*> environment(cx, DebuggerEnv_checkThis(cx, args, fnname, false)); \ I imagine the false flag will get phased out at some point?

Eddy, could you repost a single patch with all the changes? I personally don't like long patch series where code is slowly being phased out because I end up making useless comments only to find out it's addressed N patches later.

(In reply to Shu-yu Guo [:shu] from comment #18) > Comment on attachment 8764367 [details] [diff] [review] > Implement a DebuggerEnvironment class. > > Review of attachment 8764367 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with changes applied. > > ::: js/src/vm/Debugger.cpp > @@ +131,3 @@ > > "Environment", > > JSCLASS_HAS_PRIVATE | > > JSCLASS_HAS_RESERVED_SLOTS(JSSLOT_DEBUGENV_COUNT), > > Get rid of JSSLOT_DEBUGENV_{OWNER,COUNT} and use the static constants inside > DebuggerEnvironment. > The patch "Remove obsolete enums and macros for DebuggerEnvironment." does this after all JS native functions have been rewritten to use the static constants. > @@ +1050,5 @@ > > /* Create a new Debugger.Environment for env. */ > > RootedObject proto(cx, &object->getReservedSlot(JSSLOT_DEBUG_ENV_PROTO).toObject()); > > + RootedNativeObject debugger(cx, object); > > + > > + envobj = DebuggerEnvironment::create(cx, proto, env, debugger); > > For more encapsulation, you could move getting the proto to inside > DebuggerEnvironment::create, since you pass |debugger| to it already. > I will do this in a followup patch. > @@ +10082,5 @@ > > objectProto = DebuggerObject::initClass(cx, obj, debugCtor); > > if (!objectProto) > > return false; > > > > + envProto = DebuggerEnvironment::initClass(cx, debugCtor, obj); > > Nit: swap order of debugCtor and obj for uniformity's sake. Ok.

(In reply to Shu-yu Guo [:shu] from comment #25) > Eddy, could you repost a single patch with all the changes? I personally > don't like long patch series where code is slowly being phased out because I > end up making useless comments only to find out it's addressed N patches > later. (In reply to Shu-yu Guo [:shu] from comment #21) > Comment on attachment 8762022 [details] [diff] [review] > Implement a C++ interface for DebuggerEnvironment.parent. > > Review of attachment 8762022 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/vm/Debugger.h > @@ +1064,5 @@ > > HandleNativeObject debugger); > > > > static DebuggerEnvironmentType type(JSContext* cx, Handle<DebuggerEnvironment*> environment); > > + static bool parent(JSContext* cx, Handle<DebuggerEnvironment*> environment, > > + MutableHandle<DebuggerEnvironment*> result); > > I'd name this getParent. There's a convention in SpiderMonkey that fallible > things that do non-trivial work have a 'get' prefix. Ok. I will edit my patches to reflect that convention. I already landed similar patches for DebuggerObject that doesn't use this convention. I will file a followup patch to address this. > > I also don't know why this method is static. It's true that > Debugger::wrapEnvironment can GC, but it's a tail call in the method. In general, is it ok for a function to not be static if it only does GC in tail calls?

(In reply to Shu-yu Guo [:shu] from comment #24) > Comment on attachment 8763915 [details] [diff] [review] > Implement a C++ interface for DebuggerEnvironment.isDebuggee. > > Review of attachment 8763915 [details] [diff] [review]: > ----------------------------------------------------------------- > > isDebuggee should be an instance method too. > > Cancelling r? here since the rest of the patch series needs similar issues > address. > > ::: js/src/vm/Debugger.cpp > @@ +9603,5 @@ > > } > > > > +#define THIS_DEBUGENVIRONMENT(cx, argc, vp, fnname, args, environment) \ > > + CallArgs args = CallArgsFromVp(argc, vp); \ > > + Rooted<DebuggerEnvironment*> environment(cx, DebuggerEnv_checkThis(cx, args, fnname, false)); \ > > I imagine the false flag will get phased out at some point? Yes, it will.

(In reply to Shu-yu Guo [:shu] from comment #25) > Eddy, could you repost a single patch with all the changes? I personally > don't like long patch series where code is slowly being phased out because I > end up making useless comments only to find out it's addressed N patches > later. Sure thing. I will address your review comments and refile as a single patch.

Here is everything in one patch. N.b. this also includes the patches you already reviewed, with review comments addressed.

Comment on attachment 8764582 [details] [diff] [review] Everything in one patch Review of attachment 8764582 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: js/src/vm/Debugger.cpp @@ +9863,5 @@ > + > +/* static */ DebuggerEnvironmentType > +DebuggerEnvironment::type() const > +{ > + /* Don't bother switching compartments just to check env's class. */ Stale comment. "check env's type", I guess. @@ +9869,5 @@ > + return DebuggerEnvironmentType::Declarative; > + else if (IsDebugScopeWrapper<DynamicWithObject>(referent())) > + return DebuggerEnvironmentType::With; > + else > + return DebuggerEnvironmentType::Object; Style nit: no else after return. @@ +9893,5 @@ > + > + /* > + * Don't bother switching compartments just to check env's class and > + * possibly get its proto. > + */ This comment is stale too. @@ +9959,5 @@ > + MOZ_ASSERT(environment->isDebuggee()); > + > + Rooted<Env*> referent(cx, environment->referent()); > + > + AutoIdVector ids(cx); As an aside, it's too bad GetPropertyKeys takes AutoIdVector instead of the new GCVector stuff so we have to do this copying dance. @@ +9977,2 @@ > return false; > + } Nit: stray whitespace

Try run for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=833aff0340af

Previous try run had assertion failures due to using the wrong prototype for DebuggerEnvironment::class_. Here is a new try run with that issue addressed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=850e4eadc528

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f5b378eddb42 Implement a C++ interface for Debugger.Environment instances;r=shu

A while ago, we agreed on the style rule that a method should be static if and only if it takes a JSContext as argument (and is therefore fallible). By those rules, DebuggerEnvironment::requireDebuggee should be a static method.

For clarity, we now call requireDebuggee explicitly in each method. As a consequence, the requireDebuggee argument passed to checkThis is now always false, so the corresponding code path can be removed.

Comment on attachment 8769653 [details] [diff] [review] DebuggerEnvironment.requireDebuggee should be a static method. Review of attachment 8769653 [details] [diff] [review]: ----------------------------------------------------------------- Great!

(In reply to Eddy Bruel [:ejpbruel] from comment #36) > Created attachment 8769653 [details] [diff] [review] > DebuggerEnvironment.requireDebuggee should be a static method. > > A while ago, we agreed on the style rule that a method should be static if > and only if it takes a JSContext as argument (and is therefore fallible). By > those rules, DebuggerEnvironment::requireDebuggee should be a static method. We made some methods static because having a Cell pointer as `this` is asking for trouble, since the GC can't relocate it. That has nothing to do with fallibility.

That is, methods of types that inherit from Cell should be static if they might GC.

(In reply to Jim Blandy :jimb from comment #39) > (In reply to Eddy Bruel [:ejpbruel] from comment #36) > > Created attachment 8769653 [details] [diff] [review] > > DebuggerEnvironment.requireDebuggee should be a static method. > > > > A while ago, we agreed on the style rule that a method should be static if > > and only if it takes a JSContext as argument (and is therefore fallible). By > > those rules, DebuggerEnvironment::requireDebuggee should be a static method. > > We made some methods static because having a Cell pointer as `this` is > asking for trouble, since the GC can't relocate it. That has nothing to do > with fallibility. You are right. I should have been more careful in my wording. What I meant to say was: 'a method should be static if and only if it takes a JSContext as argument (and can therefore GC)'. The rule we have is that a method should be static if and only if the this pointer can be moved by the GC, presumably because the method calls one or more functions that could trigger the GC. My worry was that it is not always obvious which methods qualify as such, so we could end up introducing GC hazards if we erroneously assume that a method cannot trigger the GC. To avoid this, I proposed a compromise where we only allow a method to be non-static if it does not take a JSContext* as parameter. The assumption was that if a method does not take a JSContext* as parameter, it cannot possibly GC, and therefore it is always safe to make it non-static. As we have seen with DebuggerFrame::referent, this assumption does not always hold. I am still of the opinion that we should just make every method static: style rules are intended to minimise the probability of human error. Allowing certain methods to be non-static forces you to think about whether your method can trigger the GC or not. As we have seen, it is all too easy to get this wrong. In fairness, we have the static rooting analyser to save us when we make a mistake, but that still does not make this a good style rule. That said, I made this argument to Shu yesterday, and he still thinks making every method static is overkill. It's not my call to make, so consider this an instance of me grumbling before getting back to business ;-)

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

Jason,could you help figure what to do this old bug that is still has the leave-open keyword?

Closing because it's not clear that there's any other way to pacify the bot. It can be reopened if anyone ever needs this. Although you'd have to specifically be searching for a RESOLVED bug, or you won't see it...