956376 - [jsdbg2] Implement Debugger.prototype.findSources

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 1

[Jason Orendorff [:jorendorff]]

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.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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.

Comment 3

[Tooru Fujisawa [:arai]]

I have WIP implementation (except query, but should be sufficient for the main usage).
I'll post it after some more tests.

Comment 4

[Jim Blandy :jimb]

If it doesn't need a query parameter, that's fine.

Comment 5

[Tooru Fujisawa [:arai]]

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.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add Debugger.findSources.

* 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

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Use Debugger.findSources in ThreadActor#_discoverSources.

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.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Use Debugger.findSources in PromisesActor#attach.

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.

Comment 9

[Jim Blandy :jimb]

Arai, following Nick's point in comment 2: do you have any measurements of the benefit here?

Comment 10

[Tooru Fujisawa [:arai]]

(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

[Jim Blandy :jimb]

(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

[Jim Blandy :jimb]

(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

[Jim Blandy :jimb]

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

[Jim Blandy :jimb]

(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.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add Debugger.findSources.

Thank you for reviewing :)

Removed query parameter and now it just ignores any arguments.
also added testcases

Comment 16

[Alexandre Poirot [:ochameau]]

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.

Comment 17

[Alexandre Poirot [:ochameau]]

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

Comment 18

[Jim Blandy :jimb]

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

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

Looks good!

Comment 19

[Tooru Fujisawa [:arai]]

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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e61a2874367c13ba2c5ec361a5633f7da153d45
Bug 956376 - Part 3: Use Debugger.findSources in PromisesActor#attach. r=ochameau

Comment 20

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/46dfdfe14067
https://hg.mozilla.org/mozilla-central/rev/8169bed1fafc
https://hg.mozilla.org/mozilla-central/rev/3e61a2874367

Comment 21

[Natalia Csoregi [:nataliaCs]]

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

Comment 22

[Tooru Fujisawa [:arai]]

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)

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Part 1.5: Support Debugger#findSource in WebReplay.

Fixed corresponding part in WebReplay.
basically, made source/scriptSource functions more similar to script functions,
given that findScripts and findSources are similar.

Comment 24

[Jim Blandy :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'.

Comment 25

[Tooru Fujisawa [:arai]]

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

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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb7826b8f28a91d0c3e0034c4a645f08d371885
Bug 956376 - Part 4: Use Debugger.findSources in PromisesActor#attach. r=ochameau

Comment 26

[Eliza Balazs [:ebalazs_]]

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

Comment 27

[Cosmin Sabou [:CosminS]]

This caused a high failure rate of bug 1438778 and also caused bug 1486401
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&tochange=9cb7826b8f28a91d0c3e0034c4a645f08d371885&fromchange=291b098c977eefa575929c76febffd440488e511&filter-searchStr=linux%20devtools&selectedJob=196019333

Backout link: https://hg.mozilla.org/mozilla-central/rev/9c13dbdf4cc9baf98881b4e2374363587fb017b7

Comment 28

[Tooru Fujisawa [:arai]]

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.

Comment 29

[Tooru Fujisawa [:arai]]

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

[Eliza Balazs [:ebalazs_]]

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