878308 - Continue to given line

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

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

Should be able to right click a line of code in the debugger while we are paused and choose a "continue to this line" type option in the context menu.

This should set a temporary breakpoint at the selected line that deletes itself when it is hit, and then resume the debugger.

Chrome has this feature and it is cool, we should have parity.

Comment 1

[Dave Camp (:dcamp)]

Does that disable other breakpoints until that temporary breakpoint is set?

Comment 2

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

(In reply to Dave Camp (:dcamp) from comment #1)
> Does that disable other breakpoints until that temporary breakpoint is set?

In Chrome, if you hit an intermediate breakpoint on the way to the temporary breakpoint created by "continue to here", the temporary bp is cleared and you will not stop at that line after resuming from the intermediate bp.

I'm not sure this is the behavior we want, though. It isn't clear to me what is best/most intuitive, because I had assumed that you would still stop at the temporary bp until I tried it.

Comment 3

[Rob Campbell [:rc] (:robcee)]

This sounds like a nice feature.

I think it would be simplest to continue as normal until the temporary breakpoint is hit. I.e., respect intervening breakpoints along the way. It might be the least surprising behavior.

Comment 4

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

One other thing I want to note is that they don't actually show the breakpoint on the line number or give the user any idea that breakpoints are how the feature is implemented behind the scenes. In fact, maybe it isn't implemented that way, I just assumed (it does seem most likely though).

Comment 5

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

Willing to mentor someone on this, as I'd like to see it happen but don't have time to do it myself. Person who takes this should probably have a debugger patch or two under their belt before trying this one.

Comment 6

[Jonathan [:elefont]]

I'd like to summarize the main points of a conversation I had with Nick regarding potential implementations for this.  

1. We take a purely client side approach by having the user set a breakpoint (which is also hidden from the user), and once control flow hits that new breakpoint, it is automatically removed.

The advantage of this approach is that it keeps the RDP simple. However, the downside is that we would need to somehow hide these breakpoints (i.e. not allow them to appear in the gutter). A second con with this approach is that we would need a second request to delete the breakpoint once control flow actually hits it.

2. We move the logic to the back-end, which would require a new type of breakpoint request over RDP. 

The advantage here is that the server would automatically clean up the breakpoint for us, so we could eliminate the second request mentioned in Option 1. However, we would need to create another RDP request type. 


Per Nick's recommendation, we should get input from Jim and Panos to see what they think.

Comment 7

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

I am in favor of option 1, since we have the primitives we can combine to create this feature already.

Does anyone feel differently?

Comment 8

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

I think overall option 2 would be easier to implement, but probably adding another very specific protocol request isn't worth the additional server complexity. Option 1 is not that hard either, and has the additional advantage that you don't need to write extra unit tests for the server part. So my vote is for option 1 as well.

Comment 9

[Brian Burg]

FWIW, option 2 is what WebKit (and probably Blink) does. As long as the frontend is capable of handling pauses at arbitrary lines, implementing the backend with breakpoints is quite straightforward.

Comment 10

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

(In reply to Brian Burg from comment #9)
> FWIW, option 2 is what WebKit (and probably Blink) does. As long as the
> frontend is capable of handling pauses at arbitrary lines, implementing the
> backend with breakpoints is quite straightforward.

Thanks for the info, Brian :)

Comment 11

[Jim Blandy :jimb]

I think both could work well.

How we deal with hitting intervening breakpoints is an interesting question.

- If the "run-to-line" request remains in force when we hit an intervening breakpoint, then that invisible temporary breakpoint could last a long time: the user could do all kinds of inspection, evals, or hit other breakpoints in the meantime. The user could even make another run-to-line request while the first one remained unsatisfied. If state is going to hang around for a long time (like this run-to-line request), then it should not be invisible. Perhaps we could call it "set one-shot breakpoint and continue", and then list the breakpoint with the others, along with a "one-shot" marker? In any case, if we do this, then it seems to me that handling it on the client is best.

- If the "run-to-line" request is forgotten when we pause early, then it would indeed be easier to have the server clean up the state automatically; the client could send a resume packet with a resumeLimit of { type:"continue-to-line", location:<location> }, and the server would respond with { from:<thread>, type:"paused", why: { "type":"continue-to-line" }, ... }. So for the client, it would be a send-and-forget kind of thing.

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Johnathan, are you still working on this bug? It's fine if you are! :) Just double checking, in case others may want to give it a shot.

Comment 13

[Jonathan [:elefont]]

Ryan, while I'd like to work on this ticket, I'm afraid I don't have time for it anymore. It'd be best to assign it to someone else.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: WIP v1

TODO:

* Rename to "Continue to this location"
* Docs
* Tests

Comment 15

[Victor Porof [:vporof][:vp]]

Comment on attachment 8365496 [details] [diff] [review]
WIP v1

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

I have a few small questions.

::: browser/devtools/debugger/debugger-panes.js
@@ +918,2 @@
>      let url = DebuggerView.Sources.selectedValue;
>      let line = DebuggerView.editor.getCursor().line + 1;

Is it really worth drying this up for just two lines? I don't think so, but I don't feel strongly about it.

@@ +958,5 @@
> +    if (DebuggerController.activeThread.paused) {
> +      let location = this._getLocationOfSelectedLine();
> +      location.column = DebuggerView.editor.getCursor().ch;
> +      let warn = DebuggerController._ensureResumptionOrder;
> +      DebuggerController.StackFrames.currentFrameDepth = -1;

We're setting currentFrameDepth to -1 and accessing _ensureResumptionOrder in an increasing number of places now, which scares me a bit. How about creating a more standard method in DebuggerController.StackFrames that deals with stepping/resuming/etc. and calling that everywhere instead.

::: browser/locales/en-US/chrome/browser/devtools/debugger.dtd
@@ +159,5 @@
>  
> +<!-- LOCALIZATION NOTE (debuggerUI.seMenuContinueToLine): This is the text that
> +  -  appears in the source editor context menu for continuing execution to the
> +  -  selected line. -->
> +<!ENTITY debuggerUI.seMenuContinueToLine "Continue to this line">

Can't find an accesskey? Or maybe you don't think it's necessary. Could make a nice power-user feature.

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Your feedback makes sense to me...  I've been a bit hesitant because I haven't convinced myself that my current approach is worth it.

I'm currently going down the server-side route, but I think it might be better to do this client-side as some of the comments from others on the team suggest.

I'll play with this again soon and see what's involved in the client-side approach.

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

It may be a while before I look at this again...  Someone else can pick it up if they'd like to.

Comment 18

[Girish Sharma [:Optimizer]]

IMO, if the feature goes like "Create a temporary breakpoint at this line, that hits one time, but other breakpoints also hit normally", then it is not adding much value to the current possible case - "Just add a breakpoint here, remove it when hit".

My use case to use such a feature would be to forget about other breakpoints completely until I hit this line. Because, if I am telling the UI to continue till this line, most probably it does not mean that continue to the next breakpoint, whatever line it is on.

An examples of my use case :

// debugger is paused somewhere before this line. (or not at all)
doSomething().then(() => {
  // I want to break in this method
  var a = foo(); // debugger;
});

The current way I debug the callback method is to add a breakpoint, or uncomment the debugger; statement.

But I really find it irritating for many cases, where I dont want to hit breakpoints in doSomething method.

Comment 19

[James Long (:jlongster)]

(In reply to Girish Sharma [:Optimizer] from comment #18)
> IMO, if the feature goes like "Create a temporary breakpoint at this line,
> that hits one time, but other breakpoints also hit normally", then it is not
> adding much value to the current possible case - "Just add a breakpoint
> here, remove it when hit".

I had written a comment saying that that behavior is useful before you posted, but it's hard to say. Always having to remove a breakpoint can be annoying too. I think it just depends on how you intend to use this feature.

It could go either way in my opinion. What if we ignored all other breakpoints, and also added a "temporary" breakpoint type? Or is that just getting too complex?

Comment 20

[Girish Sharma [:Optimizer]]

(In reply to James Long (:jlongster) from comment #19)
> (In reply to Girish Sharma [:Optimizer] from comment #18)
> > IMO, if the feature goes like "Create a temporary breakpoint at this line,
> > that hits one time, but other breakpoints also hit normally", then it is not
> > adding much value to the current possible case - "Just add a breakpoint
> > here, remove it when hit".
> 
> I had written a comment saying that that behavior is useful before you
> posted, but it's hard to say. Always having to remove a breakpoint can be
> annoying too. I think it just depends on how you intend to use this feature.

Umm, where is your comment ? :)

I take this feature as "Resume and next time break only at this point (if possible)".

If this feature was "Add a one shot breakpoint at this line, forget that breakpoint if any other breakpoint is hit in b/w", then it will lead me to either:

 - Set a normal breakpoint at that line after 2 - 3 attempts of trying to use "Continue to this line" and not actually continuing to that line.
 - Get frustrated because even though I said continue to this line, I am being switched to other scripts and other lines, after which, I accidentally pressed resume in hope of that the previously set "continue to this line" would work, but it did not.

Then again, its my take on the definition of "Continue to line", that I personally would like to be implemented.

Also, if we go ahead and implement the 

> It could go either way in my opinion. What if we ignored all other
> breakpoints, and also added a "temporary" breakpoint type? Or is that just
> getting too complex?

Why do you say "... breakpoints, *and* also added ..." ?

Comment 21

[James Long (:jlongster)]

(In reply to Girish Sharma [:Optimizer] from comment #20)
> (In reply to James Long (:jlongster) from comment #19)
> > (In reply to Girish Sharma [:Optimizer] from comment #18)
> > > IMO, if the feature goes like "Create a temporary breakpoint at this line,
> > > that hits one time, but other breakpoints also hit normally", then it is not
> > > adding much value to the current possible case - "Just add a breakpoint
> > > here, remove it when hit".
> > 
> > I had written a comment saying that that behavior is useful before you
> > posted, but it's hard to say. Always having to remove a breakpoint can be
> > annoying too. I think it just depends on how you intend to use this feature.
> 
> Umm, where is your comment ? :)

It conflicted and I thought I'd rather just respond to yours. It wasn't long, just saying that I would interpret this as "try getting to this line, but break on anything in between".

> 
> > It could go either way in my opinion. What if we ignored all other
> > breakpoints, and also added a "temporary" breakpoint type? Or is that just
> > getting too complex?
> 
> Why do you say "... breakpoints, *and* also added ..." ?

I meant that we could make this bug ignore all intermediate breakpoints, and add another feature that a "one-shot" breakpoint of some sort. I don't really like that though, it feels like bloat.

I'm fine ignoring other breakpoints, I don't have a good enough feeling of which to choose. We can always change it later if people complain.

Comment 22

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

The main use case that comes to my mind when I think about this is figuring out the code paths taken in a complex, unknown codebase. Say I wonder if a callback is being called in a long, complex flow. I can put a breakpoint in it and then continue to a line where the flow concludes. If my breakpoint is hit, then I am right at the place where I need to be and I can inspect the stack to see how I got there.

Being compatible with Chrome is also a useful feature in my view. Users may be already familiar with this behavior and without a clear benefit we shouldn't try to break their expectations.

(In reply to Girish Sharma [:Optimizer] from comment #18)
> My use case to use such a feature would be to forget about other breakpoints
> completely until I hit this line. Because, if I am telling the UI to
> continue till this line, most probably it does not mean that continue to the
> next breakpoint, whatever line it is on.

You could do that easily with one additional click to disable all breakpoints before continuing to that line. I think it's a minor inconvenience to enable an additional use case, considering there is no simple workaround for that other case if we always ignore breakpoints.

Comment 23

[Brian Burg]

(In reply to Panos Astithas [:past] from comment #22)
> The main use case that comes to my mind when I think about this is figuring
> out the code paths taken in a complex, unknown codebase. Say I wonder if a
> callback is being called in a long, complex flow. I can put a breakpoint in
> it and then continue to a line where the flow concludes. If my breakpoint is
> hit, then I am right at the place where I need to be and I can inspect the
> stack to see how I got there.

I suppose you could address the user scenario by reimplementing this feature. But, even if the semantics were clear, that workflow is still tedious and repetitive. IMO (as someone mostly on the periphery), I see the absence of this feature as an opportunity to rethink user workflows and design something more comprehensive and intuitive for that scenario.

> Being compatible with Chrome is also a useful feature in my view. Users may
> be already familiar with this behavior and without a clear benefit we
> shouldn't try to break their expectations.

I would be surprised if a non-trivial number of people used this feature. Even fewer users may truly understand it, because the semantics are unclear. If the implementors cannot agree on the behavior, I'm skeptical anyone less pedantic would risk their time on it.

Comment 24

[SuryaPavan]

Hey Nick,

I am a newbie and interested to work on this bug. With some pointers I can move ahead. Let me know if I can go ahead.

Thanks,
SuryaPavan

Comment 25

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

(In reply to SuryaPavan from comment #24)
> Hey Nick,
> 
> I am a newbie and interested to work on this bug. With some pointers I can
> move ahead. Let me know if I can go ahead.
> 
> Thanks,
> SuryaPavan

I'm not sure that we have resolved the outstanding issues of whether to implement this on the client side or server side, and what the desired behavior of intervening pauses, yet. Furthermore, this isn't really a good newbie bug, but more of a good bug for someone who is already somewhat familiar with the debugger code. Because of these things, I don't think this is a good bug for you to pick up at this time. Perhaps there is a different one you can work on to get some debugger experience and until we figure these things out.

------------------------------------------------------------------

Re-reading the discussion, I like what Jim says in favor of implementing it server-side in comment 11. Fire and forget seems nice.

I also think, since there isn't a clear winner on intervening pause behavior, we should just mimic chrome's and webkit's behavior.

Can we agree on these points?

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #25)
> Re-reading the discussion, I like what Jim says in favor of implementing it
> server-side in comment 11. Fire and forget seems nice.
> 
> I also think, since there isn't a clear winner on intervening pause
> behavior, we should just mimic chrome's and webkit's behavior.
> 
> Can we agree on these points?

I am fine with this assessment. :)

Comment 27

[James Long (:jlongster)]

I am fine with it too. No reason this should stall on a minor usage disagreement; if other behavior really is better we can always push to change it later.

Comment 28

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

I agree as well.

Comment 29

[Girish Sharma [:Optimizer]]

I agree as well. We can go ahead and make the behavior as

"add a one shot breakpoint at this line but *do* stop at other breakpoints if they are hit before. If that happens (or even if that does not happen and we stop only at this one shot breakpoint), remove this one shot breakpoint"

I would also like to say that these one shot breakpoints should not be disabled when we disable all breakpoints after creating this one shot breakpoint to achieve the following behavior:

"Stop only at this line forgetting about all other breakpoints"

Comment 30

[:Harald Kirschner :digitarald]

Closing this. Work landed in https://github.com/devtools-html/debugger.html/pull/3964