729576 - Update the remote debugging protocol to reflect the recent spec changes

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

The spec changes published on Feb 17, 2012 should be reflected on the debugger implementation. See the History tab in https://wiki.mozilla.org/Remote_Debugging_Protocol

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP

Almost done. Waiting on a couple of answers from jimb and I also need to fix the broken tests.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP 2

Fixed the tests, now waiting for jimb's clarifications.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP 3

Fixed the environment bindings grip, still waiting on jimb.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Working patch

I believe I've got all of the changes covered now.

Comment 5

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 602283 [details] [diff] [review]
Working patch

this still leaves 4 XXX comments in the code. Still a few todos left?

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Rob Campbell [:rc] (robcee) from comment #5)
> Comment on attachment 602283 [details] [diff] [review]
> Working patch
> 
> this still leaves 4 XXX comments in the code. Still a few todos left?

Yes, I was hoping Dave could take a look at those 4 and correct me if I'm wrong.

Comment 7

[Dave Camp (:dcamp)]

Comment on attachment 602283 [details] [diff] [review]
Working patch

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

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +645,5 @@
>    /**
> +   * Create a grip for the given completion value, calling createValueGrip
> +   * as needed.
> +   */
> +  createCompletionGrip: function TA_createCompletionGrip(aCompletion) {

This isn't really a grip, since it doesn't retain a server-side object.  I don't have a solid suggestion for what to call "json that describes something but isn't a grip" and I think I've misused grip for that before.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Dave Camp (:dcamp) from comment #7)
> Comment on attachment 602283 [details] [diff] [review]
> Working patch
> 
> Review of attachment 602283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +645,5 @@
> >    /**
> > +   * Create a grip for the given completion value, calling createValueGrip
> > +   * as needed.
> > +   */
> > +  createCompletionGrip: function TA_createCompletionGrip(aCompletion) {
> 
> This isn't really a grip, since it doesn't retain a server-side object.  I
> don't have a solid suggestion for what to call "json that describes
> something but isn't a grip" and I think I've misused grip for that before.

You mean that it doesn't correspond to a separate actor? Because we currently use this method to retrieve the "grip" to pass down the protocol connection from the completion value returned from frame.eval. The completion value coming from eval contains debuggee values and the completion value we have to transmit to the client has to contain grips on those values:

https://wiki.mozilla.org/Remote_Debugging_Protocol#Completion_Values
https://wiki.mozilla.org/Debugger#Completion_Values

Would calling it createCompletionValue be better?

Comment 9

[Jim Blandy :jimb]

(In reply to Dave Camp (:dcamp) from comment #7)
> Comment on attachment 602283 [details] [diff] [review]
> Working patch
> 
> Review of attachment 602283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +645,5 @@
> >    /**
> > +   * Create a grip for the given completion value, calling createValueGrip
> > +   * as needed.
> > +   */
> > +  createCompletionGrip: function TA_createCompletionGrip(aCompletion) {
> 
> This isn't really a grip, since it doesn't retain a server-side object.  I
> don't have a solid suggestion for what to call "json that describes
> something but isn't a grip" and I think I've misused grip for that before.

So this is a function that takes a Debugger-provided "completion value", and produces a protocol "completion value", right?  I think you do want to use the term "completion value", but perhaps with a qualifier: "protocol completion value". As in:

/* Return a protocol completion value representing the given Debugger-provided completion value. */

Comment 10

[Jim Blandy :jimb]

It does seem like those two concepts should share a name: they're representing the same thing. That's why I suggest the "protocol" or "Debugger" qualifier, which can be omitted in contexts where it's not ambiguous.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

createProtocolCompletionValue it is then.

Comment 12

[Jim Blandy :jimb]

Comment on attachment 602283 [details] [diff] [review]
Working patch

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

This looks good, except for the 'arguments' array bit.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +991,3 @@
>  
> +    return { name: this.obj.name || null,
> +             scope: envActor.grip() };

Similarly, this isn't a "grip", although it could contain grips.

I think the term "grip" should be restricted to the meaning given to it by the protocol docs.

Should we introduce a term, like "form", for these JSON-ish representations of things that get included in protocol packets? This is sort of a reference to the way the protocol says, "blah has the form: X"

Then we could talk about "environment forms" when we mean the { "type":"object", "object":<grip> } things, and just "environments" when we mean the Debugger.Environment instances or the underlying JS environments those represent.

Similarly, we have "frame forms", like { "actor":a, "depth":2, "type":"global" ... }, as distinct from "frames".

@@ +1280,5 @@
>     * Return the identifier bindings object as required by the remote protocol
>     * specification.
>     */
>    _bindings: function EA_bindings() {
> +    let bindings = { arguments: {}, variables: {} };

Note that arguments is an *array* of { name:descriptor } pairs; it doesn't have the same shape as "variables". This is because 1) JavaScript functions can have repeated parameter names, and 2) the order of the parameters matters.

Of course, when you do have repeated parameter names, you can only actually refer to one of the values (the last). So the Debugger API doesn't let you get at descriptors by index, only by name; it doesn't seem worth it to support this case well, beyond not crashing.

But the parameter ordering issue is more important.

@@ +1289,5 @@
>      }
>  
> +    for (let name in this.obj.parameterNames) {
> +      let desc = this.obj.environment.getVariableDescriptor(name);
> +      let descGrip = {

Would "descForm" work here?

@@ +1307,2 @@
>      for (let name in this.obj.environment.names()) {
> +      if (name in grip.bindings.arguments) {

It would be kind of nice if Debugger.Environment would just give you a list of the locals separately from the arguments...

::: toolkit/devtools/debugger/tests/unit/test_eval-01.js
@@ +33,5 @@
>        gThreadClient.addOneTimeListener("paused", function(aEvent, aPacket) {
>          // Check the return value...
>          do_check_eq(aPacket.type, "paused");
>          do_check_eq(aPacket.why.type, "clientEvaluated");
> +        do_check_eq(aPacket.why.frameFinished["return"].type, "object");

I'm pretty sure JavaScript lets you use reserved words after a '.'. So you can just say aPacket.why.frameFinished.return.type. Seems like there were other uses of ["..."] too.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Working patch v2

(In reply to Jim Blandy :jimb from comment #12)
> Comment on attachment 602283 [details] [diff] [review]
> Working patch
> 
> Review of attachment 602283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, except for the 'arguments' array bit.

Ugh, I can't believe I missed this.

> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +991,3 @@
> >  
> > +    return { name: this.obj.name || null,
> > +             scope: envActor.grip() };
> 
> Similarly, this isn't a "grip", although it could contain grips.
> 
> I think the term "grip" should be restricted to the meaning given to it by
> the protocol docs.
> 
> Should we introduce a term, like "form", for these JSON-ish representations
> of things that get included in protocol packets? This is sort of a reference
> to the way the protocol says, "blah has the form: X"
> 
> Then we could talk about "environment forms" when we mean the {
> "type":"object", "object":<grip> } things, and just "environments" when we
> mean the Debugger.Environment instances or the underlying JS environments
> those represent.
> 
> Similarly, we have "frame forms", like { "actor":a, "depth":2,
> "type":"global" ... }, as distinct from "frames".

Sounds good to me. I've changed all references of "grip" to "form" for environments and frames.

> @@ +1280,5 @@
> >     * Return the identifier bindings object as required by the remote protocol
> >     * specification.
> >     */
> >    _bindings: function EA_bindings() {
> > +    let bindings = { arguments: {}, variables: {} };
> 
> Note that arguments is an *array* of { name:descriptor } pairs; it doesn't
> have the same shape as "variables". This is because 1) JavaScript functions
> can have repeated parameter names, and 2) the order of the parameters
> matters.
> 
> Of course, when you do have repeated parameter names, you can only actually
> refer to one of the values (the last). So the Debugger API doesn't let you
> get at descriptors by index, only by name; it doesn't seem worth it to
> support this case well, beyond not crashing.
> 
> But the parameter ordering issue is more important.

I've fixed this.

> @@ +1289,5 @@
> >      }
> >  
> > +    for (let name in this.obj.parameterNames) {
> > +      let desc = this.obj.environment.getVariableDescriptor(name);
> > +      let descGrip = {
> 
> Would "descForm" work here?

Sure.

> @@ +1307,2 @@
> >      for (let name in this.obj.environment.names()) {
> > +      if (name in grip.bindings.arguments) {
> 
> It would be kind of nice if Debugger.Environment would just give you a list
> of the locals separately from the arguments...
> 
> ::: toolkit/devtools/debugger/tests/unit/test_eval-01.js
> @@ +33,5 @@
> >        gThreadClient.addOneTimeListener("paused", function(aEvent, aPacket) {
> >          // Check the return value...
> >          do_check_eq(aPacket.type, "paused");
> >          do_check_eq(aPacket.why.type, "clientEvaluated");
> > +        do_check_eq(aPacket.why.frameFinished["return"].type, "object");
> 
> I'm pretty sure JavaScript lets you use reserved words after a '.'. So you
> can just say aPacket.why.frameFinished.return.type. Seems like there were
> other uses of ["..."] too.

That's true. I went ahead and changed all instances of this pattern that I could spot.

Comment 14

[Jim Blandy :jimb]

Comment on attachment 603685 [details] [diff] [review]
Working patch v2

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

Thanks!

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/cc44d26d718b

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/cc44d26d718b