Last Comment Bug 747514 - [jsdbg2] Implement Debugger.Environment.prototype.callee accessor
: [jsdbg2] Implement Debugger.Environment.prototype.callee accessor
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jim Blandy :jimb
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 760414
  Show dependency treegraph
 
Reported: 2012-04-20 13:29 PDT by Jim Blandy :jimb
Modified: 2012-06-01 08:38 PDT (History)
3 users (show)
jimb: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Assert that object environments' scopes are either WithObjects or non-DebugScopes. (716 bytes, patch)
2012-05-29 19:57 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Implement Debugger.Environment.prototype.callee accessor. (4.63 KB, patch)
2012-05-29 20:00 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review

Description Jim Blandy :jimb 2012-04-20 13:29:46 PDT
Debugger.Environment instances for function call frames' environments should have a 'callee' accessor that returns the Debugger.Object instance referring to the function for whose invocation the environment was created. (Unless you have filed form 27B-6, obviously.)

This will make it easier for the debug server to produce binding forms for environment forms, which are supposed to distinguish between locals and arguments.
Comment 1 Jim Blandy :jimb 2012-05-21 14:24:32 PDT
Clearing in-testsuite: for now; will re-set when this actually has a patch.
Comment 2 Jim Blandy :jimb 2012-05-29 14:11:17 PDT
Proposed docs at:
https://github.com/jimblandy/DebuggerDocs/compare/master...env-callee
Comment 3 Jim Blandy :jimb 2012-05-29 19:57:00 PDT
Created attachment 628194 [details] [diff] [review]
Assert that object environments' scopes are either WithObjects or non-DebugScopes.

Just a cleanup in the neighborhood.
Comment 4 Jim Blandy :jimb 2012-05-29 20:00:22 PDT
Created attachment 628195 [details] [diff] [review]
Implement Debugger.Environment.prototype.callee accessor.
Comment 5 Jim Blandy :jimb 2012-05-29 20:09:46 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=21267313f791
Comment 6 Jim Blandy :jimb 2012-05-30 08:59:48 PDT
My, that's a nice green Try run.
Comment 7 Jason Orendorff [:jorendorff] 2012-05-30 14:26:43 PDT
Comment on attachment 628195 [details] [diff] [review]
Implement Debugger.Environment.prototype.callee accessor.

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

r=me with some minor nits.

::: js/src/jit-test/tests/debug/Environment-callee-01.js
@@ +5,5 @@
> +var dbg = new Debugger;
> +var gw = dbg.addDebuggee(g);
> +
> +function check(code, expectedType, expectedCallee) {
> +  print("check(" + uneval(code) + ")");

print() in a test is fine with me, just pointing it out since I don't think you usually leave them in. They're in all three tests.

@@ +6,5 @@
> +var gw = dbg.addDebuggee(g);
> +
> +function check(code, expectedType, expectedCallee) {
> +  print("check(" + uneval(code) + ")");
> +  var hits = "";

Huh. Why is this initialized to the empty string? Later it's an int.

@@ +20,5 @@
> +}
> +
> +check('debugger;', 'object', null);
> +check('with({}) { debugger; };', 'with', null);
> +check('let (x=1) { debugger; };', 'declarative', null);

How about a test for a let-environment in a function?

  g.eval('function q() { let (x = 1) { debugger; } }');
  check('q()', 'declarative', ???);

Actually the proposed docs don't make it totally clear to me what should happen. The environment for the nested let-block should still have .callee pointing to the function, I suppose? Or null, since it was not a call, exactly, that created the environment?

::: js/src/vm/Debugger.cpp
@@ +4309,5 @@
> +
> +    Value rval = ObjectValue(*callee);
> +    if (!dbg->wrapDebuggeeValue(cx, &rval))
> +        return false;
> +    args.rval() = rval;

I think rval needs to be rooted, since wrapDebuggeeValue could trigger GC.
Or use args.rval() in-place.
Comment 8 Jim Blandy :jimb 2012-05-30 15:36:32 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #7)
> @@ +5,5 @@
> > +var dbg = new Debugger;
> > +var gw = dbg.addDebuggee(g);
> > +
> > +function check(code, expectedType, expectedCallee) {
> > +  print("check(" + uneval(code) + ")");
> 
> print() in a test is fine with me, just pointing it out since I don't think
> you usually leave them in. They're in all three tests.

Yeah, when I have one function that runs tests and is called several times, I've found putting a 'print' in there is useful to figure out which invocation is asserting.
Comment 9 Jim Blandy :jimb 2012-05-30 15:38:16 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #7)
> @@ +6,5 @@
> > +var gw = dbg.addDebuggee(g);
> > +
> > +function check(code, expectedType, expectedCallee) {
> > +  print("check(" + uneval(code) + ")");
> > +  var hits = "";
> 
> Huh. Why is this initialized to the empty string? Later it's an int.

No reason. Thanks for pointing it out.

> @@ +20,5 @@
> > +}
> > +
> > +check('debugger;', 'object', null);
> > +check('with({}) { debugger; };', 'with', null);
> > +check('let (x=1) { debugger; };', 'declarative', null);
> 
> How about a test for a let-environment in a function?
> 
>   g.eval('function q() { let (x = 1) { debugger; } }');
>   check('q()', 'declarative', ???);
> 
> Actually the proposed docs don't make it totally clear to me what should
> happen. The environment for the nested let-block should still have .callee
> pointing to the function, I suppose? Or null, since it was not a call,
> exactly, that created the environment?

It should be null, because it's not an environment created for the call itself. I'll try to tighten up the spec language.

> ::: js/src/vm/Debugger.cpp
> @@ +4309,5 @@
> > +
> > +    Value rval = ObjectValue(*callee);
> > +    if (!dbg->wrapDebuggeeValue(cx, &rval))
> > +        return false;
> > +    args.rval() = rval;
> 
> I think rval needs to be rooted, since wrapDebuggeeValue could trigger GC.
> Or use args.rval() in-place.

Oh, thanks!

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