733461 - [jsdbg2] Debugger.prototype.findScripts doesn't implement 'query' parameter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

The absence of any findScripts whatsoever was blocking debugger UI work, so we landed it without 'query' argument support, but that needs to be implemented, too.

Without the query argument, the debugger needs to implement the same functionality in JS, by producing a complete list of scripts and then narrowing it down. At the moment, the C++ can't really do any better algorithm-wise, but it can create less garbage, and if we improve SM's data structures in the future, the algorithm could be a lot better without affecting debugger JS code.

Comment 1

[Jim Blandy :jimb]

Attachment: Implement the 'global', 'url', and 'line' query properties in Debugger.prototype.findScripts.

Here's my work-in-progress. Everything but 'innermost' and 'column' implemented. 

The 'innermost' is important, because it's what you need for setting breakpoints at source locations. I'll implement that before r?ing.

The 'column' stuff can't be implemented yet, because scripts don't have column information, as far as I can tell. Brendan was working on this, but the changes don't seem to have landed. It's less critical.

Comment 2

[Jim Blandy :jimb]

Attachment: Implement the 'query' parameter of Debugger.prototype.findScripts.

'query' parameter fully implemented, except for 'column'. Will r? once try approves.

Comment 3

[Jim Blandy :jimb]

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

Comment 4

[Jim Blandy :jimb]

Turns out Windows uses backslashes. Who knew??
Try: https://tbpl.mozilla.org/?tree=Try&rev=b24fa29f0aee

Comment 5

[Jim Blandy :jimb]

Comment on attachment 605659 [details] [diff] [review]
Implement the 'query' parameter of Debugger.prototype.findScripts.

All right, the try run is all marked up.

I will cheerfully write whatever additional tests you suggest, unless they're going to take days and days. :)

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 605659 [details] [diff] [review]
Implement the 'query' parameter of Debugger.prototype.findScripts.

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

Very nice. r=me with a few nits.

::: js/src/jit-test/tests/debug/Debugger-findScripts-07.js
@@ +25,5 @@
> +assertEq(scripts.indexOf(g2gw.script) != -1, true);
> +
> +scripts = dbg.findScripts({global: g3});
> +assertEq(scripts.indexOf(g1fw.script) != -1, false);
> +assertEq(scripts.indexOf(g2gw.script) != -1, false);

Maybe just check that scripts is empty, since g3 isn't a debuggee and findScripts only returns debuggee scripts.

::: js/src/jit-test/tests/debug/Debugger-findScripts-08.js
@@ +64,5 @@
> +assertEq(scripts.indexOf(g1gw.script) != -1, false);
> +assertEq(scripts.indexOf(g2fw.script) != -1, false);
> +assertEq(scripts.indexOf(g2gw.script) != -1, true);
> +
> +scripts = dbg.findScripts({url:"xlerb"}); // "XLERB"???

lol

::: js/src/jit-test/tests/debug/Debugger-findScripts-09.js
@@ +32,5 @@
> +
> +// Any value for 'innermost' is interpreted as a boolean.
> +assertEq(dbg.findScripts({url:"", line:1, innermost:true}).length, 0);
> +assertEq(dbg.findScripts({url:"", line:1, innermost:1}).length, 0);
> +assertEq(dbg.findScripts({url:"", line:1, innermost:"yes"}).length, 0);

The output is the same whether 'innermost' is interpreted as a boolean or not; but maybe that was intended.

::: js/src/jit-test/tests/debug/Debugger-findScripts-10.js
@@ +1,2 @@
> +// Debugger.prototype.findScripts shouldn't accept global objects in queries that aren't
> +// already debuggees.

I don't understand the comment. It looks like the test checks that we do accept non-debuggee global objects in queries.

::: js/src/jit-test/tests/debug/Debugger-findScripts-12.js
@@ +44,5 @@
> +                                 assertEq("" + s + " present", "not present");
> +                         });
> +        present.forEach(function (s) {
> +                             if (expected.indexOf(s) == -1)
> +                                 assertEq("" + s + " not present", "is present");

The first argument to assertEq is "actual"; the second is "expected", so I think "is present" and "not present" are reversed here and on line 44.

::: js/src/vm/Debugger.cpp
@@ +2035,4 @@
>          debuggees.remove(global);
>  }
>  
> +namespace js {

This doesn't have to enclose the class if you don't want it to. "Debugger" already unambiguously refers to js::Debugger here.

@@ +2041,5 @@
>  typedef HashSet<JSCompartment *, DefaultHasher<JSCompartment *>, RuntimeAllocPolicy> CompartmentSet;
>  
> +/* 
> + * A class for parsing 'findScripts' query arguments, and recognizing which scripts match the
> + * criteria they set forth.

Micro-nit: the comma looks wrong to me. Sorry.

@@ +2079,5 @@
> +        if (!query->getProperty(cx, cx->runtime->atomState.globalAtom, &global))
> +            return false;
> +        if (global.isUndefined())
> +            matchAllDebuggeeGlobals();
> +        else {

Style nit: curly braces around the consequent.

@@ +2087,5 @@
> +            GlobalObject *globalObject = &referent->global();
> +            /*
> +             * If the given global isn't a debuggee, just leave the set of
> +             * acceptable globals empty; we'll return no scripts.
> +             */

Style nit: blank line before this comment.

@@ +2109,5 @@
> +        if (!query->getProperty(cx, cx->runtime->atomState.lineAtom, &lineProperty))
> +            return false;
> +        if (lineProperty.isUndefined())
> +            hasLine = false;
> +        else if (lineProperty.isInt32()) {

Nit: Curly braces again.

Also, I think .isInt32() is wrong, we have to check .isNumber() and then do further checks for whole-number-ness.

@@ +2371,5 @@
> +
> +    if (argc >= 1) {
> +        if (!args[0].isObject()) {
> +            js_ReportValueErrorFlags(cx, JSREPORT_ERROR, JSMSG_UNEXPECTED_TYPE,
> +                                     JSDVG_SEARCH_STACK, args[0], NULL, "not an object", NULL);

I think I mentioned something like this in a previous review: I'd like to see this nice error message added to js::NonNullObject instead.

Comment 7

[Jim Blandy :jimb]

In general, all comments addressed.

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> The output is the same whether 'innermost' is interpreted as a boolean or
> not; but maybe that was intended.

Clarified comment.

> I don't understand the comment. It looks like the test checks that we do
> accept non-debuggee global objects in queries.

Yeah; comment fixed.

> The first argument to assertEq is "actual"; the second is "expected", so I
> think "is present" and "not present" are reversed here and on line 44.

I was deeply confused. I want you to know, however, that this in no way suggests that coding late at night or with little sleep carries consequences.

> This doesn't have to enclose the class if you don't want it to. "Debugger"
> already unambiguously refers to js::Debugger here.

I started out with that not being a class nested in Debugger, and then forgot to remove the namespace wrapping when I moved it in.

> Micro-nit: the comma looks wrong to me. Sorry.

It was lousy prose.

> Also, I think .isInt32() is wrong, we have to check .isNumber() and then do
> further checks for whole-number-ness.

Thanks. Math.sqrt(4) to the rescue (the only way I know to reliably get integral floats).

Comment 8

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0da9a35a841

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/b0da9a35a841