Update the remote debugging protocol to reflect the recent spec changes

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: past, Assigned: past)

Tracking

Trunk
Firefox 13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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
Created attachment 599648 [details] [diff] [review]
WIP

Almost done. Waiting on a couple of answers from jimb and I also need to fix the broken tests.
Created attachment 599928 [details] [diff] [review]
WIP 2

Fixed the tests, now waiting for jimb's clarifications.
Attachment #599648 - Attachment is obsolete: true
Blocks: 711164
Created attachment 601310 [details] [diff] [review]
WIP 3

Fixed the environment bindings grip, still waiting on jimb.
Attachment #599928 - Attachment is obsolete: true
Created attachment 602283 [details] [diff] [review]
Working patch

I believe I've got all of the changes covered now.
Attachment #601310 - Attachment is obsolete: true
Attachment #602283 - Flags: review?(jimb)
Attachment #602283 - Flags: review?(dcamp)
Comment on attachment 602283 [details] [diff] [review]
Working patch

this still leaves 4 XXX comments in the code. Still a few todos left?
(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

5 years ago
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.
Attachment #602283 - Flags: review?(dcamp) → review+
(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

5 years ago
(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

5 years ago
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.
createProtocolCompletionValue it is then.

Comment 12

5 years ago
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.
Attachment #602283 - Flags: review?(jimb)
Created attachment 603685 [details] [diff] [review]
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.
Attachment #602283 - Attachment is obsolete: true
Attachment #603685 - Flags: review?(jimb)

Comment 14

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

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

Thanks!
Attachment #603685 - Flags: review?(jimb) → review+
https://hg.mozilla.org/integration/fx-team/rev/cc44d26d718b
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/cc44d26d718b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.