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

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: benvie, Assigned: benvie)

Tracking

Trunk
mozilla25
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Assignee)

Description

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

Updated

5 years ago
Blocks: 825039
(Assignee)

Updated

5 years ago
Assignee: general → bbenvie
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

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

Comment 2

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

Comment 3

5 years ago
Created attachment 772910 [details] [diff] [review]
WIP1

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

Comment 4

5 years ago
The DebuggerDocs update pull request is at https://github.com/jimblandy/DebuggerDocs/pull/11.
(Assignee)

Comment 5

5 years ago
Created attachment 772957 [details] [diff] [review]
WIP2

Bit of cleanup.
Attachment #772910 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 772996 [details] [diff] [review]
WIP3

Fixes some bugs, adds first set of tests (Object.evalInGlobal).
Attachment #772957 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Created attachment 773006 [details] [diff] [review]
WIP4

More tests.
Attachment #772996 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 774088 [details] [diff] [review]
WIP5

https://tbpl.mozilla.org/?tree=Try&rev=e113ab308d93
Attachment #773006 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Priority: -- → P2
(Assignee)

Comment 9

5 years ago
Created attachment 774209 [details] [diff] [review]
WIP6

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

https://tbpl.mozilla.org/?tree=Try&rev=b660bcb0db69
Attachment #774088 - Attachment is obsolete: true

Comment 10

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

Comment 11

5 years ago
Created attachment 774349 [details] [diff] [review]
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.
Attachment #774209 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Created attachment 775793 [details] [diff] [review]
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.
Attachment #774349 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
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.
Attachment #775793 - Flags: review?(jimb)
(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.
(Assignee)

Comment 15

5 years ago
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!
Attachment #775793 - Flags: review?(jimb)
(Assignee)

Comment 16

5 years ago
Created attachment 776827 [details] [diff] [review]
WIP9

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

https://tbpl.mozilla.org/?tree=Try&rev=150fe5e46313
Attachment #775793 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Created attachment 777163 [details] [diff] [review]
WIP10

Looks like that resolved the issues with try. Thanks for the catch jandem!
Attachment #776827 - Attachment is obsolete: true
Attachment #777163 - Flags: review?(jimb)

Comment 18

5 years ago
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?
Attachment #777163 - Flags: review?(jimb) → review+
(Assignee)

Comment 19

5 years ago
Created attachment 777414 [details] [diff] [review]
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;`.
Attachment #777163 - Attachment is obsolete: true
Attachment #777414 - Flags: review+
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
adding checkin-needed keyword as this may not show up on people's devtools list.
Keywords: checkin-needed
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
Keywords: checkin-needed
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/16eb4d747d4e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.