Last Comment Bug 733461 - [jsdbg2] Debugger.prototype.findScripts doesn't implement 'query' parameter
: [jsdbg2] Debugger.prototype.findScripts doesn't implement 'query' parameter
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Jim Blandy :jimb
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 676281 734454
Blocks: js::dbg2
  Show dependency treegraph
 
Reported: 2012-03-06 10:56 PST by Jim Blandy :jimb
Modified: 2012-04-06 11:46 PDT (History)
3 users (show)
jimb: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement the 'global', 'url', and 'line' query properties in Debugger.prototype.findScripts. (29.10 KB, patch)
2012-03-09 13:14 PST, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Implement the 'query' parameter of Debugger.prototype.findScripts. (38.30 KB, patch)
2012-03-13 21:55 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review

Description Jim Blandy :jimb 2012-03-06 10:56:20 PST
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 2012-03-09 13:14:54 PST
Created attachment 604507 [details] [diff] [review]
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 2012-03-13 21:55:58 PDT
Created attachment 605659 [details] [diff] [review]
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 2012-03-13 21:59:00 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=f5efea9fd267
Comment 4 Jim Blandy :jimb 2012-03-16 16:53:21 PDT
Turns out Windows uses backslashes. Who knew??
Try: https://tbpl.mozilla.org/?tree=Try&rev=b24fa29f0aee
Comment 5 Jim Blandy :jimb 2012-03-16 23:38:51 PDT
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] 2012-04-04 13:39:58 PDT
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 2012-04-05 17:10:28 PDT
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 9 Matt Brubeck (:mbrubeck) 2012-04-06 11:46:53 PDT
https://hg.mozilla.org/mozilla-central/rev/b0da9a35a841

Note You need to log in before you can comment on or make changes to this bug.