889627 - Allow Debugger.Frame#eval and Debugger.Frame#evalWithBindings to specify the name of the file

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Brandon Benvie [:benvie]]

Right now evaluation with the debugger forces the script name to be "debugger eval code". It would be really useful if the script name was configurable like it is with `Cu.evalInSandbox`.

Comment 1

[Brandon Benvie [:benvie]]

Also this should cover `Debugger.Object#evalInGlobal` and `Debugger.Object#evalInGlobalWithBindings` (everything that calls into DebuggerGenericEval).

Comment 2

[Brandon Benvie [:benvie]]

I will be changing the signatures of these functions to be:

* Debugger.Frame.prototype.eval(code, [options])
* Debugger.Frame.prototype.evalWithBindings(code, bindings, [options])
* Debugger.Object.prototype.evalInGlobal(code, [options])
* Debugger.Object.prototype.evalInGlobalWithBindings(code, bindings, [options])

Where options can optionally be an object. Right now there will be only one option, "url", which can be used to specify a url to execute the code as. If no url is provided it will default to "debugger eval code" as it does now.

Comment 3

[Brandon Benvie [:benvie]]

Attachment: WIP1

First stab at this, without any tests. It works!

Comment 4

[Brandon Benvie [:benvie]]

The DebuggerDocs update pull request is at https://github.com/jimblandy/DebuggerDocs/pull/11.

Comment 5

[Brandon Benvie [:benvie]]

Attachment: WIP2

Bit of cleanup.

Comment 6

[Brandon Benvie [:benvie]]

Attachment: WIP3

Fixes some bugs, adds first set of tests (Object.evalInGlobal).

Comment 7

[Brandon Benvie [:benvie]]

Attachment: WIP4

More tests.

Comment 8

[Brandon Benvie [:benvie]]

Attachment: WIP5

https://tbpl.mozilla.org/?tree=Try&rev=e113ab308d93

Comment 9

[Brandon Benvie [:benvie]]

Attachment: WIP6

Hmm all the js tests are passing locally. Let's try this again.

https://tbpl.mozilla.org/?tree=Try&rev=b660bcb0db69

Comment 10

[Jim Blandy :jimb]

Comment on attachment 774209 [details] [diff] [review]
WIP6

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

This looks good, overall; some things to fix:

- be sure to free 'url' if it's dynamically allocated (as discussed on IRC)
- filenames are Latin-1, not UTF-8 (ditto)
- tests need meta-assertions

If you've got momentum, how about letting us specify the starting line as well (the 'lineno' argument to EvaluateInEnv)?

::: js/src/jit-test/tests/debug/Frame-eval-16.js
@@ +5,5 @@
> +function testUrl (options, expected) {
> +    dbg.onDebuggerStatement = function (frame) {
> +        dbg.onNewScript = function (script) {
> +            dbg.onNewScript = undefined;
> +            assertEq(script.url, expected);

Tests like these need to have some global variable that is initialized to false, set to true here, and then asserted to be true at the end of the test, to ensure that everything isn't passing just because the assertEq is never reached, for some unexpected reason. That is, you've got to assert that you actually did the assert that actually matters.

Some tests have a variable named 'log' that starts out as the empty string, and then the handler functions append characters to it. Then you assert at the end the contents of the entire string. That ensures that you took the path through the test that you expected.

Comment 11

[Brandon Benvie [:benvie]]

Attachment: WIP7

Addressed some of jimb's feedback.

* Change url to be NULL initialized and use ternary for default fallback
* Correctly free url when user data is provided
* Add lineNumber option

Still need to update tests and add testing for lineNumber. Also need to figure out why the tests are failing on try despite working locally.

Comment 12

[Brandon Benvie [:benvie]]

Attachment: WIP8

* Add tests for line number
* Add meta asserts using a counter to ensure all asserts have been hit

Now to figure out how to make the tests work on try. Really scratching my head here since running jit_test.py locally works fine.

Comment 13

[Brandon Benvie [:benvie]]

Comment on attachment 775793 [details] [diff] [review]
WIP8

I'm going to go ahead and flag you for review on this despite not being able to get it passing on try-server. Running `jit_test.py --tbpl` passes for me locally with this patch, so I think it's not that there's something wrong with the patch but something wrong with how I'm sending the push up to try. Damned if I know what it is though.

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Brandon Benvie [:bbenvie] from comment #13)
> Running `jit_test.py --tbpl` passes for me
> locally with this patch, so I think it's not that there's something wrong
> with the patch but something wrong with how I'm sending the push up to try.
> Damned if I know what it is though.

Did you try a debug build? It fails for me:

#0  js::CompartmentChecker::fail (c1=0x33f5400, c2=0x3433e00) at jscntxtinlines.h:47
#1  0x0035cdff in js::CompartmentChecker::check (this=0xbfff83f0, c=0x3433e00) at jscntxtinlines.h:68
#2  0x0035cd6f in js::CompartmentChecker::check (this=0xbfff83f0, obj=0x2640610) at jscntxtinlines.h:79
#3  0x00358dd3 in js::assertSameCompartment<JS::Rooted<JSObject*>, JS::Rooted<jsid> > (cx=0x224cca0, t1=@0xbfff8508, t2=@0xbfff84d8) at jscntxtinlines.h:170
#4  0x0033f42e in JS_ForwardGetPropertyTo (cx=0x224cca0, objArg=0x2640610, idArg={asBits = 37821920}, onBehalfOfArg=0x2640610, vp=0xbfff8808) at jsapi.cpp:4157
#5  0x0033f2cb in JS_GetPropertyById (cx=0x224cca0, objArg=0x2640610, idArg={asBits = 37821920}, vp=0xbfff8808) at jsapi.cpp:4145
#6  0x003400ab in JS_GetProperty (cx=0x224cca0, objArg=0x2640610, name=0x1432df5 "url", vp=0xbfff8808) at jsapi.cpp:4233
#7  0x000a3740 in DebuggerGenericEval (cx=0x224cca0, fullMethodName=0x1433244 "Debugger.Frame.prototype.eval", code=@0x3451b88, bindings=0x0, options={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x3451b90}, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::UnbarrieredMutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x3451b78}, dbg=0x33f6600, scope={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0xb39544}, iter=0xbfff8ae8) at vm/Debugger.cpp:4232
#8  0x000ac984 in DebuggerFrame_eval (cx=0x224cca0, argc=2, vp=0x3451b78) at vm/Debugger.cpp:4270

You probably have to enter options's compartment first before calling JS_GetProperty.

Comment 15

[Brandon Benvie [:benvie]]

Comment on attachment 775793 [details] [diff] [review]
WIP8

Crap, I didn't. Although I don't think that's what caused the try failures, since there's opt builds failing there. I have to fix this anyway, thanks for pointing it out!

Comment 16

[Brandon Benvie [:benvie]]

Attachment: WIP9

Fix debug assertions failing by making sure to enter the correct compartment.

https://tbpl.mozilla.org/?tree=Try&rev=150fe5e46313

Comment 17

[Brandon Benvie [:benvie]]

Attachment: WIP10

Looks like that resolved the issues with try. Thanks for the catch jandem!

Comment 18

[Jim Blandy :jimb]

Comment on attachment 777163 [details] [diff] [review]
WIP10

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

Great work. Thanks very much!

::: js/src/jit-test/tests/debug/Frame-eval-16.js
@@ +10,5 @@
> +            dbg.onNewScript = undefined;
> +            assertEq(script.url, expected);
> +            count--;
> +        };
> +        frame.eval("", options);

I *think* under some circumstances 'eval' checks for cases like empty strings (maybe?) and stuff that can be evaluated as JSON (definitely) and won't actually generate a script. You might want to consider passing eval something a little more substantial, like "1+2" or something. But you'll get a failure if that optimization interferes, so you'll know if it's ever a problem, so maybe it's not worth changing. Let's leave it.

@@ +22,5 @@
> +testUrl({ url: undefined }, "debugger eval code");
> +testUrl({ url: null }, "null");
> +testUrl({ url: 5 }, "5");
> +testUrl({ url: "test" }, "test");
> +assertEq(count, 0);

This is delicious.

::: js/src/vm/Debugger.cpp
@@ +4226,5 @@
> +    unsigned lineNumber = 1;
> +
> +    if (options.isObject()) {
> +        JSObject *obj = &options.toObject();
> +        AutoCompartment ac(cx, obj);

If you do this earlier, above the ac.construct call, can't we avoid this compartment wrangling?

Comment 19

[Brandon Benvie [:benvie]]

Attachment: WIP11

(In reply to Jim Blandy :jimb from comment #18)
> ::: js/src/vm/Debugger.cpp
> @@ +4226,5 @@
> > +    unsigned lineNumber = 1;
> > +
> > +    if (options.isObject()) {
> > +        JSObject *obj = &options.toObject();
> > +        AutoCompartment ac(cx, obj);
> 
> If you do this earlier, above the ac.construct call, can't we avoid this
> compartment wrangling?

Ah you're right! I've updated the patch to move the options stuff above `Maybe<AutoCompartment> ac;`.

Comment 20

[Rob Campbell [:rc] (:robcee)]

adding checkin-needed keyword as this may not show up on people's devtools list.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Sorry, didn't see the note about landing on fx-team until I'd already pushed to inbound :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/16eb4d747d4e

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/16eb4d747d4e