1252958 - Extend the Debugger API with a function to adopt a Debugger.Object instance from another Debugger.

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Eddy Bruel [:ejpbruel]]

As per our discussion on the mailing list, we're going to extend the interface of the debugger API so that the webconsole no longer needs to rely on unsafeDereference to implement the functionality it needs.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Proposed solution (WIP)

We want to be able to take a wrapped debuggee value that is owned by one debugger, and rewrap it as a debuggee value that is owned by another debugger.

It seems that the following code would do the trick, with one snag: unwrapDebuggeeObject (and therefore unwrapDebuggeeValue), does not allow you to unwrap a Debugger.Object instance that is owned by a different debugger. But that's exactly what we're trying to do here!

Question for jimb: Would it be ok if we refactored this code to allow unwrapping a Debugger.Object instance that is owned by a different debugger, but only for this particular use case? (we could pass this as a boolean option to unwrapDebuggeeValue, for instance).

Comment 2

[Jim Blandy :jimb]

I think what we want here is not a boolean flag, but to pull out the checks into their own function:

- Is this indeed a Debugger.Object instance?

- Does it have an owner set? (This distinguishes real Debugger.Object instances from Debugger.Object.prototype.)

Then, Debugger::unwrapDebuggeeObject can call that checker function, and then proceed with its job:

- Does this Debugger.Object have the same owner as us?

- All good, let's get the referent.

And the new method can call the checker function, and then just fetch the referent.

The 'fetch the referent' code gets duplicated, but it's a single function call and a cast, so that's negligible.

Comment 3

[Jim Blandy :jimb]

Comment on attachment 8732141 [details] [diff] [review]
Proposed solution (WIP)

Review of attachment 8732141 [details] [diff] [review]:
-----------------------------------------------------------------

Hopefully comment 2 provides the requested feedback on this patch.

But yes, something as simple as this should be all that's needed. Obviously, it needs tests and docs.

Comment 4

[Eddy Bruel [:ejpbruel]]

Attachment: Implement Debugger.rewrapDebuggeeValue

This should do the trick. Let me know if the tests and docs are adequate.

Comment 5

[Jim Blandy :jimb]

Comment on attachment 8733273 [details] [diff] [review]
Implement Debugger.rewrapDebuggeeValue

Review of attachment 8733273 [details] [diff] [review]:
-----------------------------------------------------------------

Throughout the API and documentation, we are careful to use terms like "wrapper", "wrapped", "unwrapped", and so on to only refer to cross-compartment wrappers as used by ordinary non-debugger code, never to Debugger.Object and the like, as you're using it here. Let's preserve that distinction. The function should be renamed (suggest adoptDebuggeeValue?), and the terms used in the documentation should be changed.

I think adoptDebuggeeValue (or whatever) should throw if its argument is an object, but not a Debuggee.Object. This is what the code does now, but it's not what the docs say. The tests should cover this behavior.

Otherwise, this looks good. Since I asked for so many changes, I'd like to re-review.

::: js/src/doc/Debugger/Debugger.md
@@ +497,5 @@
>      `TypeError`. Determine which global is designated by <i>global</i>
>      using the same rules as [`Debugger.prototype.addDebuggee`][add].
>  
> +<code>rewrapDebuggeeValue(<i>value</i>)</code>
> +:   Rewrap the given wrapped debuggee `value`. If `value` was a Debugger.Object

We can't use the term 'wrap' this way, and the treatment of non-debuggee values described here doesn't match the code. Perhaps:

Given a debuggee value `value` belonging to any `Debugger`, return an equivalent debuggee value belonging to this `Debugger`. If `value` is a primitive value, return it unchanged. If `value` is a `Debugger.Object` belonging to any `Debugger`, return a `Debugger.Object` belonging to this `Debugger` with the same referent. Otherwise, `value` is some other kind of object, and hence not a proper debuggee value; throw a TypeError.

::: js/src/jit-test/tests/debug/Debugger-rewrapDebuggeeValue.js
@@ +10,5 @@
> +  obj1 = args[0];
> +};
> +var dbg2 = Debugger(g);
> +var obj2 = null;
> +dbg2.onDebuggerStatement = function (frame) {

I made some comments on bug 1252945 comment 9 suggesting simpler ways to write these tests. Those comments apply here too.

::: js/src/vm/Debugger.cpp
@@ +1078,5 @@
>      assertSameCompartment(cx, object.get(), vp);
>      if (vp.isObject()) {
>          RootedObject dobj(cx, &vp.toObject());
>          if (!unwrapDebuggeeObject(cx, &dobj))
> +           return false;

Stray (incorrect) change to indentation here?

@@ +4853,5 @@
> +Debugger::rewrapDebuggeeValue(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    THIS_DEBUGGER(cx, argc, vp, "rewrapDebuggeeValue", args, dbg);
> +
> +    RootedValue v(cx, args[0]);

I think this will assert if there are no arguments:

https://hg.mozilla.org/mozilla-central/file/fda54bfb2896/js/public/CallArgs.h#l296

I think you want to call args.requireAtLeast immediately after THIS_DEBUGGER, as shown here:

https://hg.mozilla.org/mozilla-central/file/fda54bfb2896/js/src/vm/Debugger.cpp#l2923

@@ +4858,5 @@
> +    if (v.isObject()) {
> +        RootedObject obj(cx, &v.toObject());
> +        NativeObject* ndobj = ToNativeDebuggerObject(cx, &obj);
> +        if (!ndobj) {
> +            return false;

The documentation says that it returns anything but a Debugger.Object unchanged. That's not what this code does; as I said above, I think the docs are wrong.

Comment 6

[Eddy Bruel [:ejpbruel]]

Attachment: Implement Debugger.adoptDebuggeeValue

New patch with comments by jimb addressed.

Comment 7

[Jim Blandy :jimb]

Comment on attachment 8736806 [details] [diff] [review]
Implement Debugger.adoptDebuggeeValue

Review of attachment 8736806 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, with these comments addressed.

::: js/src/jit-test/tests/debug/Debugger-adoptDebuggeeValue.js
@@ +7,5 @@
> +var obj1 = null;
> +dbg1.onDebuggerStatement = function (frame) {
> +  var args = frame.arguments;
> +  obj1 = args[0];
> +};

All this, and the g.eval calls below, are just a really circuitous way to write:

obj1 = dbg1.executeInGlobal('({})').return;

Please use this simpler form, as I requested in the first review.

::: js/src/vm/Debugger.cpp
@@ +4780,5 @@
> +    }
> +
> +    if (!dbg->wrapDebuggeeValue(cx, &v)) {
> +        return false;
> +    }

This belongs inside the `if` clause. As it stands, I don't think it's incorrect, but it feels strange. If we didn't take the "then" path, then `v` is not a value from a debuggee. It's only after this statement: 

        v = ObjectValue(*obj);

that we know that `v` is a value from a debuggee. And it's weird to pass values that aren't from a debuggee to wrapDebuggeeValue.

Comment 8

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #7)
> ::: js/src/jit-test/tests/debug/Debugger-adoptDebuggeeValue.js
> @@ +7,5 @@
> > +var obj1 = null;
> > +dbg1.onDebuggerStatement = function (frame) {
> > +  var args = frame.arguments;
> > +  obj1 = args[0];
> > +};
> 
> All this, and the g.eval calls below, are just a really circuitous way to
> write:
> 
> obj1 = dbg1.executeInGlobal('({})').return;

Sorry --- executeInGlobal is a method of the global's DO, not the Debugger instance itself. So that should have been:

var dbg1 = new Debugger;
var gDO1 = dbg1.addDebuggee(g);
obj1 = gDO1.executeInGlobal('({})').return;

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6001a9e0ec92

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6001a9e0ec92

Comment 11

[Brian Grinstead [:bgrins]]

Eddy, I don't see this in the docs here: https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.  Do they need to get published?

Comment 12

[Jim Blandy :jimb]

Thanks for pointing this out, Brian. I have to manually run the scripts to update MDN. I've done so now, and the documentation for adoptDebuggeeValue should be there.

Comment 13

[Brian Grinstead [:bgrins]]

Alright, thanks Jim

Comment 14

[Eddy Bruel [:ejpbruel]]

(In reply to Jim Blandy :jimb from comment #12)
> Thanks for pointing this out, Brian. I have to manually run the scripts to
> update MDN. I've done so now, and the documentation for adoptDebuggeeValue
> should be there.

For future reference, can I run these scripts myself whenever I land some changes to the docs? (so you don't have to?)

Comment 15

[Jim Blandy :jimb]

Sure, you need to get an MDN keyid and secret as described in js/src/doc/README.md, and then run publish.sh according to the instructions there. I keep my keyid and secret in a script like the below that I keep in a bin directory with other Mozilla-related utilities:

#!/usr/bin/env bash

if ! [ -x ./publish.sh ]; then
    echo "Run in js/src/doc in the source tree." >&1
    exit 1
fi

KEYID=...
SECRET=...

./publish.sh Debugger formatted-Debugger "$KEYID" "$SECRET"
./publish.sh SavedFrame formatted-SavedFrame "$KEYID" "$SECRET"