Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

(Blocks: 1 bug)

Trunk
mozilla14
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Updated

5 years ago
Depends on: 676281
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Updated

5 years ago
Assignee: general → jimb
Status: NEW → ASSIGNED
(Assignee)

Comment 1

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

Updated

5 years ago
Blocks: 734454
(Assignee)

Updated

5 years ago
No longer blocks: 734454
Depends on: 734454
(Assignee)

Comment 2

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

Comment 3

5 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=f5efea9fd267
(Assignee)

Comment 4

5 years ago
Turns out Windows uses backslashes. Who knew??
Try: https://tbpl.mozilla.org/?tree=Try&rev=b24fa29f0aee
Assignee: jimb → jhpedemonte
Component: JavaScript Engine → Java to XPCOM Bridge
QA Contact: general → xpcom-bridge

Updated

5 years ago
Assignee: jhpedemonte → general
Component: Java to XPCOM Bridge → JavaScript Engine
QA Contact: xpcom-bridge → general
(Assignee)

Updated

5 years ago
Assignee: general → jimb
(Assignee)

Comment 5

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

Updated

5 years ago
Blocks: 560314
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.
Attachment #605659 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 7

5 years ago
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).
Assignee: jimb → blackconnect
Component: JavaScript Engine → Java-Implemented Plugins
QA Contact: general → blackconnect
Assignee: blackconnect → general
Component: Java-Implemented Plugins → JavaScript Engine
QA Contact: blackconnect → general
Assignee: general → jimb
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0da9a35a841
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/b0da9a35a841
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.