Last Comment Bug 724228 - Use getVariable instead of getVariableDescriptor until the latter lands
: Use getVariable instead of getVariableDescriptor until the latter lands
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: Firefox 15
Assigned To: Panos Astithas [:past]
:
Mentors:
Depends on:
Blocks: minotaur
  Show dependency treegraph
 
Reported: 2012-02-04 03:21 PST by Panos Astithas [:past]
Modified: 2012-05-04 07:26 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Workaround (1.72 KB, patch)
2012-02-04 03:21 PST, Panos Astithas [:past]
no flags Details | Diff | Review
Workaround v2 (2.59 KB, patch)
2012-02-28 10:03 PST, Panos Astithas [:past]
no flags Details | Diff | Review
Workaround v3 (5.22 KB, patch)
2012-03-21 08:51 PDT, Panos Astithas [:past]
no flags Details | Diff | Review
Workaround v4 (6.58 KB, patch)
2012-04-12 14:55 PDT, Panos Astithas [:past]
no flags Details | Diff | Review
Patch v5 (16.48 KB, patch)
2012-04-19 11:11 PDT, Panos Astithas [:past]
no flags Details | Diff | Review
Patch v6 (16.94 KB, patch)
2012-04-20 06:25 PDT, Panos Astithas [:past]
no flags Details | Diff | Review
Patch v7 (27.20 KB, patch)
2012-04-20 12:23 PDT, Panos Astithas [:past]
dcamp: feedback+
Details | Diff | Review
Patch v8 (27.68 KB, patch)
2012-04-21 03:21 PDT, Panos Astithas [:past]
dcamp: review+
Details | Diff | Review

Description Panos Astithas [:past] 2012-02-04 03:21:53 PST
Created attachment 594416 [details] [diff] [review]
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 [:past] 2012-02-28 10:03:29 PST
Created attachment 601314 [details] [diff] [review]
Workaround v2

Rebased on top of bug 729576 and simplified a bit.
Comment 2 Panos Astithas [:past] 2012-03-21 08:51:02 PDT
Created attachment 607978 [details] [diff] [review]
Workaround v3

Rebased on top of bug 692984 and fixed variable display.
Comment 3 Panos Astithas [:past] 2012-04-12 14:55:04 PDT
Created attachment 614576 [details] [diff] [review]
Workaround v4

Discovered a couple of issues that I still need to address fully. Storing my WIP for safekeeping.
Comment 4 Panos Astithas [:past] 2012-04-19 11:11:31 PDT
Created attachment 616655 [details] [diff] [review]
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 2012-04-19 15:11:27 PDT
Yeah, stack frames and environments have a slightly subtle relationship. Let me look at the spec.
Comment 6 Panos Astithas [:past] 2012-04-20 06:25:45 PDT
Created attachment 616953 [details] [diff] [review]
Patch v6

More debug logging.
Comment 7 Panos Astithas [:past] 2012-04-20 12:23:31 PDT
Created attachment 617063 [details] [diff] [review]
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) 2012-04-20 13:31:21 PDT
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 2012-04-20 17:15:21 PDT
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/pastithas@mozilla.com-3837d6df4fb9
Comment 10 Panos Astithas [:past] 2012-04-21 03:21:07 PDT
Created attachment 617203 [details] [diff] [review]
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 [:past] 2012-04-24 12:41:05 PDT
https://hg.mozilla.org/integration/fx-team/rev/a974c45914eb
Comment 12 Tim Taubert [:ttaubert] 2012-04-27 05:47:47 PDT
https://hg.mozilla.org/mozilla-central/rev/a974c45914eb
Comment 13 :Ehsan Akhgari (out sick) 2012-05-02 13:24:32 PDT
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) 2012-05-03 05:55:00 PDT
https://hg.mozilla.org/integration/fx-team/rev/a84d63b5ba47
Comment 15 Tim Taubert [:ttaubert] 2012-05-04 07:26:13 PDT
https://hg.mozilla.org/mozilla-central/rev/a84d63b5ba47

Note You need to log in before you can comment on or make changes to this bug.