The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
mozilla15
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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.
Flags: in-testsuite?
(Assignee)

Comment 1

5 years ago
Clearing in-testsuite: for now; will re-set when this actually has a patch.
Flags: in-testsuite?
(Assignee)

Comment 2

5 years ago
Proposed docs at:
https://github.com/jimblandy/DebuggerDocs/compare/master...env-callee
(Assignee)

Comment 3

5 years ago
Created attachment 628194 [details] [diff] [review]
Assert that object environments' scopes are either WithObjects or non-DebugScopes.

Just a cleanup in the neighborhood.
Assignee: general → jimb
Status: NEW → ASSIGNED
Attachment #628194 - Flags: review?(jorendorff)
(Assignee)

Comment 4

5 years ago
Created attachment 628195 [details] [diff] [review]
Implement Debugger.Environment.prototype.callee accessor.
Attachment #628195 - Flags: review?(jorendorff)
(Assignee)

Comment 5

5 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=21267313f791
(Assignee)

Comment 6

5 years ago
My, that's a nice green Try run.
Attachment #628194 - Flags: review?(jorendorff) → review+
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.
Attachment #628195 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
(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!
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9078667d27e3
Flags: in-testsuite+
Target Milestone: --- → mozilla15
Blocks: 760414
https://hg.mozilla.org/mozilla-central/rev/d0d38708ed3e
https://hg.mozilla.org/mozilla-central/rev/9078667d27e3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.