Closed Bug 956376 Opened 7 years ago Closed 2 years ago

[jsdbg2] Implement Debugger.prototype.findSources

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: fitzgen, Assigned: arai)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

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
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)
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)
(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.
(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 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.
(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 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+
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
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 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+
Status: RESOLVED → REOPENED
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
You need to log in before you can comment on or make changes to this bug.