690558 - [jsdbg2] Implement most of Debugger.Environment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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

Attachment: WIP 1 - untested

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: 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]]

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

Attachment: v3

Comment 5

[Jim Blandy :jimb]

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]

Sorry, hit "publish" before I intended.

Comment 7

[Jim Blandy :jimb]

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]

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]

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

Attachment: WIP 4

Unbitrotted version of v3.

Comment 11

[Jason Orendorff [:jorendorff]]

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

Thanks, bugzilla. That was awesome.

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: Interdiff from WIP 4 to WIP 5

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: WIP 5

Comment 15

[Jim Blandy :jimb]

Attachment: Add a test that checks environment chains of functions with deep static chains and shallow dynamic chains.

Comment 16

[Jim Blandy :jimb]

Attachment: Add a test that checks environment chains of functions with deep static chains and shallow dynamic chains.

Fixed unconventional definition of 'range'.

Comment 17

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c620a2f8919

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3c620a2f8919

Comment 19

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Environment