break del command suffers from an off by 1 error

RESOLVED FIXED in Firefox 24

Status

RESOLVED FIXED
6 years ago
3 months ago

People

(Reporter: rcampbell, Assigned: jwalker)

Tracking

unspecified
Firefox 24
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
STR:

Go to a page, set three breakpoints.

In the commandline, run "break del 3".

Error returned, breakpoint not found.

break del 2 removes breakpoint 3.
(Reporter)

Comment 1

6 years ago
Created attachment 706064 [details] [diff] [review]
break del by one WIP

fixing the tests which don't run. Still working on it.
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
(Reporter)

Comment 2

6 years ago
de-assigning while the unittests get fixed up.
Assignee: rcampbell → nobody
Status: ASSIGNED → NEW
Assigning to me so I don't forget
Assignee: nobody → jwalker
Created attachment 747408 [details] [diff] [review]
v1

The short version is: We were not passing the context around to several places that we should be.

There is a context for the 'exec' of a function. It tells you stuff like the current target, what's on the command line, the current document/window, etc

    gcli.addCommand({
      name: "foo",
      exec: function(args, context) {
        // ...
      }
    });

The same context *should* be available to functions that define parameters, but was missing:

    gcli.addCommand({
      name: "break add line",
      params: [
        {
          name: "file",
          type: {
            name: "selection",
            data: function(context) {
              // return a list of the JS files in the current page
              // we can get the debugger using context.environment.target
            }
          },
        },
        ...
      ],
      exec: function(args, context) {
        // ...
      }
    });

The same goes for max/min functions to numbers:

    gcli.addCommand({
      name: "break del",
      params: [
        {
          name: "breakid",
          type: {
            name: "number",
            min: 0,
            max: function(context) {
              // return the max breakpoint id
              // we can get the debugger using context.environment.target
            },
          },
        }
      ],
      exec: function(args, context) {
        // ...
      }
    });

This patch is basically just passing 'context' to places that need it, and updating the tests to fix some cleanup and bitrot issues.

It works on my machine. I've not pushed to try yes, but will do.
Attachment #706064 - Attachment is obsolete: true
Attachment #747408 - Flags: review?(rcampbell)
(Reporter)

Comment 5

5 years ago
this doesn't apply cleanly.
(Reporter)

Updated

5 years ago
Attachment #747408 - Flags: review?(rcampbell)
Didn't apply cleanly because it depends on bug 839862.
Depends on: 839862
Created attachment 748871 [details] [diff] [review]
v2

I decided that I didn't like using a number as an identifier for a breakpoint so I changed it to use a label, which should be way nicer.
Also beefed up the unit tests a bit.
Attachment #747408 - Attachment is obsolete: true
Attachment #748871 - Flags: review?(rcampbell)
(In reply to Joe Walker [:jwalker] from comment #7)

I'm rather confused as to what should be passed as an argument to "break del". Why is using an index a bad thing? It looks to me like it's faster than typing anything else.

While dealing with multiple files may currently be awkward with indices, I think a better solution to such scenarios would be a completion list "like break list" (we also have a remove button in the output there, which this patch seems to break).
(In reply to Victor Porof [:vp] from comment #9)
> (In reply to Joe Walker [:jwalker] from comment #7)
> 
> I'm rather confused as to what should be passed as an argument to "break
> del". Why is using an index a bad thing? It looks to me like it's faster
> than typing anything else.

Using numbers relies on you have *just* typed "break list" and then counted down. Creating or removing a breakpoint from the UI will change the list so your memory could be useless.

Plus there's completion to help you type the new labels, and it's obvious what you're going to do, which you can't say for an index.

> While dealing with multiple files may currently be awkward with indices, I
> think a better solution to such scenarios would be a completion list "like
> break list" (we also have a remove button in the output there, which this
> patch seems to break).

I did just notice a bug with a page that ended with a '/' which I'll fix, but the remove button WFM. What page do you see it on?
The perpetual debugger debugging page http://htmlpad.org/debugger/ on debugger/. It'd be nice if the labels used in "break del" would be synced with what's shown in the sources list. SourceUtils has a method for that. I'd also be really happy if the documentation for the <breakpoint> param would be updated :) I imagine some cases may be unintuitive.
(In reply to Joe Walker [:jwalker] from comment #10)
> 
> Plus there's completion to help you type the new labels, and it's obvious
> what you're going to do, which you can't say for an index.
> 

Completion doesn't seem to work for me. Same test page as previous comment.
(In reply to Victor Porof [:vp] from comment #11)
> The perpetual debugger debugging page http://htmlpad.org/debugger/ on
> debugger/. It'd be nice if the labels used in "break del" would be synced
> with what's shown in the sources list. SourceUtils has a method for that.

I was using it, but had troubles:
* SourceUtils hard truncates at 20 chars for the sources list, and I can see why, but there's no hard 20 char limit on the cli
* this might not be a problem except that it prepends an elipsis, which is hard to type
* and because the truncate is hard at 20 chars, it gives you some strange labels "...ite_long_filename.js"

Now I considered altering the sources list, but that didn't seem right either. Eventually I just thought that they had different requirements.

> I'd also be really happy if the documentation for the <breakpoint> param
> would be updated :) I imagine some cases may be unintuitive.

Good point, thanks.
(In reply to Joe Walker [:jwalker] from comment #13)
> 

Just to make sure we're talking about the same thing, I was referring to SourceUtils.getSourceLabel. In which case you can easily add a noTrim option to that method.

> Now I considered altering the sources list, but that didn't seem right
> either. Eventually I just thought that they had different requirements.

I'm glad you avoided altering the sources list :)

What I'm trying to say is that some values for the <breakpoint> param may be frustratingly confusing and very hard to cover in a short documentation. It's a bad idea to assume labels mean one thing in the debugger's sources list and something else in gcli. What do you do about foo/ for example? foo/:23 like everywhere in the debugger, or /:23 with a different implementation? What ?about=request&params? What about #references? And if we're going for making things look and act like in the debugger's sources list, why not reuse the methods in SourceUtils?
(In reply to Victor Porof [:vp] from comment #14)
> (In reply to Joe Walker [:jwalker] from comment #13)
> > 
> 
> Just to make sure we're talking about the same thing, I was referring to
> SourceUtils.getSourceLabel. In which case you can easily add a noTrim option
> to that method.
> 
> > Now I considered altering the sources list, but that didn't seem right
> > either. Eventually I just thought that they had different requirements.
> 
> I'm glad you avoided altering the sources list :)
> 
> What I'm trying to say is that some values for the <breakpoint> param may be
> frustratingly confusing and very hard to cover in a short documentation.
> It's a bad idea to assume labels mean one thing in the debugger's sources
> list and something else in gcli. What do you do about foo/ for example?
> foo/:23 like everywhere in the debugger, or /:23 with a different
> implementation? What ?about=request&params? What about #references? And if
> we're going for making things look and act like in the debugger's sources
> list, why not reuse the methods in SourceUtils?

So, with completion then you shouldn't need documentation, and worst case all you need to do is to call 'break list' to see the labels.

How would I call SourceUtils to avoid the problems in comment 13? I'm not against using it (in fact I'd prefer to use it) but wasn't aware that I could use it for what we needed.
(In reply to Joe Walker [:jwalker] from comment #15)
> 
> So, with completion then you shouldn't need documentation, and worst case
> all you need to do is to call 'break list' to see the labels.
> 

Agreed, but let's make sure completion works first. Adding a breakpoint on htmlpad.org/debugger on line 13 and then doing |break del| -> tab/enter/whatever doesn't seem to do anything.

> How would I call SourceUtils to avoid the problems in comment 13? I'm not
> against using it (in fact I'd prefer to use it) but wasn't aware that I
> could use it for what we needed.

Just SourceUtils.getSourceLabel(url) is enough.
Created attachment 749193 [details] [diff] [review]
v3

This uses SourceUtils, and WFM on http://htmlpad.org/debugger/ will post images showing what I think you should get.

I r?vporof just because he's been doing most of the reviewing so far. Happy either way.
Attachment #748871 - Attachment is obsolete: true
Attachment #748871 - Flags: review?(rcampbell)
Attachment #749193 - Flags: review?(vporof)
Comment on attachment 749193 [details] [diff] [review]
v3

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

This looks good, just a few small changes below for r+. I did manage to get the completion popup and output from another command to overlap, but I guess this is a different bug and not related to this patch.
1. break list
2. Click "Remove" on an entry
3. Click on the gcli input

::: browser/devtools/debugger/CmdDebugger.jsm
@@ +38,5 @@
> +
> +  for (let source in dbg.panelWin.DebuggerView.Sources) {
> +    for (let { attachment: breakpoint } in source) {
> +      let label = SourceUtils.getSourceLabel(source.value)
> +                                    + ":" + breakpoint.lineNumber;

You know what? You can actually use source.label here, since it's already stored, instead of going through all the SourceUtils.getSourceLabel shenanigans. I've been blind, sorry.

So:
> let label = source.label + ":" + breakpoint.lineNumber;

@@ +47,5 @@
> +        url: source.value,
> +        lineNumber: breakpoint.lineNumber,
> +        lineText: breakpoint.lineText,
> +        truncatedLineText: SourceUtils.trimUrlLength(breakpoint.lineText,
> +                                                  MAX_LINE_TEXT_LENGTH, "end")

Uber nit: defining this as a variable before the object literal would avoid bending whitespace because of timeless wrapping rules.

@@ +52,5 @@
> +      };
> +    }
> +  }
> +
> +  return Object.keys(breakpoints).map(key => { return breakpoints[key]; });

Why go through all this trouble when you can create an array in the first place? I don't see the benefit of populating an object then turning it into an array, but maybe I'm missing something.

@@ +225,5 @@
>      if (!dbg) {
>        return gcli.lookup("debuggerStopped");
>      }
>  
>      let breakpoints = dbg.getAllBreakpoints();

CmdDebugger.jsm is using both dbg.getAllBreakpoints and getAllBreakpoints. I would say this is a recipe for disaster.

In this particular case, a much more elegant solution would be to use the debugger's public breakpoint API:

let breakpoint = dbg.getBreakpoint(args.breakpoint.url, args.breakpoint.lineNumber);
Attachment #749193 - Flags: review?(vporof)
(In reply to Victor Porof [:vp] from comment #19)
> Comment on attachment 749193 [details] [diff] [review]
> v3
> 
> Review of attachment 749193 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, just a few small changes below for r+. I did manage to get
> the completion popup and output from another command to overlap, but I guess
> this is a different bug and not related to this patch.
> 1. break list
> 2. Click "Remove" on an entry
> 3. Click on the gcli input

Sigh - more fighting with XUL panels. JSTerm integration is forever on the horizon, so these problems keep getting pushed back. Maybe it's time to think about fixing bugs twice. :(

Rob - there are a bunch of issues like this, which is one of the reasons that I'm keen on getting going with JSTerm.

> ::: browser/devtools/debugger/CmdDebugger.jsm
> @@ +38,5 @@
> > +
> > +  for (let source in dbg.panelWin.DebuggerView.Sources) {
> > +    for (let { attachment: breakpoint } in source) {
> > +      let label = SourceUtils.getSourceLabel(source.value)
> > +                                    + ":" + breakpoint.lineNumber;
> 
> You know what? You can actually use source.label here, since it's already
> stored, instead of going through all the SourceUtils.getSourceLabel
> shenanigans. I've been blind, sorry.
> 
> So:
> > let label = source.label + ":" + breakpoint.lineNumber;

Done.

> @@ +47,5 @@
> > +        url: source.value,
> > +        lineNumber: breakpoint.lineNumber,
> > +        lineText: breakpoint.lineText,
> > +        truncatedLineText: SourceUtils.trimUrlLength(breakpoint.lineText,
> > +                                                  MAX_LINE_TEXT_LENGTH, "end")
> 
> Uber nit: defining this as a variable before the object literal would avoid
> bending whitespace because of timeless wrapping rules.

Nit in your Uber nit. No it wouldn't, I tried that ;-)

      var t = SourceUtils.trimUrlLength(breakpoint.lineText, MAX_LINE_TEXT_LENGTH, "end");

Is 91 chars in length

> @@ +52,5 @@
> > +      };
> > +    }
> > +  }
> > +
> > +  return Object.keys(breakpoints).map(key => { return breakpoints[key]; });
> 
> Why go through all this trouble when you can create an array in the first
> place? I don't see the benefit of populating an object then turning it into
> an array, but maybe I'm missing something.

This is a hangover from part of the original point of the code - to ensure that breakpoint.label is unique.

> @@ +225,5 @@
> >      if (!dbg) {
> >        return gcli.lookup("debuggerStopped");
> >      }
> >  
> >      let breakpoints = dbg.getAllBreakpoints();
> 
> CmdDebugger.jsm is using both dbg.getAllBreakpoints and getAllBreakpoints. I
> would say this is a recipe for disaster.
> 
> In this particular case, a much more elegant solution would be to use the
> debugger's public breakpoint API:
> 
> let breakpoint = dbg.getBreakpoint(args.breakpoint.url,
> args.breakpoint.lineNumber);

Ah - much better.
Created attachment 750428 [details] [diff] [review]
v4
Attachment #749193 - Attachment is obsolete: true
Attachment #750428 - Flags: review?(vporof)
Comment on attachment 750428 [details] [diff] [review]
v4

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

Smashing!
Attachment #750428 - Flags: review?(vporof) → review+
https://hg.mozilla.org/mozilla-central/rev/873bf55af0cd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.