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

Implementing the entire C++ interface for Debugger.Object instances in one go would likely lead to a very large patch. Large patches are always a pain to review, so to make life easier on you, I'm taking a incremental approach instead. Luckily, the work required lends itself well to such an incremental approach; we can simply refactor one method at a time. This patch adds the DebuggerObject class, with methods to initialize the class and create instances of itself. It also implements a C++ interface for the isSealed/Frozen/Extensible methods. This seemed like a good way to start. I'm putting this patch up early so you have an opportunity to comment on the overall approach. If you have any issues, please let me know asap, so I can continue working on the rest of the work, without risking having to make significant changes to the patches later.

Fixed a minor oversight in the patch (forgot to remove a now obsolete function).

Comment on attachment 8750812 [details] [diff] [review] Implement a C++ interface for isSealed/Frozen/Extensible. Review of attachment 8750812 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I just had a few comments. Ideally, we'll pull all the DebuggerObject_blah functions into DebuggerObject, but I'm glad all that mechanical work is not mixed into this patch. ::: js/src/vm/Debugger.cpp @@ +7648,5 @@ > } > > +#define THIS_DEBUGOBJECT(cx, argc, vp, fnname, args, object) \ > + CallArgs args = CallArgsFromVp(argc, vp); \ > + RootedObject obj(cx, DebuggerObject_checkThis(cx, args, fnname)); \ This should take `obj, object` as its last two arguments, so that the users of this macro get to specify the name of the checked `this` variable. The other THIS_ macros follow that pattern. @@ +8849,5 @@ > +} > + > +/* static */ DebuggerObject* > +DebuggerObject::create(JSContext* cx, HandleObject proto, HandleObject referent, > + HandleNativeObject debugger) Isn't it the case that `proto` is always fetched from `debugger`? We could drop that argument, and make this function fetch it itself. ::: js/src/vm/Debugger.h @@ +1047,5 @@ > + } > + > + bool isExtensible(JSContext* cx, bool* result) { > + return isSealedHelper(cx, OpPreventExtensions, "isExtensible", result); > + } Let's just use isSealedHelper for isSealed and isFrozen only, drop SealHelperOp, and use the existing IntegrityLevel enum instead. I think that will simplify isSealedHelper a bit. isExensible should be its own separate function, define out-of-line in Debugger.cpp.

(In reply to Jim Blandy :jimb from comment #3) > Comment on attachment 8750812 [details] [diff] [review] > Implement a C++ interface for isSealed/Frozen/Extensible. > > Review of attachment 8750812 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. I just had a few comments. > > Ideally, we'll pull all the DebuggerObject_blah functions into > DebuggerObject, but I'm glad all that mechanical work is not mixed into this > patch. > Yes. Let's do that in a later patch. > ::: js/src/vm/Debugger.cpp > @@ +7648,5 @@ > > } > > > > +#define THIS_DEBUGOBJECT(cx, argc, vp, fnname, args, object) \ > > + CallArgs args = CallArgsFromVp(argc, vp); \ > > + RootedObject obj(cx, DebuggerObject_checkThis(cx, args, fnname)); \ > > This should take `obj, object` as its last two arguments, so that the users > of this macro get to specify the name of the checked `this` variable. The > other THIS_ macros follow that pattern. That's not entirely correct. The other macros use `obj` to specify the name of the referent. The checked `this` value is never visible to the users of those macros. This macro uses `object` to refer to the name of the checked `this` value. The 'obj' variable is used as a temporary, and is only there because I cannot use Rooted<DebuggerObject> directly. It will never be used by users of this macro. That said, it's probably still better to allow users of this macro to give `obj` an explicit name, even though it's just a temporary, so I'll make the requested change. > > @@ +8849,5 @@ > > +} > > + > > +/* static */ DebuggerObject* > > +DebuggerObject::create(JSContext* cx, HandleObject proto, HandleObject referent, > > + HandleNativeObject debugger) > > Isn't it the case that `proto` is always fetched from `debugger`? We could > drop that argument, and make this function fetch it itself. > The way I think of it is that NativeObjects have fixed slots that represent their internal data. This internal data should only be exposed via getters (such as referent() on DebuggerObject). With that in mind, I wanted to avoid accessing the fixed slots of Debugger within the methods of DebuggerObject. It feels better encapsulated this way, so I'd prefer to keep it the way it is. If you strongly object to that, let me know so we can discuss it. > ::: js/src/vm/Debugger.h > @@ +1047,5 @@ > > + } > > + > > + bool isExtensible(JSContext* cx, bool* result) { > > + return isSealedHelper(cx, OpPreventExtensions, "isExtensible", result); > > + } > > Let's just use isSealedHelper for isSealed and isFrozen only, drop > SealHelperOp, and use the existing IntegrityLevel enum instead. I think that > will simplify isSealedHelper a bit. isExensible should be its own separate > function, define out-of-line in Debugger.cpp. Agreed.

(In reply to Eddy Bruel [:ejpbruel] from comment #4) > > @@ +8849,5 @@ > > > +} > > > + > > > +/* static */ DebuggerObject* > > > +DebuggerObject::create(JSContext* cx, HandleObject proto, HandleObject referent, > > > + HandleNativeObject debugger) > > > > Isn't it the case that `proto` is always fetched from `debugger`? We could > > drop that argument, and make this function fetch it itself. > > > > The way I think of it is that NativeObjects have fixed slots that represent > their internal data. This internal data should only be exposed via getters > (such as referent() on DebuggerObject). With that in mind, I wanted to avoid > accessing the fixed slots of Debugger within the methods of DebuggerObject. > > It feels better encapsulated this way, so I'd prefer to keep it the way it > is. > If you strongly object to that, let me know so we can discuss it. Yeah, I can see that. Ideally, we'd have a NativeObject subclass for Debugger as well, with accessors (at the C++ level, of course) for the prototypes.

I'm a little confused by these patches. The preventExtensions/seal/freeze patch removes a definition for enum SealHelperOp from Debugger.cpp, while the hunk for Debugger.h shows another definition for SealHelperOp, local to DebuggerObject, being unchanged. Could I see these patches rebased per comment 4? Also, I just thought of something unfortunate: It's not possible to use non-static methods in JSObject subclasses for anything very significant, because there's no way to put the `this` pointer in a rooted location. This is why everything ends up using static methods that take a HandleObject. The other approach is to use a this-ful method, but then declare a Rooted<NativeObject*> self at the top of the function and always use that. It's not pretty. If you screw up, the static analysis will catch you. So make sure these patches get a static analysis try push before you try to land them.

(In reply to Jim Blandy :jimb from comment #9) > I'm a little confused by these patches. The preventExtensions/seal/freeze > patch removes a definition for enum SealHelperOp from Debugger.cpp, while > the hunk for Debugger.h shows another definition for SealHelperOp, local to > DebuggerObject, being unchanged. Could I see these patches rebased per > comment 4? There's nothing magic going on here. I made the SealOp enum a member of DebuggerObject in the first patch. I had to keep the previous (non-member) SealOp enum around, however, because it was still used by preventExtensions/seal/freeze (via sealHelper, which this patch didn't touch). In this patch, I made preventExtensions/seal/freeze members of DebuggerObject as well, so the non-member SealOp enum is no longer required. Sorry if that led to confusion. I already based these patches on the rebased patch 1, as per comment 4, so there's not much I can do here. In light of the above, could you please look at the patches again? > > Also, I just thought of something unfortunate: > > It's not possible to use non-static methods in JSObject subclasses for > anything very significant, because there's no way to put the `this` pointer > in a rooted location. This is why everything ends up using static methods > that take a HandleObject. > > The other approach is to use a this-ful method, but then declare a > Rooted<NativeObject*> self at the top of the function and always use that. > It's not pretty. > That's what I made the DEBUGGER_OBJECT macro for. As it turns out, you can't use Rooted<DebuggerObject> directly, because the Rooting API expects DebuggerObject to have a trace method. Instead, I create a RootedObject, make sure it stays on the stack, and then obtain a DebuggerObject& from that, on which we can call the method. > If you screw up, the static analysis will catch you. So make sure these > patches get a static analysis try push before you try to land them. What try settings should I use for that?

Comment on attachment 8751186 [details] [diff] [review] Implement a C++ interface for preventExtensions/seal/freeze. Rerequesting review as per comment 11. If things are still not clear, please hook me up on irc.

(In reply to Eddy Bruel [:ejpbruel] from comment #11) > > > > Also, I just thought of something unfortunate: > > > > It's not possible to use non-static methods in JSObject subclasses for > > anything very significant, because there's no way to put the `this` pointer > > in a rooted location. This is why everything ends up using static methods > > that take a HandleObject. > > > > The other approach is to use a this-ful method, but then declare a > > Rooted<NativeObject*> self at the top of the function and always use that. > > It's not pretty. > > > > That's what I made the DEBUGGER_OBJECT macro for. As it turns out, you can't > use Rooted<DebuggerObject> directly, because the Rooting API expects > DebuggerObject to have a trace method. Instead, I create a RootedObject, > make sure it stays on the stack, and then obtain a DebuggerObject& from > that, on which we can call the method. I don't think this would be too painful for the JS interface forwarding the C++ interface; the DEBUGGER_OBJECT macro guarantees that the `this` pointer is always rooted before we call into the methods on DebuggerObject. However, for other consumers of the C++ interface, such as the high-level Rust API we intend to build, we would have to make sure that the DebuggerObject instance is rooted before calling into any of the methods. The C++ type system doesn't help us out there either. I'm not sure how much of an issue that would be in practice. It does introduce a possibility for error, but it's a fairly well defined one, and once we wrap the C++ interface in the higher level Rust API, we no longer have to worry about it, since it should take care of the rooting for us. On the other hand, I think current practice in SpiderMonkey is to just turn the member functions into static functions, and make sure the this value is rooted there. That doesn't really solve the problem so much as move it around, so I'm not sure what's the best option here. Jim, what do you think? Should I change the existing patches to use static functions instead of member functions? Personally, I dislike it, but it does seem to be established practice within SM.

This broke SPIDERMONKEY_PROMISE builds. You moved promiseProperties to a private static class member, but its used outside of the class in JS_DefineDebuggerObject.

(In reply to Ben Kelly [:bkelly] from comment #15) > This broke SPIDERMONKEY_PROMISE builds. You moved promiseProperties to a > private static class member, but its used outside of the class in > JS_DefineDebuggerObject. Thanks for letting me know Ben. Turns out I moved it to DebuggerObject::initClass, but then forgot to remove it from JS_DefineDebuggerObject. I'll write up a fix for that later in the day.

Here's the promised patch to fix some compile errors I introduced to Debugger.cpp for SPIDERMONKEY_PROMISE builds (and also some that were already there, apparently).

Comment on attachment 8752203 [details] [diff] [review] Fix SPIDERMONKEY_PROMISE errors in Debugger.cpp. Review of attachment 8752203 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

(In reply to Eddy Bruel [:ejpbruel] from comment #13) > I don't think this would be too painful for the JS interface forwarding the > C++ interface; the DEBUGGER_OBJECT macro guarantees that the `this` pointer > is always rooted before we call into the methods on DebuggerObject. To be clear: we're talking about cases where the type of `this` is `JSObject *` or some subclass. It's not good enough to just store the pointer in a Rooted<T> somewhere, because the GC now relocates objects. When that happens, the GC certainly updates the Rooted<T>, but the `this` pointer becomes a dangling pointer. So you have to be careful never to use `this`, even implicitly. There are some very surprising cases where things can go south, for example: Rooted<JSObject*> obj; obj->setSlot(... expression that may GC ...) is unsafe, because the compiler is free to fetch the `this` value for the call to setSlot from obj and store it in a temporary before evaluating the arguments. If the GC moves the object, obj will be updated, but the previously fetched `this` may not. (The static analysis catches this.) This is why SM switched to static methods for everything: it's just less confusing to not even have `this` pointers around.

Comment on attachment 8751186 [details] [diff] [review] Implement a C++ interface for preventExtensions/seal/freeze. Review of attachment 8751186 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +8398,3 @@ > > + if (!object.seal(cx)) > + return false; This is nice. @@ +8873,5 @@ > + ac.emplace(cx, obj); > + ErrorCopier ec(ac); > + return PreventExtensions(cx, obj); > + > +} Stray blank line. @@ +8895,5 @@ > return true; > } > > +bool > +DebuggerObject::sealHelper(JSContext* cx, SealHelperOp op, const char* name) What I meant at the end of comment 3 was that we could delete SealHelperOp entirely (or rather, not introduce it), and use IntegrityLevel throughout. SealHelperOp is just redundant.

Comment on attachment 8751188 [details] [diff] [review] Implement a C++ interface for getOwnPropertyNames/Symbols. Review of attachment 8751188 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +8891,5 @@ > return true; > } > > +bool > +DebuggerObject::getOwnPropertyKeys(JSContext* cx, unsigned flags, AutoValueVector* result) const Since `result` can't be null, SpiderMonkey style says this should be a reference, not a pointer.

Comment on attachment 8751781 [details] [diff] [review] Implement a C++ interface for define/deleteProperty. Review of attachment 8751781 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.h @@ +1074,4 @@ > private: > + enum { > + JSSLOT_DEBUGOBJECT_OWNER, > + JSSLOT_DEBUGOBJECT_COUNT Following the pattern of other analogous enums, the first should be named OWNER_SLOT, and JSSLOT_DEBUGOBJECT_COUNT should be dropped entirely in favor of DebuggerObject::RESERVED_SLOTS. See, for example: https://dxr.mozilla.org/mozilla-central/source/js/src/vm/ScopeObject.h#360

Comment on attachment 8750812 [details] [diff] [review] Implement a C++ interface for isSealed/Frozen/Extensible. Review of attachment 8750812 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +7651,5 @@ > + CallArgs args = CallArgsFromVp(argc, vp); \ > + RootedObject obj(cx, DebuggerObject_checkThis(cx, args, fnname)); \ > + if (!obj) \ > + return false; \ > + DebuggerObject& object = obj->as<DebuggerObject>(); \ I just realized: this isn't going to work. If a GC moves anything, obj will be updated, but not this copy of that pointer, object. Sorry for not picking this up.

(In reply to Eddy Bruel [:ejpbruel] from comment #11) > (In reply to Jim Blandy :jimb from comment #9) > > If you screw up, the static analysis will catch you. So make sure these > > patches get a static analysis try push before you try to land them. > > What try settings should I use for that? try: -b d -p linux64-sh-haz -u none -t none It's the SM(r) build in the treeherder results.

(In reply to Jim Blandy :jimb from comment #25) > It's the SM(r) build in the treeherder results. Sorry, I think it's SM-tc(H), actually. https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=27878791

(In reply to Jim Blandy :jimb from comment #20) > (In reply to Eddy Bruel [:ejpbruel] from comment #13) > > I don't think this would be too painful for the JS interface forwarding the > > C++ interface; the DEBUGGER_OBJECT macro guarantees that the `this` pointer > > is always rooted before we call into the methods on DebuggerObject. > > To be clear: we're talking about cases where the type of `this` is `JSObject > *` or some subclass. > > It's not good enough to just store the pointer in a Rooted<T> somewhere, > because the GC now relocates objects. When that happens, the GC certainly > updates the Rooted<T>, but the `this` pointer becomes a dangling pointer. > > So you have to be careful never to use `this`, even implicitly. There are > some very surprising cases where things can go south, for example: > > Rooted<JSObject*> obj; > obj->setSlot(... expression that may GC ...) > > is unsafe, because the compiler is free to fetch the `this` value for the > call to setSlot from obj and store it in a temporary before evaluating the > arguments. If the GC moves the object, obj will be updated, but the > previously fetched `this` may not. (The static analysis catches this.) > > This is why SM switched to static methods for everything: it's just less > confusing to not even have `this` pointers around. Thanks for the explanation Jim. It's been a while since I last touched SpiderMonkey code; looks like my understanding of rooting has gotten rusty. One of the above patches has already landed. I will write a patch to change that code so that it never references `this`, and will rewrite the patches that didn't land yet in the same way.

(In reply to Eddy Bruel [:ejpbruel] from comment #28) > Thanks for the explanation Jim. It's been a while since I last touched > SpiderMonkey code; looks like my understanding of rooting has gotten rusty. > > One of the above patches has already landed. I will write a patch to change > that code so that it never references `this`, and will rewrite the patches > that didn't land yet in the same way. Okay, that'll be great. I'll look forward to the reviews, and try to turn them around quickly.

This patch fixes the issues with the patch that already landed. In particular, it avoids referring to the `this` value, turning the existing member functions into static functions. It also gets rid of isSealedHelper, as per your request.

Comment on attachment 8753416 [details] [diff] [review] Refactor isExtensible/isSealed/isFrozen Review of attachment 8753416 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, this looks great. Just one thinko to point out: ::: js/src/vm/Debugger.cpp @@ +7648,5 @@ > } > > +#define THIS_DEBUGOBJECT(cx, argc, vp, fnname, object) \ > + CallArgs args = CallArgsFromVp(argc, vp); \ > + Rooted<DebuggerObject*> object(cx, DebuggerObject_checkThis(cx, args, "isExtensible")); \ I don't think you want "isExtensible" in there as a string constant!

Comment on attachment 8753840 [details] [diff] [review] Implement a C++ interface for preventExtensions/seal/freeze. Review of attachment 8753840 [details] [diff] [review]: ----------------------------------------------------------------- Looks great.

As it turns out, I forgot to explicitly name the args parameter in the THIS_DEBUGOBJECT macro. This patch rectifies that. I also took the opportunity for rename all instances Debugger* owner to Debugger* dbg. This is what we did before. After working with the former for a while, I feel that the latter more clearly expresses what kind of object we are operating on.

One thing I noticed while writing the last patch is that a lot of arguments end up having to be mutable because we need to unwrap/rewrap them. This is really an implementation detail of these functions, so the fact that this shows up in their arguments is not desirable. It also creates side effects in the sense that arguments may no longer be in the compartment you expect after calling a function. Could we change these functions so that they take non-mutable arguments, create a rooted copy of each of these arguments in the function, and then unwrap/rewrap those?

(In reply to ejpbruel from comment #42) > One thing I noticed while writing the last patch is that a lot of arguments > end up having to be mutable because we need to unwrap/rewrap them. This is > really an implementation detail of these functions, so the fact that this > shows up in their arguments is not desirable. It also creates side effects > in the sense that arguments may no longer be in the compartment you expect > after calling a function. > > Could we change these functions so that they take non-mutable arguments, > create a rooted copy of each of these arguments in the function, and then > unwrap/rewrap those? Yes, that would be great. Much better. If you apply this to functions I've already reviewed, it will be faster for me to review patches that address only this issue, rather than revisions of the patches that add the functions from scratch.

(In reply to ejpbruel from comment #40) > I also took the opportunity for rename all instances Debugger* owner to > Debugger* dbg. This is what we did before. After working with the former for > a while, I feel that the latter more clearly expresses what kind of object > we are operating on. I don't have a strong opinion, but consistency with existing use is definitely a good thing.

Comment on attachment 8754387 [details] [diff] [review] Implement a C++ interface for defineProperties. Review of attachment 8754387 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +9038,5 @@ > +{ > + RootedObject referent(cx, object->referent()); > + Debugger* owner = object->owner(); > + > + size_t n = ids.length(); You should probably assert that ids->length() == descs->length().

Comment on attachment 8754397 [details] [diff] [review] Implement a C++ interface for call/apply. Review of attachment 8754397 [details] [diff] [review]: ----------------------------------------------------------------- You've deleted all uses of `enum ApplyOrCallMode` and the static `ApplyOrCall' function, so those should be deleted. This omits the checks currently in `ApplyOrCall` that we don't try to pass more than `ARGS_LENGTH_MAX` arguments. Those should be preserved. ::: js/src/vm/Debugger.cpp @@ +8593,2 @@ > { > + THIS_DEBUGOBJECT(cx, argc, vp, "call", callArgs, object); I think you mean "apply", and probably applyArgs.

This patch contains various API fixes: - Remove the use of MutableHandle arguments in those cases where they are only necessary because we need to unwrap/rewrap the arguments. Instead, we pass a Handle argument, create a Rooted copy of that, and unwrap/rewrap that instead. - Remove the uses of AutoIdVector and AutoValueVector. According to jandem and till, the preferred APIs these days are IdVector and ValueVector, resp., so we should prefer to use those when defining a new interface. Note that some APIs still expect AutoIdVector or AutoValueVector, resp., so in some places a conversion step will be required for the time being. - Made some minor whitespace changes where I felt it clarified the code.

Comment on attachment 8754861 [details] [diff] [review] Various API fixes. Review of attachment 8754861 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, with some comments. ::: js/src/vm/Debugger.cpp @@ +8304,2 @@ > { > AutoValueVector vals(cx); Shouldn't this AutoValueVector go as well? @@ +8449,5 @@ > if (!ReadPropertyDescriptors(cx, props, false, &ids, &descs)) > return false; > + Rooted<IdVector> ids2(cx, IdVector(cx)); > + if (!ids2.append(ids.begin(), ids.end())) > + return false; Avoiding this copy would entail fixing GetPropertyKeys, which is used everywhere. *sigh* Oh well. @@ +8856,4 @@ > { > RootedObject referent(cx, object->referent()); > > + AutoIdVector ids(cx); Okay, this AutoIdVector is also still here because of GetPropertyKeys. @@ +8974,5 @@ > { > RootedObject referent(cx, object->referent()); > Debugger* dbg = object->owner(); > > + Rooted<PropertyDescriptor> desc2(cx, desc); SpiderMonkey has a convention for arguments that need to be immediately copied into a rooted location: the argument is called desc_, and then the rooted copy is called desc. This way, most of the uses get the simpler name. For example: https://hg.mozilla.org/mozilla-central/file/1579b9e2e50f/js/src/vm/ScopeObject.cpp#l2342 Here we just want to be able to wrap the thing without affecting the caller, but I think it's a close enough analogy that we should follow the pattern. @@ +9000,5 @@ > { > RootedObject referent(cx, object->referent()); > Debugger* dbg = object->owner(); > > + Rooted<PropertyDescriptorVector> descs2(cx, PropertyDescriptorVector(cx)); Similarly here: descs_ and descs. @@ +9041,5 @@ > } > > /* static */ bool > +DebuggerObject::call(JSContext* cx, Handle<DebuggerObject*> object, HandleValue thisv, > + Handle<ValueVector> args, MutableHandleValue result) Similarly: thisv_, thisv.

Evaluation in the debugger is implemented in terms of DebuggerGenericEval. We want to give this function a strongly typed interface, so it can be called directly by the C++ functions on DebuggerObject for executeInGlobal/executeInGlobalWithBindings. This turns out to be a little bit more work than the previous patches, so I've divided the work into four patches. In the first patch (i.e. this one) I introduce a RAII class named EvalOptions, which can be used to pass eval options from C++ (without having to use a weakly typed JSObject).

At the moment, DebuggerGenericEval takes a HandleString containing the code to be evaluated as argument. For the C++ interface (where we don't always have the code in a JSString), it would be more convenient to use mozilla::Range<const char16_t>. This requires the JS interface function to obtain a mozilla:Range<const char16_t> from a JSValue if they want to use DebuggerGenericEval, so I've added a ValueToStableChars helper function to reduce code duplication somewhat.

Using HandleObject instead of HandleValue for the bindings argument is slightly stronger typed. I also makes the use of the EvalBindings enum unnecessary, since we can distinguish between bindings/no bindings by checking if the HandleObject is nullptr.

Now that DebuggerGenericEval is strongly typed, we can define C++ functions for executeInGlobal/executeInGlobalWithBindings, and have them call DebuggerGenericEval directly.

Comment on attachment 8755408 [details] [diff] [review] Use mozilla::<const char16_t> for eval code instead of HandleValue. Review of attachment 8755408 [details] [diff] [review]: ----------------------------------------------------------------- Looks great.

By the way, I think most of these patches are mis-titled.

And attachment 8755406 [details] [diff] [review] and attachment 8755411 [details] [diff] [review] are the same patch. And whatever patch introduces ValueToStableChars is nowhere to be found, only one that moves it. You should look your patches over before posting them for review. Please correct things, and then re-request review.

I don't know how I ended up attaching the wrong patches. Apologies for wasting your time like that. I'm going to reattach the patches after checking them over. It looks like the patch you already r+'d was supposed to be the first one, so I'm carrying over your r+ for that one.

https://hg.mozilla.org/mozilla-central/rev/57f5f7e9c22f https://hg.mozilla.org/mozilla-central/rev/6d54549fc904 https://hg.mozilla.org/mozilla-central/rev/144fe3771970 https://hg.mozilla.org/mozilla-central/rev/ba7c84e1ebed https://hg.mozilla.org/mozilla-central/rev/a9db912dc99a https://hg.mozilla.org/mozilla-central/rev/76278dc8e385 https://hg.mozilla.org/mozilla-central/rev/bce289607686 https://hg.mozilla.org/mozilla-central/rev/4d524c2e2b2e https://hg.mozilla.org/mozilla-central/rev/374516c9221a https://hg.mozilla.org/mozilla-central/rev/2ff5471d5dd3

Comment on attachment 8755769 [details] [diff] [review] Use mozilla::<const char16_t> for eval code instead of HandleValue. Review of attachment 8755769 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. ::: js/src/vm/Debugger.cpp @@ +7560,5 @@ > } > > +static bool > +ValueToStableChars(JSContext* cx, const char *fnname, HandleValue value, > + AutoStableStringChars& stableChars) Pulling this out of DebuggerGenericEval is a nice cleanup.

Oh, make sure you fix the log message on that patch before you commit - I think you mean mozilla::Range<const char16_t>.

Comment on attachment 8755772 [details] [diff] [review] Use HandleObject for eval bindings instead of HandleValue. Review of attachment 8755772 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. ::: js/src/vm/Debugger.cpp @@ +7579,1 @@ > enum EvalBindings { EvalHasExtraBindings = true, EvalWithDefaultBindings = false }; I think you've removed the only use of this enum, so it can be deleted too.

Comment on attachment 8756357 [details] [diff] [review] Implement a C++ interface for asEnvironment. Review of attachment 8756357 [details] [diff] [review]: ----------------------------------------------------------------- Could we hold off on this patch until after we've introduced a DebuggerEnvironment type? Then we can use tighter types for the signature.

These patches have a lot of function reordering in them, where a function moves to a different spot in the file. I'd readily believe that the new locations may make more sense, so that's all good. But reading over the patches, it's hard for me to tell whether you've modified anything in the function body. Would it be possible to keep patches that move a function strictly separate from patches that modify anything in it? It would make it a lot easier to see those changes (as in, 'diff' would actually work). I think you've been doing this so far anyway; I'd just like to formally ask for it, as a reviewer.

(In reply to Jim Blandy :jimb from comment #79) > These patches have a lot of function reordering in them, where a function > moves to a different spot in the file. I'd readily believe that the new > locations may make more sense, so that's all good. But reading over the > patches, it's hard for me to tell whether you've modified anything in the > function body. > > Would it be possible to keep patches that move a function strictly separate > from patches that modify anything in it? It would make it a lot easier to > see those changes (as in, 'diff' would actually work). I think you've been > doing this so far anyway; I'd just like to formally ask for it, as a > reviewer. Sure thing! I'll make sure to do so in any future patches. I didn't really consider using a DebuggerEnvironment type before. It sounds like a good idea, but I'm not sure what such a type would look like. I assume we can't simply use a MutableHandle<Env*>, so did you have something specific in mind?

I just meant, a special NativeObject subclass for Debugger.Environment instances, just like you've already done for Debugger.Object. The purpose of Debugger.Object.prototype.asEnvironment is simply to produce a Debugger.Environment, given a Debugger.Object whose referent is a global object.

The `Env` type is only used within Debugger to refer to JSObjects that even non-debugged code uses to represent binding environments. Updated comment in bug 1275730. Those should never be exposed outside SpiderMonkey.

(In reply to Jim Blandy :jimb from comment #81) > I just meant, a special NativeObject subclass for Debugger.Environment > instances, just like you've already done for Debugger.Object. The purpose of > Debugger.Object.prototype.asEnvironment is simply to produce a > Debugger.Environment, given a Debugger.Object whose referent is a global > object. If I understand you correctly, this subclass of NativeObject would (at least for the moment) not have any methods or other members, and would just serve as a type marker to make sure we don't use a JSObject that is not a DebuggerEnvironment in places where the latter is expected. Is that correct? If so, that is easy to do, and I should be able to update the patch accordingly tomorrow.

Right. When we provide C++ bindings for the Debugger.Environment object methods, then they'd be static methods of DebuggerEnvironment, just as we're doing for DebuggerObject now. And it's nice to move things like JSSLOT_DEBUGENV_OWNER and DebuggerEnv_class into the class. But those changes can come later.

Comment on attachment 8756360 [details] [diff] [review] Implement a C++ interface for forceLexicalInitializationByName Review of attachment 8756360 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +8587,5 @@ > } > > +// Lookup a binding on the referent's global scope and change it to undefined > +// if it is an uninitialized lexical, otherwise do nothing. The method's return > +// value is true _only_ when an uninitialized lexical has been altered, otherwise Pre-existing, but: could we change this text to read, "The method's JavaScript return value is true..." @@ -8730,5 @@ > - > - if (!args[0].isString()) { > - JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, > - JSMSG_NOT_EXPECTED_TYPE, "Debugger.Object.prototype.forceLexicalInitializationByName", > - "string", InformalValueTypeName(args[0])); The patch removes this check, but I think we still need it. Its purpose is to forbid integer ids, which ValueToIdentifier will otherwise accept. But the check probably belongs behind the C++ API, in DebuggerObject::forceLexicalInitializationByName, since the `HandleId` type can be any sort of id. @@ +9152,2 @@ > { > + RootedObject referent(cx, object->referent()); nit: this would be nicer just above the globalLexical definition, after the requireGlobalObject check. Other things being equal, it's nice to produce values just as they're needed. (Similarly in the asEnvironment patch...) @@ +9155,2 @@ > if (!DebuggerObject::requireGlobalObject(cx, object)) > return false; Probably the check for integer ids belongs right here.

(In reply to Eddy Bruel [:ejpbruel] from comment #83) > (In reply to Jim Blandy :jimb from comment #81) > > I just meant, a special NativeObject subclass for Debugger.Environment > > instances, just like you've already done for Debugger.Object. The purpose of > > Debugger.Object.prototype.asEnvironment is simply to produce a > > Debugger.Environment, given a Debugger.Object whose referent is a global > > object. > > If I understand you correctly, this subclass of NativeObject would (at least > for the moment) not have any methods or other members, and would just serve > as a type marker to make sure we don't use a JSObject that is not a > DebuggerEnvironment in places where the latter is expected. Is that correct? > > If so, that is easy to do, and I should be able to update the patch > accordingly tomorrow. I *just* realised that this entire time, what you were talking about is simply a NativeObject subclass for Debugger.Environment instances. For some reason, I got it into my head that DebuggerEnvironment was a separate thing, which is why I said I didn't consider a type like this before, and was unsure what this it should look like. This must have been very confusing for you; sorry for being such a pancake! :-)

(In reply to Eddy Bruel [:ejpbruel] from comment #86) > This must have been very confusing for you; sorry for being such a pancake! > :-) It was! "He *just* did one of these a few days ago..." :D

Now that I actually understand what you were talking about: let's hold off on asEnvironment until we actually have the DebuggerEnvironment type. I've obsoleted the patch for asEnvironment, and merged the parts that the patch for forceLexicalInitializationByName depends on (in particular, DebuggerObject::requireGlobalObject) with the latter. You didn't review those parts yet, so here's the updated patch. I also addressed your review comments for the previous version.

DebuggerObject_getBoundArguments returns one of three different kinds of values: 1. If the referent has one or more bound arguments, it returns a non-empty array. 2. If the referent has no bound arguments, it returns an empty array. 3. If the referent is not a bound function, it returns undefined. To create a strongly typed interface for DebuggerObject_getBoundArguments, the array to be returned should be passed as an output parameter. In particular, DebuggerObject::getBoundArguments takes a MutableHandle to a ValueVector. When using an output parameter to return an array, there isn't really a good way to distinguish between no array and an empty array. We can't use the functions return value for this, because this is already reserved for signaling success/failure. We could use a second output parameter, but this feels messy. Given the above, I instead opted to slightly change the API so that if the referent is not a bound function, we also return an empty array. Even with this change, we can still tell the difference between case 2 and 3 by using the isBoundFunction property. As far as I'm able to tell, none of the current consumers of the debugger API depend on the above distinction, so this change should not cause any regressions. Jim, let me know whether you agree with this API change or not. If not, please let me know what you prefer instead.

FYI: I had the same problem with parameterNames that I had with boundArguments before. I worked around it in a similar way; in this case, the callable property can be used to distinguish between the two cases.

(In reply to Eddy Bruel [:ejpbruel] from comment #89) > Given the above, I instead opted to slightly change the API so that if the > referent is not a bound function, we also return an empty array. Even with > this change, we can still tell the difference between case 2 and 3 by using > the isBoundFunction property. Any reason this cannot simply throw for non-bound functions? shu advised me to do that in the Promise-related accessors on DO, and it seems to make sense to me.

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a426b39dec Use EvalOptions for eval options instead of HandleObject;r=jimb

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/98a6ae087add Use mozilla::range<const char16_t> for eval code instead of HandleValue;r=jimb

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b11a57f5b70b Use HandleObject for eval bindings instead of HandleValue;r=jimb

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/11c4daf94661 Implement a C++ interface for executeInGlobal(WithBindings);r=jimb

sorry backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=29094073&repo=mozilla-inbound

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/44d2dae11890 Backed out changeset 11c4daf94661 https://hg.mozilla.org/integration/mozilla-inbound/rev/3635ade100d8 Backed out changeset b11a57f5b70b https://hg.mozilla.org/integration/mozilla-inbound/rev/0b6f1dcb1c21 Backed out changeset 98a6ae087add for spidermonkey test failures

I don't know how to reproduce these test failures locally. I've tried passing --args "--baseline-eager" (or other combinations of args for which the tests seem to fail) as an option to jit-tests.py, but that doesn't cause the tests to fail locally. Jim, can you help out here?

Needinfo'ing jimb for comment 101.

Jim, I no longer need info on comment 101, but would still like your opinion on comment 89. I could not reproduce the test failures from inbound with a local shell build. However, I can reproduce them with a shell build downloaded from taskcluster. Attaching a debugger to that gives me a stack trace that suggests we are segfaulting in error handling code invoked from ValueToStableChars. The patch that introduces ValueToStableChars copies its error handling code from elsewhere, but omits the last argument that we used to pass. I have no idea why this would cause the test to crash on inbound, and not locally, but this seems to be what's causing the problem. I'm going to re-add the omitted argument, and do a full try push for these patches to see if that fixes the issue.

Try push for the patch to use EvalOptions for eval options instead of HandleObject: https://treeherder.mozilla.org/#/jobs?repo=try&revision=094963548b48

(In reply to Eddy Bruel [:ejpbruel] from comment #89) > When using an output parameter to return an array, there isn't really a good > way to distinguish between no array and an empty array. We can't use the > functions return value for this, because this is already reserved for > signaling success/failure. We could use a second output parameter, but this > feels messy. This seems like a strange change at the JS API level. JS treats an empty array as true, so code can't call boundArguments and use a boolean check, which would be a nice idiom to support. Also, this design means that behavior given a referent that's not a bound function is the same as with a referent that is a bound function with no arguments. At the C++ level, I think it makes sense for DebuggerObject::isBoundFunction to be infallible, and simply return a bool directly, and then to make the the other C++ bound function accessors simply assert that the referent is a bound function. The JS methods should just check isBoundFunction and throw a type error if that returns false, and then call the C++ function if it is, knowing that it won't assert. There are other styles I've seen where one returns a Maybe value by reference, but I doubt Rust's bindgen can handle that. We need to keep things pretty simple.

Comment on attachment 8758205 [details] [diff] [review] Implement a C++ interface for proto and className. Review of attachment 8758205 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +1116,5 @@ > Debugger::wrapDebuggeeObject(JSContext* cx, MutableHandleObject obj) > { > + if (!obj) > + return true; > + nit: I think I would rather see a MOZ_ASSERT(obj) here, and then have the callers check for the null case. @@ +8965,5 @@ > + return false; > + } > + > + result.set(proto); > + return dbg->wrapDebuggeeObject(cx, result); That is, the null check should occur here instead.

(In reply to Eddy Bruel [:ejpbruel] from comment #103) > The patch that introduces ValueToStableChars copies its error handling code > from elsewhere, but omits the last argument that we used to pass. I have no > idea why this would cause the test to crash on inbound, and not locally, but > this seems to be what's causing the problem. Isn't that because JSMSG_NOT_EXPECTED_TYPE requires three arguments, but you only passed two? The stdarg macros are probably just grabbing either benign or non-benign garbage off the stack.

Obligatory: "Yes, C is a wonderful design, and stdargs is fine!"

Comment on attachment 8758207 [details] [diff] [review] Implement a C++ interface for name/displayName/parameterNames. Review of attachment 8758207 [details] [diff] [review]: ----------------------------------------------------------------- As with the bound function accessors, the API I'd like to see here is an infallible DebuggerObject::isFunction method returning bool, and then have methods that are inappropriate for non-functions simply MOZ_ASSERT that the referent is a function. The JS behavior should remain unchanged.

Note that `callable` and the proposed `isFunction` are not the same: `callable` includes function proxies, for which many JS accessors (parameterNames, script, ...) return undefined. At the C++ level, `isFunction` should return true on DebuggerObjects whose referents are functions, not function proxies: the exact set of DO's for which parameterNames, script, etc. return useful information.

Comment on attachment 8758208 [details] [diff] [review] Implement a C++ interface for callable/isArrowFunction. Review of attachment 8758208 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.h @@ +1052,5 @@ > HandleNativeObject debugger); > > static bool className(JSContext* cx, Handle<DebuggerObject*> object, > MutableHandleString result); > + static bool isCallable(JSContext* cx, Handle<DebuggerObject*> object, bool& result); There's no reason to make this fallible. It should simply return a boolean value. @@ +1058,5 @@ > static bool displayName(JSContext* cx, Handle<DebuggerObject*> object, > MutableHandleString result); > static bool parameterNames(JSContext* cx, Handle<DebuggerObject*> object, > MutableHandle<StringVector> result); > + static bool isArrowFunction(JSContext* cx, Handle<DebuggerObject*> object, bool &result); Similarly here.

Comment on attachment 8758209 [details] [diff] [review] Implement a C++ interface for global/allocationSite/errorMessageName. Review of attachment 8758209 [details] [diff] [review]: ----------------------------------------------------------------- This is just the callable/isArrowFunction patch again.

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a824e6ca8cbe Use mozilla::range<const char16_t> for eval code instead of HandleValue;r=jimb

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/95f7967752e0 Use HandleObject for eval bindings instead of HandleValue;r=jimb

In the change landed in comment 113: + if (!value.isString()) { + JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_NOT_EXPECTED_TYPE, + fnname, "string", "string"); + return false; + } This prints the message, "<fn>: expected string, got string", which will make someone angry if they ever see it.

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a0e8aaffd0 Quick fix for an unhelpful error message;r=me

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/229098e94645 Quick fix for an unhelpful error message;r=me

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6b013082eb56 Implement a C++ interface for executeInGlobal(WithBindings);r=jimb

New patch with comments by jimb addressed. Instead of making the functions throw if the referent is not a proper function, I optioned to leave the behavior of the JS functions unchanged, by making them return undefined instead. If that is not the behavior you want, let's file a follow up bug to make these functions throw instead.

New patch with comments by jimb addressed.

This patch just moves some functions around.

I've adopted the same pattern here, where the C++ functions assert that the referent is a promise, while the JS functions check this at runtime, and throw and error if it is not. The requirePromise function is necessary because if the referent is not a promise, we want to throw an error message containing the referent's actual class. I'd like to keep the referent private to DebuggerObject, so we need a member function for this. I also took the opportunity to rewrite forceLexicalInitialization and executeInGlobal to adopt the same pattern: here, the requireGlobal function is used by the JS functions to check if the referent is actually a global. The promise accessor functions helps to avoid some code duplication due to the fact that we have to unwrap the referent before checking whether it is a promise.

Creating a C++ interface for the promiseState property is somewhat of a pain in the neck, because it returns multiple values. One option is to create a helper class that contains both the state of the promise, as well as a permanent rooted value for its result/reason, and use that as an output parameter. Another option is to split this property up so that promiseState just returns the state, and promiseResult/promiseReason return the result/reason, respectively. In the latter case, the C++ functions for promiseResult/promiseReason would assert that promiseState has the expected value, while the JS functions check this at runtime, and throw and error if they do not. Till, what approach do you prefer here? I'm personally leaning towards the latter.

Comment on attachment 8758667 [details] [diff] [review] Implement a C++ interface for isPromise/promiseLifetime. Review of attachment 8758667 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. I'd split the global-related changes out into their own patch, though. ::: js/src/vm/Debugger.cpp @@ +8185,5 @@ > > + if (!DebuggerObject::requirePromise(cx, object)) > + return false; > + > + args.rval().setNumber(DebuggerObject::promiseLifetime(cx, object)); I assume you'll apply this change to all the other promise-related functions, too? @@ +9380,5 @@ > RootedShape shape(cx); > if (!LookupProperty(cx, globalLexical, id, &pobj, &shape)) > return false; > > + result = false; Wrong indentation

(In reply to Eddy Bruel [:ejpbruel] from comment #124) > Another option is to split this property up so that promiseState just > returns the state, and promiseResult/promiseReason return the result/reason, > respectively. In the latter case, the C++ functions for > promiseResult/promiseReason would assert that promiseState has the expected > value, while the JS functions check this at runtime, and throw and error if > they do not. I'm ok with this approach. There's no real need to keep things the way they are now: that was mostly done to stay similar to what PromiseDebugging did, but it's not important.

https://hg.mozilla.org/mozilla-central/rev/d4a426b39dec https://hg.mozilla.org/mozilla-central/rev/a824e6ca8cbe https://hg.mozilla.org/mozilla-central/rev/95f7967752e0

(In reply to Eddy Bruel [:ejpbruel] from comment #119) > Instead of making the functions throw if the referent is not a proper > function, I optioned to leave the behavior of the JS functions unchanged, by > making them return undefined instead. Yes, that's perfect.

Comment on attachment 8758659 [details] [diff] [review] Implement a C++ interface for the bound function properties. Review of attachment 8758659 [details] [diff] [review]: ----------------------------------------------------------------- Found some mistakes that need fixed, but otherwise looks great. ::: js/src/jit-test/tests/debug/Object-boundTargetFunction-02.js @@ +14,5 @@ > > var nw = gw.executeInGlobal("var n = Math.max; n").return; > assertEq(nw.isBoundFunction, false); > assertEq(nw.boundThis, undefined); > +assertEq(fw.boundArguments, undefined); This looks like search-and-replace damage. Why are we checking fw here, instead of nw? @@ +20,5 @@ > > var ow = gw.executeInGlobal("var o = {}; o").return; > assertEq(ow.isBoundFunction, false); > assertEq(ow.boundThis, undefined); > +assertEq(fw.boundArguments, undefined); Similarly. ::: js/src/vm/Debugger.cpp @@ +8083,5 @@ > > static bool > DebuggerObject_getBoundTargetFunction(JSContext* cx, unsigned argc, Value* vp) > { > + THIS_DEBUGOBJECT(cx, argc, vp, "get boundTargetFunction", args, object) Nice catch.

https://hg.mozilla.org/mozilla-central/rev/a8a0e8aaffd0 https://hg.mozilla.org/mozilla-central/rev/229098e94645 https://hg.mozilla.org/mozilla-central/rev/6b013082eb56

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/153b81041c1e Implement a C++ interface for forceLexicalInitializationByName;r=jimb

Comment on attachment 8758667 [details] [diff] [review] Implement a C++ interface for isPromise/promiseLifetime. As it turns out, I cannot test these patches, because the promise debugger tests don't pass even without my patches applied. Till gave me a patch that should make the promise debugger tests pass, but it doesn't work. Given that this is the case, and since I'd still like to land this bug before the next release, let's cancel this patch, and file a followup bug for the promise properties instead. This seems reasonable, since SpiderMonkey promises are not yet enabled by default.

I moved these changes out of the patch I obsoleted earlier, because they are not related to spidermonkey promises. I adopted the same pattern to forceLexicalInitializationByName/executeInGlobal(WithBindings) as we did for the other functions. Namely, the C++ functions should assert, while the native JS functions check at runtime that the referent is actually a global.

(In reply to Eddy Bruel [:ejpbruel] from comment #133) > I adopted the same pattern to > forceLexicalInitializationByName/executeInGlobal(WithBindings) as we did for > the other functions. Namely, the C++ functions should assert, while the > native JS functions check at runtime that the referent is actually a global. If we were going to push static checking as far as possible, we would actually subclass js::DebuggerObject for different types of referent, and then move those methods into the subclasses. I can't quite decide whether I think that's worth it or not, but I definitely don't think it needs to be part of this bug.

Comment on attachment 8758660 [details] [diff] [review] Implement a C++ interface for name/displayName/parameterNames. Review of attachment 8758660 [details] [diff] [review]: ----------------------------------------------------------------- The Debugger.Object accessors are supposed to be very circumspect about handing out information about non-debuggee functions. name and displayName are fine, but pretty much all other information is supposed to be off-limits. Right now, some of the accessors do check that dbg->observesGlobal(&obj->global()), but the bound function accessors don't bother. This is a bug. Filed as bug 1277699. I think what we want at the C++ level is separate predicates, isFunction and isDebuggeeFunction, the latter being what we should be using on all accessors other than name and displayName. Fixing this will avoid the change to behavior of the parameterNames accessor in Object-script-lazy.js. ::: js/src/vm/Debugger.cpp @@ +8884,5 @@ > + RootedFunction fun(cx, &referent->as<JSFunction>()); > + > + /* Only hand out parameter info for debuggee functions. */ > + if (!dbg->observesGlobal(&fun->global())) > + return true; So, this check should move into the new isDebuggeeFunction predicate, and this method should MOZ_ASSERT(isDebuggeeFunction()).

The C++ interface for name/displayName is good as is, so I've separated those out into a separate patch.

This patch essentially implements bug 1277699. Note that I still have to take care of isArrowFunction, because that property doesn't have a C++ interface yet. I wrote a patch for that earlier, but somehow it got removed from my queue during rebasing, so it never landed. When I give isArrowFunction a C++ interface in a later patch, I will also make sure that it asserts that the referent is a debugger function.

I'd like us to adopt the same pattern we use for the bound function properties, for the methods that operate on a global environment. That is, they should assert that the referent is actually a global object. The JS function should check this at runtime, and if the check fails, return undefined without doing anything.

Here is parameterNames again, with the behavior you wanted. I also take care of isArrowFunction in this patch.

The remainder of these are just rebases of the earlier patches.

Comment on attachment 8759652 [details] [diff] [review] Bound function properties should assert that referent is debuggee function. Review of attachment 8759652 [details] [diff] [review]: ----------------------------------------------------------------- Great tests. ::: js/src/jit-test/tests/debug/Object-boundTargetFunction-02.js @@ +18,5 @@ > assertEq(fw.boundArguments, undefined); > assertEq(nw.boundTargetFunction, undefined); > > var ow = gw.executeInGlobal("var o = {}; o").return; > +assertEq(ow.isBoundFunction, undefined); This change is good, but it needs to be documented in js/src/doc/Debugger/Debugger.Object.md. ::: js/src/vm/Debugger.cpp @@ +8913,5 @@ > > /* static */ bool > DebuggerObject::isBoundFunction(JSContext* cx, Handle<DebuggerObject*> object) > { > + MOZ_ASSERT(DebuggerObject::isDebuggeeFunction(cx, object)); Since we're in a DebuggerObject method, I think you can simply write: MOZ_ASSERT(isDebuggeeFunction(cx, object)); even though it's a static method. But I'm not sure...

Comment on attachment 8759655 [details] [diff] [review] Implement a C++ interface for isCallable/isArrowFunction/parameterNames. Review of attachment 8759655 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/debug/Object-script-lazy.js @@ +42,5 @@ > var hDO = g2w.getOwnPropertyDescriptor('h').value; > assertEq(hDO.global, g2w); > assertEq(hDO.unwrap().global === g2w, false); > assertEq(hDO.unwrap().isBoundFunction, undefined); > +assertEq(hDO.unwrap().isArrowFunction, undefined); This behavior makes sense, but it needs to be documented. ::: js/src/vm/Debugger.cpp @@ +7964,5 @@ > + for (size_t i = 0, len = names.length(); i < len; i++) { > + if (names[i]) > + vals[i].setString(names[i]); > + else > + vals[i].setUndefined(); If I'm reading right, the sole purpose of ValueVector is to convert JSString * values to either StringValues or Undefined. Could we have DebuggerObject_getParameterNames just keep the original code that calls NewDenseFullyAllocatedArray and ensureDenseInitializedLength, and then uses setDenseElement to populate it directly? I don't think it matters for efficiency; it just seems less circuitous.

Comment on attachment 8759659 [details] [diff] [review] Implement a C++ interface for global/allocationSite/errorMessageName. Review of attachment 8759659 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.h @@ +1058,5 @@ > static bool isArrowFunction(JSContext* cx, Handle<DebuggerObject*> object); > static bool isGlobal(JSContext* cx, Handle<DebuggerObject*> object); > static bool className(JSContext* cx, Handle<DebuggerObject*> object, > MutableHandleString result); > + static bool global(JSContext* cx, Handle<DebuggerObject*> object, MutableHandleObject result); Would it be possible to use MutableHandle<GlobalObject*> result here? @@ +1072,5 @@ > MutableHandleValue result); > static bool boundArguments(JSContext* cx, Handle<DebuggerObject*> object, > MutableHandle<ValueVector> result); > + static bool allocationSite(JSContext* cx, Handle<DebuggerObject*> object, > + MutableHandleObject result); Would it be possible to use MutableHandle<SavedFrame*> result here?

Comment on attachment 8759660 [details] [diff] [review] Move properties into DebuggerObject. Review of attachment 8759660 [details] [diff] [review]: ----------------------------------------------------------------- This patch includes a bunch of code motion, and I can't compare the before and after effectively. Please leave the code where it was whenever possible. ::: js/src/vm/Debugger.cpp @@ +7873,2 @@ > > + args.rval().setBoolean(DebuggerObject::isCallable(cx, object)); I'm pretty sure you can omit DebuggerObject:: here, and elsewhere. That's one of the nice things about moving it into the class.

(In reply to Jim Blandy :jimb from comment #149) > Would it be possible to use MutableHandle<SavedFrame*> result here? Nick says there's a typedef for this, and another for MutableHandle<GlobalObject*>.

Comment on attachment 8759661 [details] [diff] [review] Move methods into DebuggerObject. Review of attachment 8759661 [details] [diff] [review]: ----------------------------------------------------------------- This one is much better. Thanks for avoiding code motion. ::: js/src/vm/Debugger.cpp @@ +8320,5 @@ > { > THIS_DEBUGOBJECT(cx, argc, vp, "isExtensible", args, object) > > bool result; > if (!DebuggerObject::isExtensible(cx, object, result)) I'm pretty sure you can omit DebuggerObject:: here and elsewhere. @@ +8750,5 @@ > }; > #endif // SPIDERMONKEY_PROMISE > > const JSFunctionSpec DebuggerObject::methods_[] = { > + JS_FN("isExtensible", DebuggerObject::isExtensibleMethod, 0, 0), And all the DebuggerObject:: prefixes can disappear here, which is awesome.

Just a reminder: this bug needs to stay open until the various doc changes requested have been landed.

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b11548673318 Implement a C++ function for makeDebuggeeValue/unsafeDereference/unwrap;r=jimb

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/368484809241 Implement a C++ interface for the bound function properties;r=jimb

Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9d4e3c2f6a90 Implement a C++ interface for proto and className;r=jimb