[jsdbg2] Implement most of Debugger.Environment

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

Other Branch
mozilla11
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 563561 [details] [diff] [review]
WIP 1 - untested
Assignee: general → jorendorff
(Assignee)

Updated

6 years ago
Blocks: 691894
(Assignee)

Comment 2

6 years ago
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.
Attachment #563561 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #564655 - Flags: review?(jimb)
(Assignee)

Comment 3

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

Comment 4

6 years ago
Created attachment 564996 [details] [diff] [review]
v3
Attachment #564655 - Attachment is obsolete: true
Attachment #564996 - Flags: review?(jimb)
(Assignee)

Updated

6 years ago
Blocks: 692984
Blocks: 694526
Keywords: dev-doc-needed

Comment 5

6 years ago
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

6 years ago
Sorry, hit "publish" before I intended.

Comment 7

6 years ago
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

6 years ago
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

6 years ago
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+
(Assignee)

Updated

6 years ago
Depends on: 698028
(Assignee)

Comment 10

6 years ago
Created attachment 570759 [details] [diff] [review]
WIP 4

Unbitrotted version of v3.
(Assignee)

Comment 11

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

Comment 12

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

Comment 13

6 years ago
Created attachment 579803 [details] [diff] [review]
Interdiff from WIP 4 to WIP 5
(Assignee)

Comment 14

6 years ago
Created attachment 579805 [details] [diff] [review]
WIP 5
Attachment #570759 - Attachment is obsolete: true

Comment 15

6 years ago
Created attachment 579886 [details] [diff] [review]
Add a test that checks environment chains of functions with deep static chains and shallow dynamic chains.
Attachment #579886 - Flags: review?(jorendorff)

Comment 16

6 years ago
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'.
Attachment #579886 - Attachment is obsolete: true
Attachment #579886 - Flags: review?(jorendorff)
Attachment #580137 - Flags: review?(jorendorff)
(Assignee)

Updated

6 years ago
Attachment #579886 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #580137 - Flags: review?(jorendorff) → review+

Comment 17

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

4 years ago
Summary: Implement most of Debugger.Environment → [jsdbg2] Implement most of Debugger.Environment
https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Environment
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.