724228 - Use getVariable instead of getVariableDescriptor until the latter lands

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

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

Attachment: Workaround

Bug 692984 is coming along and apparently it will only provide getVariable, not getVariableDescriptor for now. We should use that for now, since it will (partially) provide one of our major missing bits: the variables in the stack frame environment.

In this patch I'm putting all variables in the mutable bucket, but I don't think it would be any worse if I chose the immutable one.

Comment 1

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

Attachment: Workaround v2

Rebased on top of bug 729576 and simplified a bit.

Comment 2

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

Attachment: Workaround v3

Rebased on top of bug 692984 and fixed variable display.

Comment 3

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

Attachment: Workaround v4

Discovered a couple of issues that I still need to address fully. Storing my WIP for safekeeping.

Comment 4

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

Attachment: Patch v5

Rebased, fixed the protocol part (now that I can test things), improved the performance of the frontend part. Need to investigate some more corner cases in the protocol and add tests.

Comment 5

[Jim Blandy :jimb]

Yeah, stack frames and environments have a slightly subtle relationship. Let me look at the spec.

Comment 6

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

Attachment: Patch v6

More debug logging.

Comment 7

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

Attachment: Patch v7

This fixes all the cases I could come up with. There is a short-term change in the creation of an environment actor's protocol form, where I'm currently supplying the stack frame this environment belong to. Long-term there will be a Debugger.Environment.prototype.callee property that I could be using, as discussed with Jim.

Comment 8

[Dave Camp (:dcamp)]

Comment on attachment 617063 [details] [diff] [review]
Patch v7

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

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +1368,5 @@
>     * Returns an environment form for use in a protocol message.
> +   *
> +   * @param object aObject
> +   *        The stack frame or function object whose environment bindings are
> +   *        being generated.

This param is temporary, right?  Might want to explain that in the comment.

@@ +1412,5 @@
>     * Return the identifier bindings object as required by the remote protocol
>     * specification.
> +   *
> +   * @param object aObject [optional]
> +   *        The stack frame or function object whose environment bindings are

... same here.

@@ +1498,5 @@
>     * @param aRequest object
>     *        The protocol request object.
>     */
>    onAssign: function EA_onAssign(aRequest) {
> +    let desc = this.obj.getVariableDescriptor(aRequest.name);

How does this work?  With a good answer to that I can r+

Comment 9

[Mozilla RelEng Bot]

Try run for 3837d6df4fb9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3837d6df4fb9
Results (out of 219 total builds):
    exception: 1
    success: 185
    warnings: 33
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3837d6df4fb9

Comment 10

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

Attachment: Patch v8

(In reply to Dave Camp (:dcamp) from comment #8)
> Comment on attachment 617063 [details] [diff] [review]
> Patch v7
> 
> Review of attachment 617063 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +1368,5 @@
> >     * Returns an environment form for use in a protocol message.
> > +   *
> > +   * @param object aObject
> > +   *        The stack frame or function object whose environment bindings are
> > +   *        being generated.
> 
> This param is temporary, right?  Might want to explain that in the comment.

Sure, I added a comment with a reference to bug 747514.

> @@ +1412,5 @@
> >     * Return the identifier bindings object as required by the remote protocol
> >     * specification.
> > +   *
> > +   * @param object aObject [optional]
> > +   *        The stack frame or function object whose environment bindings are
> 
> ... same here.

Done.

> @@ +1498,5 @@
> >     * @param aRequest object
> >     *        The protocol request object.
> >     */
> >    onAssign: function EA_onAssign(aRequest) {
> > +    let desc = this.obj.getVariableDescriptor(aRequest.name);
> 
> How does this work?  With a good answer to that I can r+

Well, it doesn't :-)
I only had to touch this code because of the refactoring that made the environment actor keep a reference to the environment, instead of the frame or function. We will get the 'assign' protocol operation to work in bug 724862.

Comment 11

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

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

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 13

[(no longer active)]

This patch was in a range which caused a Ts regression, so I backed out the whole range:

https://hg.mozilla.org/mozilla-central/rev/24a6a53c714a

Please reland after investigating and fixing the regression.

Comment 14

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

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

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

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