[jsdbg2] Implement Debugger.prototype.findSources

RESOLVED FIXED in Firefox 64

Status

()

RESOLVED FIXED
5 years ago
6 months ago

People

(Reporter: fitzgen, Assigned: arai)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Half the time we are using |findScripts| in the debugger server, we are just iterating over ever script and putting its D.Source in a set so that we can get the set of all sources. It sure would be a lot easier if the Debugger API took care of this for us, and it seems like it belongs at that layer as well.
Jim and I had hoped users would monkeypatch extra methods onto Debugger. Like this:

    Debugger.prototype.sources = function sources(query) {
        return new Set([s.source for (s of dbg.findScripts(query)) if (s.source)]);
    };

For whatever reason, people don't do this, so the alternative would be to create a file, js/src/builtin/Debugger.js, for this kind of thing.

This would be easy to add. We're already have js/src/builtin/Array.js for Array methods. See also the corresponding JS_SELF_HOSTED_FN lines in js/src/jsarray.cpp, and search js/src/Makefile.in for Array.js.
(In reply to Jason Orendorff [:jorendorff] from comment #1)
> Jim and I had hoped users would monkeypatch extra methods onto Debugger.
> Like this:
> 
>     Debugger.prototype.sources = function sources(query) {
>         return new Set([s.source for (s of dbg.findScripts(query)) if
> (s.source)]);
>     };
> 
> For whatever reason, people don't do this, so the alternative would be to
> create a file, js/src/builtin/Debugger.js, for this kind of thing.
> 
> This would be easy to add. We're already have js/src/builtin/Array.js for
> Array methods. See also the corresponding JS_SELF_HOSTED_FN lines in
> js/src/jsarray.cpp, and search js/src/Makefile.in for Array.js.

The problem here is that we would still have to iterate over all scripts, which has proven to be slow. After talking with Jim, we think it would be nice if JSCompartment kept a list of ScriptSourceObjects that we could iterate over and get D.Source objects for and just yield those. Then we could add query objects at a later point in time.
Blocks: 1479813
I have WIP implementation (except query, but should be sufficient for the main usage).
I'll post it after some more tests.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
OS: Mac OS X → Unspecified
Hardware: x86 → Unspecified
Version: 25 Branch → Trunk

Comment 4

8 months ago
If it doesn't need a query parameter, that's fine.
this also looks like want findSource, but I haven't yet figured out if it's really replace-able
(given that it's not applying unique)
https://searchfox.org/mozilla-central/rev/088938b1495aeac586bff94234c9382a4b8ca6da/devtools/server/actors/promises.js#68

I'll file followup bug for that one.
* Added Debugger::SourceQuery which iterates over scripts and collects unique sources (put into SourceQuery.sources GCHashSet)
  * Added Debugger::QueryBase as a base class for Debugger::{ScriptQuery,SourceQuery},
    which stores common fields and has methods for realm
    (I haven't applied this to ObjectQuery, since it doesn't share much code)
  * Added Debugger.findSources which returns the array of Debugger.Source in the debuggee globals.
    currently this doesn't support query, and it throws if any parameter is given
Attachment #8999427 - Flags: review?(jimb)
what ThreadActor#_discoverSources does is the following:
  1. collects scripts by Debugger.findScripts
  2. perform "unique" operation on sources, by creating <source,script> map
  3. iterates over scripts (map values), and creates source actors for corresponding source

which is actually just collecting sources and creates source actors.
so, replaced the whole code with Debugger.findSources.
Attachment #8999428 - Flags: review?(poirot.alex)
What PromisesActor#attach does is the following:
  1. collects scripts by Debugger.findScripts
  2. iterates over scripts, and creates source actors for corresponding source

this time, it doesn't explicitly perform "unique" operation, but
TabSources#createSourceActors returns cached actors if the source is known,
and PromisesActor#attach doesn't use the returned value,
so this also can be replaced by Debugger.findSources.
Attachment #8999429 - Flags: review?(poirot.alex)

Comment 9

7 months ago
Arai, following Nick's point in comment 2: do you have any measurements of the benefit here?
(In reply to Jim Blandy :jimb from comment #9)
> Arai, following Nick's point in comment 2: do you have any measurements of
> the benefit here?

in bug 1479813's case, the time taken by findSource and the surrounding code is reduced from 162ms to 13ms :)
given that we can skip wrapping scripts and putting many items in array and map.
(bug 1479813 comment #11)

Comment 11

7 months ago
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #2)
> The problem here is that we would still have to iterate over all scripts,
> which has proven to be slow. After talking with Jim, we think it would be
> nice if JSCompartment kept a list of ScriptSourceObjects that we could
> iterate over and get D.Source objects for and just yield those. Then we
> could add query objects at a later point in time.

One problem with both the original and this approach is that garbage collection can remove sources that the user expects to see. I don't know how often this affects actual users, but I've heard people complain about it.

It's probably not practical to retain all source objects forever, just in case the compartment might be debugged. But it might be practical for the compartment or realm to keep a set of all the URLs that have been loaded, regardless of GC. The size of this set could be capped. Then the debugger UI could at least populate its tree of scripts properly, and then fetch them on demand.

Comment 12

7 months ago
(In reply to Tooru Fujisawa [:arai] from comment #10)
> in bug 1479813's case, the time taken by findSource and the surrounding code
> is reduced from 162ms to 13ms :)
> given that we can skip wrapping scripts and putting many items in array and
> map.

Oh, right, you did post those measurements. Thank you!

Comment 13

7 months ago
Comment on attachment 8999427 [details] [diff] [review]
Part 1: Add Debugger.findSources.

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

This needs tests. See js/src/jit-test/tests/debug for examples, especially the ones with 'findScripts' in their name. (There are too many of those, but I think they're mostly concerned with queries, so this should not need nearly as many.) You can use the shell's `evaluate` builtin to create scripts with fake URLs and such:
https://searchfox.org/mozilla-central/rev/ca869724246f4230b272ed1c8b9944596e80d920/js/src/shell/js.cpp#6959-6999

::: js/src/doc/Debugger/Debugger.md
@@ +361,2 @@
>  :   Return an array of all [`Debugger.Source`][source] instances matching
> +    <i>query</i> (query is not yet implemented). Each source appears only once

I would just delete the query argument from the docs entirely. We can revive it from hg if we need it.

Comment 14

7 months ago
(In reply to Jim Blandy :jimb from comment #11)
> One problem with both the original and this approach is that garbage
> collection can remove sources that the user expects to see. I don't know how
> often this affects actual users, but I've heard people complain about it.

To be clear, I do not think that should block this patch.
Thank you for reviewing :)

Removed query parameter and now it just ignores any arguments.
also added testcases
Attachment #8999427 - Attachment is obsolete: true
Attachment #8999427 - Flags: review?(jimb)
Attachment #8999911 - Flags: review?(jimb)
Comment on attachment 8999428 [details] [diff] [review]
Part 2: Use Debugger.findSources in ThreadActor#_discoverSources.

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

Thanks a lot for tweaking that!

I'm not sure I'm a good reviewer for this, but I'm not sure we have anyone actively maintaining these actors.
Attachment #8999428 - Flags: review?(poirot.alex) → review+
Comment on attachment 8999429 [details] [diff] [review]
Part 3: Use Debugger.findSources in PromisesActor#attach.

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

I'm wondering if we should remove this actor.
It seems to be unused:
  https://searchfox.org/mozilla-central/search?q=symbol:%23PromisesFront&redirect=false
Attachment #8999429 - Flags: review?(poirot.alex) → review+

Comment 18

7 months ago
Comment on attachment 8999911 [details] [diff] [review]
Part 1: Add Debugger.findSources.

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

Looks good!
Attachment #8999911 - Flags: review?(jimb) → review+

Comment 20

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/46dfdfe14067
https://hg.mozilla.org/mozilla-central/rev/8169bed1fafc
https://hg.mozilla.org/mozilla-central/rev/3e61a2874367
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Backing out for browser_dbg-quick-open.js failures.

backout: https://hg.mozilla.org/mozilla-central/rev/ddd5a54f6a52fecbb51a177f514f3cdf0d40caff

push : https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3e61a2874367c13ba2c5ec361a5633f7da153d45&filter-searchStr=devtools merged to central. 

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=195428919&repo=mozilla-inbound&lineNumber=4154

19:45:08     INFO - TEST-PASS | devtools/client/debugger/new/test/mochitest/browser_dbg-quick-open.js | quickOpen enabled - 
19:45:08     INFO - Waiting for state change: selected source
19:45:08     INFO - Console message: [JavaScript Warning: "Removed unsafe attribute. Element: svg. Attribute: xmlns." {file: "resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js" line: 5811}]
19:45:08     INFO - Console message: [JavaScript Warning: "Removed unsafe attribute. Element: svg. Attribute: xmlns." {file: "resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js" line: 5811}]
19:45:08     INFO - Console message: [JavaScript Warning: "Removed unsafe attribute. Element: svg. Attribute: xmlns." {file: "resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js" line: 5811}]
19:45:08     INFO - Console message: [JavaScript Warning: "Removed unsafe attribute. Element: svg. Attribute: xmlns." {file: "resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js" line: 5811}]
19:45:08     INFO - Buffered messages finished
19:45:08     INFO - TEST-UNEXPECTED-FAIL | devtools/client/debugger/new/test/mochitest/browser_dbg-quick-open.js | Test timed out - 
19:45:08     INFO - Removing tab.
19:45:08     INFO - Waiting for event: 'TabClose' on [object XULElement].
19:45:08     INFO - Got event: 'TabClose' on [object XULElement].
19:45:08     INFO - Tab removed and finished closing
19:45:08     INFO - GECKO(1982) | MEMORY STAT | vsize 4612MB | residentFast 561MB | heapAllocated 161MB
19:45:08     INFO - TEST-OK | devtools/client/debugger/new/test/mochitest/browser_dbg-quick-open.js | took 45287ms
Flags: needinfo?(arai.unmht)
Status: RESOLVED → REOPENED
status-firefox63: fixed → affected
Resolution: FIXED → ---
Target Milestone: mozilla63 → ---
to be clear, the actual issue is devtools/client/debugger/new/test/mochitest/browser_dbg_rr_breakpoints-01.js,
which I forgot to implement Debugger#findSources in replay debugger.

(browser_dbg-quick-open.js is pre-existing intermittent, bug 1438778)
Fixed corresponding part in WebReplay.
basically, made source/scriptSource functions more similar to script functions,
given that findScripts and findSources are similar.
Flags: needinfo?(arai.unmht)
Attachment #9003689 - Flags: review?(jimb)

Comment 24

7 months ago
Comment on attachment 9003689 [details] [diff] [review]
Part 1.5: Support Debugger#findSource in WebReplay.

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

::: devtools/server/actors/replay/debugger.js
@@ +169,5 @@
>    // ScriptSource methods
>    /////////////////////////////////////////////////////////
>  
>    _getSource(id) {
> +    const rv = this._scriptSources[id];

Nit: we should use a more descriptive name than 'rv'. Perhaps 'source'?

::: devtools/server/actors/replay/replay.js
@@ +511,5 @@
>      return RecordReplayControl.getContent(request.url);
>    },
>  
> +  findSources(request) {
> +    const rv = [];

Nit: 'rv' should be something more helpful, like 'sources'.
Attachment #9003689 - Flags: review?(jimb) → review+

Comment 26

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee0b4798b061
https://hg.mozilla.org/mozilla-central/rev/d96d56907ce0
https://hg.mozilla.org/mozilla-central/rev/37f94ae472d6
https://hg.mozilla.org/mozilla-central/rev/9cb7826b8f28
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago7 months ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Status: RESOLVED → REOPENED
status-firefox63: fixed → affected
Resolution: FIXED → ---
Target Milestone: mozilla63 → ---
bug 1486401 is caused by Part 4 (Part 3 in attachment).
I'll leave it to another bug given that other parts are more important,
and I'll reland others once bug 1438778 is fixed and confirmed this doesn't regress it again.
Flags: needinfo?(arai.unmht)
Depends on: 1438778
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b621aee022c61cfe496427354519c9dfd790aaa
Bug 956376 - Part 0: Use results with single file in browser_dbg-quick-open.js (debugger.html PR 6997) r=jlast

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0cb356cbf18fb88059be94d1c40f3feb2433e5
Bug 956376 - Part 1: Add Debugger.findSources. r=jimb

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f68c00ed7d9c6f37c16a452f38d4c7ab42462db
Bug 956376 - Part 2: Support Debugger#findSource in WebReplay. r=jimb

https://hg.mozilla.org/integration/mozilla-inbound/rev/27fd50934e4a2dace3c6abb8bed6a2965fc3bf4e
Bug 956376 - Part 3: Use Debugger.findSources in ThreadActor#_discoverSources. r=ochameau

Comment 30

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b621aee022c
https://hg.mozilla.org/mozilla-central/rev/4c0cb356cbf1
https://hg.mozilla.org/mozilla-central/rev/5f68c00ed7d9
https://hg.mozilla.org/mozilla-central/rev/27fd50934e4a
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago6 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
status-firefox63: affected → ---
You need to log in before you can comment on or make changes to this bug.