Last Comment Bug 694526 - Implement Function grip actor message handling
: Implement Function grip actor message handling
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Panos Astithas [:past]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 690377 690558 692984
Blocks: minotaur 692405 694538
  Show dependency treegraph
 
Reported: 2011-10-14 03:35 PDT by Panos Astithas [:past]
Modified: 2012-02-01 07:32 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (7.84 KB, patch)
2011-10-20 10:02 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
WIP 2 (13.75 KB, patch)
2011-10-21 08:59 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
WIP 3 (15.77 KB, patch)
2011-10-24 12:40 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch (27.64 KB, patch)
2011-10-26 09:22 PDT, Panos Astithas [:past]
dcamp: review-
Details | Diff | Splinter Review
Working patch v2 (27.05 KB, patch)
2011-10-27 08:30 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch v3 (27.87 KB, patch)
2011-10-31 10:14 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch v4 (27.97 KB, patch)
2011-11-01 05:03 PDT, Panos Astithas [:past]
dcamp: review+
Details | Diff | Splinter Review

Description Panos Astithas [:past] 2011-10-14 03:35:02 PDT
The remote protocol specification describes the messages that a Function grip actor must respond to:

https://wiki.mozilla.org/Remote_Debugging_Protocol#Functions

We can implement "nameAndParameters" and "scope" now, but "decompile" will have to wait for bug 690377.
Comment 1 Panos Astithas [:past] 2011-10-20 08:25:41 PDT
"scope" depends on bug 690558, but I'll stub out the relevant parts for now.
Comment 2 Panos Astithas [:past] 2011-10-20 09:59:03 PDT
Since I'm adding support for Debugger.Environment as part of this patch, handling the "assign" request will need bug 692984 to be resolved first.
Comment 3 Panos Astithas [:past] 2011-10-20 10:02:29 PDT
Created attachment 568429 [details] [diff] [review]
WIP

Still WIP, lots of TODOs.
Comment 4 Panos Astithas [:past] 2011-10-21 08:59:42 PDT
Created attachment 568662 [details] [diff] [review]
WIP 2

Finished the server part. I've added checks for the missing platform bits, since the relevant patches may not land for some time and we might want to land this sooner. Also did some moving around in the breakpoint actor code for consistency with the rest of the code.

In the next iteration, I'll expose these protocol operations in the client API.
Comment 5 Panos Astithas [:past] 2011-10-24 12:40:59 PDT
Created attachment 569140 [details] [diff] [review]
WIP 3

The variables pane now displays the "this" and "arguments" objects.
Comment 6 Panos Astithas [:past] 2011-10-26 09:22:23 PDT
Created attachment 569696 [details] [diff] [review]
Working patch

This is a working version that gets the debugger to display "this", "arguments" and function call parameters in the property view. Further inspection of the parameter objects will be implemented in bug 694538.

The tests do not exercise many new code paths, like the various EnvironmentActor-related ones, since that functionality depends on the bugs blocking this one. I'd like to get this in since the existing functionality is very useful and file a followup bug to write tests (and fixes if necessary) for the parts that depend on the other bugs. Otherwise we could shelve this patch for now and get back to finish it after the blocking ones land.
Comment 7 Dave Camp (:dcamp) 2011-10-26 13:21:11 PDT
Comment on attachment 569696 [details] [diff] [review]
Working patch

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

r- because I don't understand the environmentPool stuff.  Might just need an explanation to switch that to r+.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +515,5 @@
> +    this._environmentPool = environmentPool;
> +    this._environmentPool.addActor(actor);
> +    this.conn.addActorPool(environmentPool);
> +    environment.actor = actor;
> +

I don't understand this.  What happens to an existing this._environmentPool when this method is called a second time?

Do we need a separate pool for environments?  Do they not make sense as breakpoint-lifetime actors?

@@ -639,0 +672,37 @@
> > +  /**
> > +   * Handle a protocol request to provide the source code of a function.
> > +   */
> > +  onDecompile: function OA_onDecompile(aRequest) {
NaN more ...

actor ? actor.grip() : actor is a pretty common pattern, I wonder if we should add a simple grip(actor) function to do that.
Comment 8 Panos Astithas [:past] 2011-10-27 08:30:34 PDT
Created attachment 569984 [details] [diff] [review]
Working patch v2

(In reply to Dave Camp (:dcamp) from comment #7)
> Comment on attachment 569696 [details] [diff] [review] [diff] [details] [review]
> Working patch
> 
> Review of attachment 569696 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> r- because I don't understand the environmentPool stuff.  Might just need an
> explanation to switch that to r+.
> 
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +515,5 @@
> > +    this._environmentPool = environmentPool;
> > +    this._environmentPool.addActor(actor);
> > +    this.conn.addActorPool(environmentPool);
> > +    environment.actor = actor;
> > +
> 
> I don't understand this.  What happens to an existing this._environmentPool
> when this method is called a second time?
> 
> Do we need a separate pool for environments?  Do they not make sense as
> breakpoint-lifetime actors?

Doh, you're right, this is not good. I tried to copy the frame pool approach, but I botched it.

The environment actors lifetime should be similar to the lifetime of the breakpoint actors, but I didn't reuse that pool because of the different handling. The breakpoint pool is manually managed with explicit "delete" requests that remove objects, while the environment actors should be automatically managed, like the _pausePool or the threadLifetime pool. Come to think of it, the lifetime of environment actors should probably span across pauses, since the Debugger API mentions that the engine will return the same Debugger.Environment instance for the same scope. Breakpoints on the other hand could span contexts, since reloading a page should probably maintain the breakpoints both in the client and server.

In this patch I put all EnvironmentActors to the threadlifetimePool, so they should be properly taken care of by the existing pool handling code. There may be more stupid mistakes in this part of the patch, since it can't be tested yet.

> @@ -639,0 +672,37 @@
> > > +  /**
> > > +   * Handle a protocol request to provide the source code of a function.
> > > +   */
> > > +  onDecompile: function OA_onDecompile(aRequest) {
> NaN more ...
> 
> actor ? actor.grip() : actor is a pretty common pattern, I wonder if we
> should add a simple grip(actor) function to do that.

It seems prone to confusion to have both foo.grip() and grip(foo), but I gave it a shot. Unfortunately, some code paths didn't have grip(foo) in scope, like the event-triggered onDebuggerStatement. Fixing that would require to define grip(foo) as an inherited method, leading to even more confusion between foo.grip() and bar.grip(foo) or this.grip(foo).
Comment 9 Dave Camp (:dcamp) 2011-10-27 15:31:29 PDT
(In reply to Panos Astithas [:past] from comment #8)

(sorry, I meant pause lifetime, not breakpoint lifetime).

> In this patch I put all EnvironmentActors to the threadlifetimePool, so they
> should be properly taken care of by the existing pool handling code. There
> may be more stupid mistakes in this part of the patch, since it can't be
> tested yet.

Just talked this over with jimb: When requesting a frame's |environment|, that environment should be parented by the frame - when the frame is dropped, the environment should be dropped too.  So I guess FrameActors should each have a pool, to which we'll add frame-lifetime objects like the environment.
When requesting a function grip's |scope|, the environment should have the same parent as the function grip.  So you can just added it to the same pool that the function grip belongs to.

That can lead to multiple grips for the same environment, but there's some precedent for that when promoting grips to different lifetimes.
Comment 10 Panos Astithas [:past] 2011-10-31 10:14:59 PDT
Created attachment 570747 [details] [diff] [review]
Working patch v3

Changed environmentActor to accept a pool object and store the newly-created actor in there. Call sites provide the proper pool, since FrameActor now hosts a _frameLifetimePool. In the case of a "scope" request to a function, the function's registeredPool is used, as you suggested. This will not be the _frameLifetimePool though, since object actors are put into the _pausePool or _threadLifetimePool. Shouldn't object actors get stored in the same frame pool? Why are they treated differently?
Comment 11 Panos Astithas [:past] 2011-11-01 05:03:11 PDT
Created attachment 570970 [details] [diff] [review]
Working patch v4

Fixed a broken function grips unit test that I noticed while working on object grips.
Comment 13 Panos Astithas [:past] 2011-12-14 03:47:17 PST
(In reply to Panos Astithas [:past] from comment #1)
> "scope" depends on bug 690558, but I'll stub out the relevant parts for now.

Bug 690558 is now fixed, so we need to merge from fx-team in order to work on this.
Comment 14 Panos Astithas [:past] 2012-02-01 07:27:29 PST
(In reply to Panos Astithas [:past] from comment #13)
> (In reply to Panos Astithas [:past] from comment #1)
> > "scope" depends on bug 690558, but I'll stub out the relevant parts for now.
> 
> Bug 690558 is now fixed, so we need to merge from fx-team in order to work
> on this.

This work was done in bug 715543.

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