Closed Bug 690558 Opened 8 years ago Closed 8 years ago

[jsdbg2] Implement most of Debugger.Environment

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: jorendorff, Assigned: jorendorff)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 4 obsolete files)

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.)
Attached patch WIP 1 - untested (obsolete) — Splinter Review
Assignee: general → jorendorff
Blocks: 691894
Attached patch v2 (obsolete) — Splinter Review
I spun off env.eval and .evalWithBindings into bug 691894, since they require going in and doing some damage to the parser.
Attachment #563561 - Attachment is obsolete: true
Attachment #564655 - Flags: review?(jimb)
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.
Attachment #564655 - Flags: review?(jimb)
Attached patch v3Splinter Review
Attachment #564655 - Attachment is obsolete: true
Attachment #564996 - Flags: review?(jimb)
Blocks: 692984
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.
Sorry, hit "publish" before I intended.
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 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 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.
Attachment #564996 - Flags: review?(jimb) → review+
Depends on: 698028
Attached patch WIP 4 (obsolete) — Splinter Review
Unbitrotted version of v3.
(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.
Assignee: jorendorff → blackconnect
Component: JavaScript Engine → Java-Implemented Plugins
QA Contact: general → blackconnect
Target Milestone: --- → mozilla10
Version: Other Branch → 11 Branch
Thanks, bugzilla. That was awesome.
Assignee: blackconnect → jorendorff
Component: Java-Implemented Plugins → JavaScript Engine
QA Contact: blackconnect → general
Target Milestone: mozilla10 → ---
Version: 11 Branch → Other Branch
Attached patch WIP 5Splinter Review
Attachment #570759 - Attachment is obsolete: true
Fixed unconventional definition of 'range'.
Attachment #579886 - Attachment is obsolete: true
Attachment #579886 - Flags: review?(jorendorff)
Attachment #580137 - Flags: review?(jorendorff)
Attachment #579886 - Flags: review+
Attachment #580137 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c620a2f8919
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/3c620a2f8919
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Summary: Implement most of Debugger.Environment → [jsdbg2] Implement most of Debugger.Environment
You need to log in before you can comment on or make changes to this bug.