Last Comment Bug 690558 - [jsdbg2] Implement most of Debugger.Environment
: [jsdbg2] Implement most of Debugger.Environment
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla11
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
Depends on: 698028
Blocks: 687543 691894 692984 694526
  Show dependency treegraph
 
Reported: 2011-09-29 14:54 PDT by Jason Orendorff [:jorendorff]
Modified: 2014-12-12 15:41 PST (History)
8 users (show)
jimb: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP 1 - untested (29.78 KB, patch)
2011-09-29 14:55 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
v2 (57.97 KB, patch)
2011-10-04 13:29 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
v3 (58.51 KB, patch)
2011-10-05 14:04 PDT, Jason Orendorff [:jorendorff]
jimb: review+
Details | Diff | Review
WIP 4 (54.77 KB, patch)
2011-10-31 10:45 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
Interdiff from WIP 4 to WIP 5 (3.49 KB, patch)
2011-12-07 12:57 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
WIP 5 (56.23 KB, patch)
2011-12-07 12:58 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
Add a test that checks environment chains of functions with deep static chains and shallow dynamic chains. (1.91 KB, patch)
2011-12-07 16:01 PST, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Review
Add a test that checks environment chains of functions with deep static chains and shallow dynamic chains. (1.89 KB, patch)
2011-12-08 12:09 PST, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Review

Description Jason Orendorff [:jorendorff] 2011-09-29 14:54:38 PDT
This bug is for:
   Debug.Environment.prototype.type, .outerEnvironment, .object,
       .names(), .find(), .eval(), .evalWithBindings()
   Debugger.Function.prototype.scope
   Debugger.Frame.prototype.environment

This doesn't include .defineVariable or .getVariableDescriptor which are needed for bug 687543. (I guess .eval() gets you everything, but we can do a little better than that I hope.)
Comment 1 Jason Orendorff [:jorendorff] 2011-09-29 14:55:23 PDT
Created attachment 563561 [details] [diff] [review]
WIP 1 - untested
Comment 2 Jason Orendorff [:jorendorff] 2011-10-04 13:29:05 PDT
Created attachment 564655 [details] [diff] [review]
v2

I spun off env.eval and .evalWithBindings into bug 691894, since they require going in and doing some damage to the parser.
Comment 3 Jason Orendorff [:jorendorff] 2011-10-05 13:29:54 PDT
Comment on attachment 564655 [details] [diff] [review]
v2

Oh, two of the tests have silly mistakes. New patch coming in a few minutes.  C++ part of the patch will be unchanged.
Comment 4 Jason Orendorff [:jorendorff] 2011-10-05 14:04:55 PDT
Created attachment 564996 [details] [diff] [review]
v3
Comment 5 Jim Blandy :jimb 2011-10-26 18:37:57 PDT
Comment on attachment 564996 [details] [diff] [review]
v3

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

I didn't see any tests for 'find' finding bindings that are function arguments, or that are 'let' bindings; it seems like we should have those. It would be nice to cover both captured and live Call objects, and captured and live Block objects.

::: js/src/jit-test/tests/debug/Environment-find-04.js
@@ +6,5 @@
> +var dbg = Debugger(g);
> +var hits = 0;
> +g.h = function () {
> +    var env = dbg.getNewestFrame().environment;
> +    assertThrowsInstanceOf(function () { env.find(); }, TypeError);

If we're testing undefined, should we also test 'null', false, and {}? Maybe not; I don't think we're that thorough about testing the typechecking of every other function in the API.
Comment 6 Jim Blandy :jimb 2011-10-26 18:38:16 PDT
Sorry, hit "publish" before I intended.
Comment 7 Jim Blandy :jimb 2011-10-26 19:04:25 PDT
Comment on attachment 564996 [details] [diff] [review]
v3

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

Some more work-in-progress comments:

::: js/src/jit-test/tests/debug/Environment-gc-01.js
@@ +7,5 @@
> +dbg.onEnterFrame = function (frame) { env = frame.environment; };
> +assertEq(g.f(22), 44);
> +dbg.onEnterFrame = undefined;
> +
> +assertEq(env.find("x"), env);

Okay, I see now that this covers the argument case...

@@ +10,5 @@
> +
> +assertEq(env.find("x"), env);
> +assertEq(env.names().join(","), "x");
> +
> +gc();

This can't be written more accurately with findReferences?

::: js/src/jit-test/tests/debug/Environment-gc-02.js
@@ +15,5 @@
> +// Test that they work now.
> +function check() {
> +    for (var i = 0; i < N; i++) {
> +        assertEq(arr[i].find("b"), null);
> +        assertEq(arr[i].find("a"), arr[i]);

And this covers the non-live call case.

::: js/src/jit-test/tests/debug/Environment-identity-02.js
@@ +24,5 @@
> +
> +// separate visits to a block in the same call frame
> +test("(function () { for (var i = 0; i < 3; i++) { let j = i * 4; h(); }})();", 3);
> +
> +// two strict direct eval calls in the same function scope

"three", right?

::: js/src/jit-test/tests/debug/Environment-identity-03.js
@@ +7,5 @@
> +
> +var g = newGlobal('new-compartment');
> +g.eval("function h() { debugger; }");
> +var dbg = Debugger(g);
> +var hits, name, shared, unshared;

It's obnoxious to take a long time to review and then pick nits, but it seems like it would be nicer to avoid this global state and having to reset it by making the variables local to 'test', and then assigning to dbg.onDebuggerStatement, in 'test', a handler that closes over those local variables, instead of global variables.
Comment 8 Jim Blandy :jimb 2011-10-27 12:03:36 PDT
Comment on attachment 564996 [details] [diff] [review]
v3

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

More work-in-progress comments:

::: js/src/jit-test/tests/debug/Environment-identity-03.js
@@ +66,5 @@
> +    code += "function f" + i + "(a" + i + ") {\n";
> +code += "function f" + N + "(a" + N + ") { eval('a0 + a1'); h(); }\n";
> +for (i = N - 1; i >= 0; i--) {
> +    var call = "f" + (i + 1) + "(a" + i + " - 1);";
> +    code += call + " if (a" + i + " === 0) " + call + " }\n";

This is awesome! It might be cool to have a similar variant where the call stack remains flat, while the environment chain grows longer and longer:

function f<n>() { return function f<n+1>() { return function f<n+2>() { ... } } }

::: js/src/jit-test/tests/debug/Environment-names-02.js
@@ +10,5 @@
> +       "with ({a: 1, '0xcafe': 2, ' ': 3, '': 4, '0': 5}) h();");
> +assertEq(names.indexOf("0xcafe"), -1);
> +assertEq(names.indexOf(" "), -1);
> +assertEq(names.indexOf(""), -1);
> +assertEq(names.indexOf("0"), -1);

You probably want to check that 'a' *is* present, too, right?

::: js/src/jit-test/tests/debug/Environment-type-01.js
@@ +2,5 @@
> +
> +var g = newGlobal('new-compartment');
> +var dbg = Debugger(g);
> +var actual;
> +g.h = function () { actual += dbg.getNewestFrame().environment.type; }

This could also be a function local to 'test'.

@@ +20,5 @@
> +test("for (let x in h()) ;", 'object');
> +test("for (let x in {a:1}) h();", 'declarative');
> +test("'use strict'; eval('var z = 1; h();');", 'declarative');
> +test("for (var x in [h(m) for (m in [1])]) ;", 'declarative');
> +test("for (var x in (h(m) for (m in [1]))) ;", 'declarative');

Would it be worth testing the environment of B in (A for (x in B)) as well?

Catch blocks? Function bodies, no eval?

frame.evalWithBindings? I guess that's unspecified.

::: js/src/jit-test/tests/debug/Object-environment-02.js
@@ +1,1 @@
> +// The .environment of a function Debugger.Object is an Environment object.

It would be nice to test the .environment property of a native function, as well.
Comment 9 Jim Blandy :jimb 2011-10-27 14:55:36 PDT
Comment on attachment 564996 [details] [diff] [review]
v3

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

All right! I have no comments on the actual code; I guess GetPropertyNames and lookupProperty do all the really challenging stuff, which is as it should be!

::: js/src/jit-test/tests/debug/Environment-identity-03.js
@@ +66,5 @@
> +    code += "function f" + i + "(a" + i + ") {\n";
> +code += "function f" + N + "(a" + N + ") { eval('a0 + a1'); h(); }\n";
> +for (i = N - 1; i >= 0; i--) {
> +    var call = "f" + (i + 1) + "(a" + i + " - 1);";
> +    code += call + " if (a" + i + " === 0) " + call + " }\n";

This is awesome! It might be cool to have a similar variant where the call stack remains flat, while the environment chain grows longer and longer:

function f<n>() { return function f<n+1>() { return function f<n+2>() { ... } } }

::: js/src/jit-test/tests/debug/Environment-names-02.js
@@ +10,5 @@
> +       "with ({a: 1, '0xcafe': 2, ' ': 3, '': 4, '0': 5}) h();");
> +assertEq(names.indexOf("0xcafe"), -1);
> +assertEq(names.indexOf(" "), -1);
> +assertEq(names.indexOf(""), -1);
> +assertEq(names.indexOf("0"), -1);

You probably want to check that 'a' *is* present, too, right?

::: js/src/jit-test/tests/debug/Environment-type-01.js
@@ +2,5 @@
> +
> +var g = newGlobal('new-compartment');
> +var dbg = Debugger(g);
> +var actual;
> +g.h = function () { actual += dbg.getNewestFrame().environment.type; }

This could also be a function local to 'test'.

@@ +20,5 @@
> +test("for (let x in h()) ;", 'object');
> +test("for (let x in {a:1}) h();", 'declarative');
> +test("'use strict'; eval('var z = 1; h();');", 'declarative');
> +test("for (var x in [h(m) for (m in [1])]) ;", 'declarative');
> +test("for (var x in (h(m) for (m in [1]))) ;", 'declarative');

Would it be worth testing the environment of B in (A for (x in B)) as well?

Catch blocks? Function bodies, no eval?

frame.evalWithBindings? I guess that's unspecified.

::: js/src/jit-test/tests/debug/Object-environment-01.js
@@ +5,5 @@
> +var gw = dbg.addDebuggee(g);
> +assertEq(gw.environment, undefined);
> +
> +g.eval("var r = /x/;");
> +var rw = gw.getOwnPropertyDescriptor("r").value;

This would be a nice application for evalInGlobal, if we had it.

::: js/src/jit-test/tests/debug/Object-environment-02.js
@@ +1,1 @@
> +// The .environment of a function Debugger.Object is an Environment object.

It would be nice to test the .environment property of a native function, as well.

::: js/src/vm/Debugger.cpp
@@ +3850,5 @@
> +    if (env->isCall() || env->isBlock() || env->isDeclEnv()) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_DEBUG_NO_SCOPE_OBJECT);
> +        return false;
> +    }
> +    JSObject *obj = env->isWith() ? env->getProto() : env;

Would it be good to actually make this an 'if' and assert that env is a global object if it's not a 'with'? Then this could become an 'if/else' chain handling all the cases.
Comment 10 Jason Orendorff [:jorendorff] 2011-10-31 10:45:07 PDT
Created attachment 570759 [details] [diff] [review]
WIP 4

Unbitrotted version of v3.
Comment 11 Jason Orendorff [:jorendorff] 2011-12-07 12:53:11 PST
(In reply to Jim Blandy :jimb from comment #9)
> Would it be good to actually make this an 'if' and assert that env is a
> global object if it's not a 'with'? Then this could become an 'if/else'
> chain handling all the cases.

It's not necessarily a global object. In a DOM event handler, like an onclick= attribute, other objects (the Element and I think its ancestors in the DOM tree) can be on the scope chain.

It occurs to me that we have no facility for testing anything like that in the shell... which is bad. Filed bug 708384 for that.
Comment 12 Jason Orendorff [:jorendorff] 2011-12-07 12:55:44 PST
Thanks, bugzilla. That was awesome.
Comment 13 Jason Orendorff [:jorendorff] 2011-12-07 12:57:24 PST
Created attachment 579803 [details] [diff] [review]
Interdiff from WIP 4 to WIP 5
Comment 14 Jason Orendorff [:jorendorff] 2011-12-07 12:58:45 PST
Created attachment 579805 [details] [diff] [review]
WIP 5
Comment 15 Jim Blandy :jimb 2011-12-07 16:01:59 PST
Created attachment 579886 [details] [diff] [review]
Add a test that checks environment chains of functions with deep static chains and shallow dynamic chains.
Comment 16 Jim Blandy :jimb 2011-12-08 12:09:47 PST
Created attachment 580137 [details] [diff] [review]
Add a test that checks environment chains of functions with deep static chains and shallow dynamic chains.

Fixed unconventional definition of 'range'.
Comment 18 Ed Morley [:emorley] 2011-12-09 06:55:38 PST
https://hg.mozilla.org/mozilla-central/rev/3c620a2f8919

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