Closed Bug 676281 Opened 13 years ago Closed 12 years ago

[jsdbg2] Debugger.prototype.findScripts

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: jorendorff, Assigned: jimb)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Debuggers need an API to get all existing scripts for a given debuggee. (That is, Debugger.Script objects for all existing scripts that could conceivably be the .script of a Frame whose scope includes a given debuggee global object.)

Steps 1-3 below are necessary because there is currently no way to create a Debugger.Script object for a function script or eval script, given only a pointer to the JSScript. We currently need a holder object. That has to change.

Plan:

1. Populate JSScript::object for eval scripts and JS_Evaluate* just like we do for JS_CompileScript. Stop destroying those scripts eagerly; let GC collect them.

2. Make function scripts populate JSScript::object (pointing to the model Function JSObject).

3. Remove all the "unheld script" nonsense and the passing around of scope objects.

4. Add Debugger.prototype.getAllScripts; it'll use the JSCompartment's circular linked list of all scripts.
Depends on: 665167
Summary: Debugger.prototype.getAllScripts → [jsdbg2] Debugger.prototype.getAllScripts
Attached patch v1 (obsolete) — Splinter Review
Assignee: general → jorendorff
Attachment #551123 - Flags: review?(jimb)
Attached patch v2 (obsolete) — Splinter Review
Add code and tests for invalid arguments. D'oh.
Attachment #551123 - Attachment is obsolete: true
Attachment #551123 - Flags: review?(jimb)
Attachment #551165 - Flags: review?(jimb)
Given that JSScripts no longer need holder objects, does this patch need to be revised?
This patch needs to be rewritten for the new JSScript arrangement. Cribbing from other stuff I see, it seems like the answer would involve this sort of magic:

    for (gc::CellIter i(cx, this, gc::FINALIZE_SCRIPT); !i.done(); i.next()) {
Attachment #551165 - Flags: review?(jimb)
Will getAllScripts() include worker scripts as well? I mean scripts added in a page with new Worker("worker.js").
(In reply to Panos Astithas [:past] from comment #5)
> Will getAllScripts() include worker scripts as well? I mean scripts added in
> a page with new Worker("worker.js").

Would be nice to have, but we could accept a follow-up for that.
(In reply to Panos Astithas [:past] from comment #5)
> Will getAllScripts() include worker scripts as well? I mean scripts added in
> a page with new Worker("worker.js").

No, because those scripts are in a different thread.

The way the API is designed you'll have a Debugger object in that thread, too. But it needs more work that we haven't started yet. I'll chat with jimb today and we'll see if we can get some bugs filed for it.
Blocks: js::dbg2
(In reply to Jason Orendorff [:jorendorff] from comment #7)
> (In reply to Panos Astithas [:past] from comment #5)
> > Will getAllScripts() include worker scripts as well? I mean scripts added in
> > a page with new Worker("worker.js").
> 
> No, because those scripts are in a different thread.
> 
> The way the API is designed you'll have a Debugger object in that thread,
> too. But it needs more work that we haven't started yet. I'll chat with jimb
> today and we'll see if we can get some bugs filed for it.

that sounds like we'll need some corresponding UI to go along with it. If more than one thread, display a thread list near the stack.
Change title to reflect new spec, draft on this branch in the debugge docs GitHub repo:
https://github.com/jimblandy/DebuggerDocs/tree/script-enumeration
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Summary: [jsdbg2] Debugger.prototype.getAllScripts → [jsdbg2] Debugger.prototype.findScripts
Assignee: jorendorff → jimb
First draft. Doesn't return eval scripts; no 'query' parameter.
Attachment #551165 - Attachment is obsolete: true
The scripts are unrooted from the point that we leave the AutoLockGC region until we put them in the final array. We need an AutoScriptVector or something of that ilk.
This version eliminates the locking around CellIter (billm said it was unnecessary), and defines an AutoScriptVector type to hold the list we're building.
Attachment #601826 - Flags: review?(jorendorff)
Attachment #601799 - Attachment is obsolete: true
Third draft. Finds eval scripts. No 'query' parameter support.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=e40b0d124aef
Attachment #601826 - Attachment is obsolete: true
Attachment #601826 - Flags: review?(jorendorff)
Attachment #602203 - Flags: review?(jorendorff)
Comment on attachment 602203 [details] [diff] [review]
Implement Debugger.prototype.findScripts.

Great!

In vm/Debugger.cpp:
>+            JSScript *script = fri.fp()->script();
>+            /*

Style nit: blank line before that comment.
Attachment #602203 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8809550cb08
Target Milestone: --- → mozilla13
Blocks: 733461
Flags: in-testsuite+
(In reply to Jim Blandy :jimb from comment #16)

That is the wrong changeset id. Hmm.
Here's the real mozilla-inbound changeset ID:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6692fb9f8f21
https://hg.mozilla.org/mozilla-central/rev/6692fb9f8f21
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.