The default bug view has changed. See this FAQ.

[jsdbg2] Debugger.prototype.findScripts

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jimb)

Tracking

(Blocks: 1 bug)

Other Branch
mozilla13
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Depends on: 665167
Summary: Debugger.prototype.getAllScripts → [jsdbg2] Debugger.prototype.getAllScripts
Blocks: 670002
(Reporter)

Comment 1

6 years ago
Created attachment 551123 [details] [diff] [review]
v1
Assignee: general → jorendorff
Attachment #551123 - Flags: review?(jimb)
(Reporter)

Comment 2

6 years ago
Created attachment 551165 [details] [diff] [review]
v2

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)
(Assignee)

Comment 3

6 years ago
Given that JSScripts no longer need holder objects, does this patch need to be revised?
(Assignee)

Comment 4

5 years ago
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()) {
(Reporter)

Updated

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

Comment 7

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

Updated

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

Comment 9

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

Updated

5 years ago
Assignee: jorendorff → jimb
(Assignee)

Comment 10

5 years ago
Created attachment 601799 [details] [diff] [review]
Implement Debugger.prototype.findScripts.

First draft. Doesn't return eval scripts; no 'query' parameter.
(Assignee)

Updated

5 years ago
Attachment #551165 - Attachment is obsolete: true
(Assignee)

Comment 11

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

Comment 12

5 years ago
Created attachment 601826 [details] [diff] [review]
Implement Debugger.prototype.findScripts.

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)
(Assignee)

Updated

5 years ago
Attachment #601799 - Attachment is obsolete: true
(Assignee)

Comment 13

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

Comment 14

5 years ago
Created attachment 602203 [details] [diff] [review]
Implement Debugger.prototype.findScripts.

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)
(Reporter)

Comment 15

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

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8809550cb08
Target Milestone: --- → mozilla13
(Assignee)

Updated

5 years ago
Blocks: 733461
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
(Assignee)

Comment 17

5 years ago
(In reply to Jim Blandy :jimb from comment #16)

That is the wrong changeset id. Hmm.
(Assignee)

Comment 18

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.