795917 - Get executable lines through the remote debugging protocol

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

It should be possible to get list of executable lines for given script. This is already possible to do in JSD2 implementation, but the API is not exposed through the remote debugging protocol

Honza

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Honza, is this something that Firebug absolutely must have or is it a nice to have? I don't think any other debugger displays visual cues as to where a breakpoint can be set, so I would assume that this is more something Firebug users are accustomed to, but could presumably move away from.

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Panos Astithas [:past] from comment #1)
> Honza, is this something that Firebug absolutely must have or is it a nice
> to have? I don't think any other debugger displays visual cues as to where a
> breakpoint can be set, so I would assume that this is more something Firebug
> users are accustomed to, but could presumably move away from.

Of course it doesn't block the user from debugging JS if this features is not there, but it's one of the many little features that make Firebug great (and unique) tool. So, I would like to keep it, especially when the back end API are already there and we only need to extend the remote protocol.

Honza

Comment 3

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

We could make this a new RDP request to the SourceActor. Would need to iterate over every Debugger.Script in the source and then iterate over each bytecode in the script and get its line. Add that line to a set. Once done iterating source map each line position and return the set of source mapped lines over the protocol.

https://wiki.mozilla.org/DevTools/Hacking
https://wiki.mozilla.org/Remote_Debugging_Protocol
http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#2409

Comment 4

[Seris]

Attachment: toolkit/devtools/server/actors/script.js Add a function to get executables lines

I've done a part of the bugfix, the function has been write.
But i don't know what to do after that. How can i share this ?

Comment 5

[Eddy Bruel [:ejpbruel]]

(In reply to Seris from comment #4)
> Created attachment 8419446 [details] [diff] [review]
> toolkit/devtools/server/actors/script.js Add a function to get executables
> lines
> 
> I've done a part of the bugfix, the function has been write.
> But i don't know what to do after that. How can i share this ?

To get your patch into the tree, you usually go through the following steps:
1. (Re)write your patch
2. Run the tests against your patch.
3. If any of the tests fail, go back to 1.
4. Otherwise, request a review for your patch.
5. If you got any review comments, go back to 1.
6. Otherwise, request for somebody to land your patch.

Here's some documentation on how to run the devtools tests:
https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests

To request a review for your patch, you can do the same thing you just did, except you have to set the review flag instead of the feedback flag.

Hope that clarifies things. If you have any more questions, don't hesitate to ask!

Comment 6

[James Long (:jlongster)]

Just happened to see this bug; won't we need to update this every time garbage collection is run? Right now, top-level forms can be garbage collected which make it impossible to set a breakpoint on it. There's another bug somewhere to fix that, but I feel like that should be fixed first... Otherwise, it'll look like you definitely can set a breakpoint in certain places, where you can't.

Comment 7

[Seris]

Attachment: getExecutablesLines.patch

Comment 8

[Seris]

Attachment: bug-795917-fix.patch

Comment 9

[Eddy Bruel [:ejpbruel]]

(In reply to James Long (:jlongster) from comment #6)
> Just happened to see this bug; won't we need to update this every time
> garbage collection is run? Right now, top-level forms can be garbage
> collected which make it impossible to set a breakpoint on it. There's
> another bug somewhere to fix that, but I feel like that should be fixed
> first... Otherwise, it'll look like you definitely can set a breakpoint in
> certain places, where you can't.

Does Firebug also work this way?

In my opinion, we can look at this in two ways. Either we provide visual cues for lines on which setting a breakpoint will definitely have an effect. If that's what we want to do, I agree with jlongster: we should make sure that scripts being garbage collected doesn't interfere with that.

Alternatively, we can interpret the visual cues so that setting a breakpoint on a line without these cues will definitely *not* have an effect, in which case we should move forward as planned.

Honza, can you provide some feedback on how you want this feature to work in Firebug?

Comment 10

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8419661 [details] [diff] [review]
bug-795917-fix.patch

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

Thanks for doing this Serris!

The overall idea of your patch looks good. The only problem is that you're not taking into account that sources can be source mapped. In case you don't know what source mapping is about, here's a few links that hopefully clarifies things:  http://www.html5rocks.com/en/tutorials/developertools/sourcemaps/ and http://sokra.github.io/source-map-visualization/

In a nutshell, we have original sources (which is what the user sees in the debugger) and generated sources (what the engine sees). When we call getExecutableLines, we expect aUrl to be the url of the original source. Since the engine only knows about generated sources, the first step should be to map the original source url to a generated source url.

Once we have the generated source url, the second step is to call getAllColumnOffsets with this url. We need to use this instead of getAllOffsets because source maps can map multiple original lines to a single generate line. This function returns a bunch of offsets with a url, line number, column number, etc. Since source mapping can map multiple urls to a single url, we are only interested in those offsets for which the original url matches that of the source actor.

The line numbers in each offset represent lines in the generated source file, but what we are interested in is the executable line numbers in the original source file. So as the next step we need to map these line numbers back to the original source. Finally, since multiple offsets may map to the same line, we need to remove duplicate line numbers.

To map between original sources and generated sources, you can use the ThreadSources object. You can get this object from the thread actor, which is a property of the source actor.

As a final remark, note that you should also add your method to SourceActor.prototype.requestTypes if you want it to be recognized by the protocol.

I hope that clarifies things for you. Source mapping can be confusing when you first run into it, so don't hesitate to ask if you have any questions!

::: toolkit/devtools/server/actors/script.js
@@ +2594,5 @@
> +   * @param String url
> +   * @return Array - Executables lines of the script
> +   **/
> +  getExecutablesLines: function(aUrl){
> +    var dbg = new Debugger(this._threadActor.global);

There's no need to create a new Debugger instance here. The thread actor already has a reference to it.

@@ +2599,5 @@
> +
> +    let exec_lines = [];
> +
> +    // Get scripts of the current tab
> +    let scripts = dbg.findScripts();

You don't have to filter for url's manually here, you can pass an option to findScripts with a url property that will do the filtering for you.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Eddy Bruel [:ejpbruel] from comment #9)
> (In reply to James Long (:jlongster) from comment #6)
> > Just happened to see this bug; won't we need to update this every time
> > garbage collection is run? Right now, top-level forms can be garbage
> > collected which make it impossible to set a breakpoint on it. There's
> > another bug somewhere to fix that, but I feel like that should be fixed
> > first... Otherwise, it'll look like you definitely can set a breakpoint in
> > certain places, where you can't.
> 
> Does Firebug also work this way?
Yeah, Firebug is solving the same problem. Since there are no GC events Firebug
could utilize (or am I wrong?), the Script panel viewport (the visible area)
is regularly updated to reflect the current state. Note that Firebug displays line numbers
of executable line in green.

> In my opinion, we can look at this in two ways. Either we provide visual
> cues for lines on which setting a breakpoint will definitely have an effect.
> If that's what we want to do, I agree with jlongster: we should make sure
> that scripts being garbage collected doesn't interfere with that.
> 
> Alternatively, we can interpret the visual cues so that setting a breakpoint
> on a line without these cues will definitely *not* have an effect, in which
> case we should move forward as planned.
> 
> Honza, can you provide some feedback on how you want this feature to work in
> Firebug?
Agree, some visual clues are definitely needed.
I can see following cases:

1) The line is executable and the user can set a breakpoint
 - e.g. clicking on the breakpoint column (in the Script panel) creates red circle
   representing the breakpoint.

2) The line is not executable and the user can't set a breakpoint
 - e.g. clicking on the breakpoint column doesn't create a red circle

3) The line was executable, but isn't anymore since the script has been garbage collected.
 - e.g. clicking on the breakpoint column creates red circle with a decorator (a badge)
   indicating that the breakpoint is not active now, but can hit when the page is reloaded
   and the script exists again (again for limited time). This might be known as future
   breakpoint.


Honza

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

The debugger can't really tell between cases 2 and 3 above, it only knows that there is no bytecode in the specified URL & line. The only "smart" thing the thread actor does currently is to consider breakpoints in URLs that it doesn't know about as outright errors, while breakpoints in URLs that used to exist at some point but no longer contain bytecode are treated as case 3 above (although it may well be that the user is trying to set a breakpoint at a blank line in a scavenged script - case 2 above).

Comment 13

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

1) We should go ahead and land the patch here without worrying about the GC sensitive behavior; we can worry about that before we/firebug start building UI on top of it. No need to slow up this patch before then, when the fix likely won't even touch the same code.

2) I just realized that a solution to different problem I've been kicking around would also solve this one. Debugger.prototype.findScripts doesn't do any indexing, it just iterates over every script in the debuggee compartments. This can be slow (I've seen ~200ms jank), and so I've been thinking about implementing something like BreakpointStore but for D.Script to do simple indexing. Whenever we call dbg.findScripts, we'd replace that with ScriptStore.prototype.findScripts or whatever. New D.Scripts would be added to the store via the dbg.onNewScript hook. The ScriptStore could be blown away every page navigation. Not only would this speed up script queries, it would hold strong references to the D.Scripts which would solve our GC sensitivity issues here and elsewhere.

Comment 14

[Seris]

Attachment: bug-795917-fix.patch

Add support to source map

Comment 15

[Seris]

Attachment: bug-795917-fix.patch

Comment 16

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8421750 [details] [diff] [review]
bug-795917-fix.patch

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

Good work! This looks like a big improvement over your previous patch. I don't see anything fundamentally wrong with it, which is why my review comments will be mostly about style issues this time.

Note that style issues are usually not a big deal (well, I guess they are to some people ;-)), and if there are only a few, you sometimes get a positive review for a patch anyway, provided you address the comments before you land it. The reason I didn't do that this time is that your patch still needs tests :)

As I said on irc earlier, let's talk to fitzgen and see what kind of test coverage we need for this to be able to land.

::: toolkit/devtools/server/actors/script.js
@@ +2667,5 @@
>    /**
> +   * Get all executables lines from the current source
> +   * @return Array - Executables lines of the script
> +   **/
> +  getExecutablesLines: function(){

Space here after the ().

@@ +2669,5 @@
> +   * @return Array - Executables lines of the script
> +   **/
> +  getExecutablesLines: function(){
> +    // Check if the original source is source mapped
> +    if(this._sourceMap){

Please use spaces here to separate the parenthesised condition, like this:

if (this._sourceMap) {

@@ +2672,5 @@
> +    // Check if the original source is source mapped
> +    if(this._sourceMap){
> +      let exec_lines = [];
> +      // Position of executables lines in the generated source
> +      let offsets = this._extractAllColumnOffsets(this._generatedSource);

You didn't pass a second argument to extractAllColumnOffsets. That's obviously valid JS, of course, but in general I don't think we should omit arguments that are expected to be booleans instead of undefined. Just explicitly pass false here.

@@ +2674,5 @@
> +      let exec_lines = [];
> +      // Position of executables lines in the generated source
> +      let offsets = this._extractAllColumnOffsets(this._generatedSource);
> +
> +      for (var o = 0; o < offsets.length; o++) {

Using the new for...of syntax you could actually write this as:

for (let offset of offsets)

To learn more about the for...of syntax, you can take a look at:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of

As an aside, you should also prefer the use of let over var. You already did that everywhere else, so I'm guessing this was just an oversight :)

@@ +2694,5 @@
> +
> +  /**
> +   * Extract all executables lines from the current source
> +   * @param String url 
> +   * @param Boolean onlyLine

Thanks for taking the trouble to write comments for this as well. Perhaps this comment would be even better if you also gave a short description of what the arguments are for?

Also, note that this function doesn't actually extract executable lines, but offsets (see comment below), so you might want to phrase that bit differently.

@@ +2697,5 @@
> +   * @param String url 
> +   * @param Boolean onlyLine
> +   * @return Array - Executables lines of the script
> +   **/
> +  _extractAllColumnOffsets: function(url, onlyLine){

I'm not completely happy with this name, since we don't return all column offsets, only the offsets that are entry points. And sometimes we return line offsets instead of column offsets, if onlyLine is set to true.

How do you feel about naming this getExecutableOffsets instead?

@@ +2702,5 @@
> +    let dbg = this._thread.dbg;
> +    let scripts = dbg.findScripts(url);
> +    let exec_lines = [];
> +
> +    for (let s = 0; s < scripts.length; s++) {

See my comment on using the for...of syntax above.

@@ +2705,5 @@
> +
> +    for (let s = 0; s < scripts.length; s++) {
> +      let offsets = scripts[s].getAllColumnOffsets();
> +
> +      for (var o = 0; o < offsets.length; o++) {

Same here.

@@ +2707,5 @@
> +      let offsets = scripts[s].getAllColumnOffsets();
> +
> +      for (var o = 0; o < offsets.length; o++) {
> +        let to_push = onlyLine ? offsets[o].lineNumber : offsets[o];
> +        if(exec_lines.indexOf(to_push) === -1){

I understand what you're trying to do here, but using an array as a set is somewhat inefficient, since array lookup is O(n).

Did you know that SpiderMonkey has support for the Set object?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set

My advice is to use that instead.

Comment 17

[Seris]

Attachment: bug-795917-fix.patch

Comment 18

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

Comment on attachment 8421901 [details] [diff] [review]
bug-795917-fix.patch

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

::: toolkit/devtools/server/actors/script.js
@@ +2670,5 @@
> +   **/
> +  getExecutablesLines: function() {
> +    // Check if the original source is source mapped
> +    if (this._sourceMap){
> +      let exec_lines = [];

Style nitpick: please use camelCase and not snake_case.

@@ +2698,5 @@
> +   * @param String url - extract offsets of the script with this url
> +   * @param Boolean onlyLine - will return only the line number
> +   * @return Set - Executables lines of the script
> +   **/
> +  getExecutableOffsets: function(url, onlyLine){

Echoing what Eddy said about this function being called "getExecutableOffsets" but returning executable lines, which is counter to its name.

@@ +2928,5 @@
>    "blackbox": SourceActor.prototype.onBlackBox,
>    "unblackbox": SourceActor.prototype.onUnblackBox,
>    "prettyPrint": SourceActor.prototype.onPrettyPrint,
> +  "disablePrettyPrint": SourceActor.prototype.onDisablePrettyPrint,
> +  "getExecutablesLines": SourceActor.prototype.getExecutablesLines

RDP request handlers return

  { error: "someErrorName", message: "Human readable error message" }

objects on failure, and on success return objects with whatever data we expected. So either the request handler shouldn't be |getExecutableLines| but some function which calls |getExecutableLines| or |getExecutableLines| should return an object

  { executableLines: [...] }

on success.

Comment 19

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8421901 [details] [diff] [review]
bug-795917-fix.patch

Clearing the review flag as agreed on irc, because the patch doesn't yet contains tests.

Comment 20

[Eddy Bruel [:ejpbruel]]

Quick update: I helped Seris with some issues he had with his patch. The test seems to run now, but isn't complete yet. Once it is, we can go for another review round, and try to get it landed.

Seris, any idea on when you'll be able to get this up for another review?

Comment 21

[Seris]

Attachment: bug-795917-fix.patch

Both actor's method and test working

Comment 22

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8482174 [details] [diff] [review]
bug-795917-fix.patch

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

Sorry it took so long Serris, but the good news is your patch looks ready to land!

I have a few more style issues that I'd like you to address. In particular getExecutablesLines should be getExecutableLines. Additionally, it would be nice if we could sort the array of results from lowest to highest line number.

r+ with those changes addressed. I assume you don't have commit access yet? If you don't, you can put the final version of the patch up here and set the checkin flag, and I will make sure to land it for you.

Thanks for finishing this up!

::: toolkit/devtools/client/dbg-client.jsm
@@ +2408,5 @@
> +   * Get executables lines from SourceClient
> +   *
> +   * @param aCallback Function
> +   *        The callback function called when we receive the response from the server.
> +   */

This does not do what the comment suggests. Moreover, it seems to be a copy of the blackBox function above it. Did you add this by mistake?

::: toolkit/devtools/server/actors/script.js
@@ +2644,5 @@
>    /**
> +   * Get all executables lines from the current source
> +   * @return Array - Executables lines of the current script
> +   **/
> +  getExecutablesLines: function () {

Please rename this to getExecutableLines

@@ +2656,5 @@
> +    if (this._sourceMap){
> +      lines = new Set();
> +
> +      // Position of executables lines in the generated source
> +      let offsets = this.getExecutablesOffsets(this._generatedSource , false);

Style nit: no space before the ,

@@ +2663,5 @@
> +          line: offset.lineNumber,
> +          column: offset.columnNumber
> +        });
> +
> +        if(source === this._url){

Style nit: spaces before the ( and after the )

@@ +2672,5 @@
> +      // Converting the set given by getExecutablesOffsets to an array
> +      lines = this.getExecutablesOffsets(this._url, true);
> +    }
> +
> +    packet.lines = [line for (line of lines)];

It would be nice if we could sort the array from lowest to highest.

@@ +2683,5 @@
> +   * @param String url - extract offsets of the script with this url
> +   * @param Boolean onlyLine - will return only the line number
> +   * @return Set - Executables offsets/lines of the script
> +   **/
> +  getExecutablesOffsets: function (url, onlyLine){

Please rename this to getExecutableOffsets

@@ +2686,5 @@
> +   **/
> +  getExecutablesOffsets: function (url, onlyLine){
> +    let offsets = new Set();
> +    for (let s of this.threadActor.dbg.findScripts(this.threadActor.global)) {
> +      if(s.url === url){

Style nit: spaces before the ( and after the )

Comment 23

[Seris]

Attachment: bug-795917-fix.patch

Comment 24

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/fx-team/rev/81f24d931d89

Comment 25

[Eddy Bruel [:ejpbruel]]

Hi Serris,

Resolving a bug is usually not done by us, but by the tree sherrifs once they've confirmed the patch doesn't cause tests to fail on the tree.

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/81f24d931d89

Comment 27

[Eddy Bruel [:ejpbruel]]

Congratulations on landing your first patch Serris! Sorry it took so long. I hope to see more of your work in the future.