747514 - [jsdbg2] Implement Debugger.Environment.prototype.callee accessor

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

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]

Clearing in-testsuite: for now; will re-set when this actually has a patch.

Comment 2

[Jim Blandy :jimb]

Proposed docs at:
https://github.com/jimblandy/DebuggerDocs/compare/master...env-callee

Comment 3

[Jim Blandy :jimb]

Attachment: Assert that object environments' scopes are either WithObjects or non-DebugScopes.

Just a cleanup in the neighborhood.

Comment 4

[Jim Blandy :jimb]

Attachment: Implement Debugger.Environment.prototype.callee accessor.

Comment 5

[Jim Blandy :jimb]

Try: https://tbpl.mozilla.org/?tree=Try&rev=21267313f791

Comment 6

[Jim Blandy :jimb]

My, that's a nice green Try run.

Comment 7

[Jason Orendorff [:jorendorff]]

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]

(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]

(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!

Comment 10

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9078667d27e3

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d0d38708ed3e
https://hg.mozilla.org/mozilla-central/rev/9078667d27e3