719122 - Setting a breakpoint in a line without code should skip forward

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

When a protocol client sets a breakpoint in a line that has no code, the debugger should automatically skip forward to the next line that has. If there are none, an error should be returned to the client. Currently such cases fail silently, while returning success.

Comment 1

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

Attachment: WIP

Work in progress. I seem to get weird offsets from SpiderMonkey. Need to investigate further.

Comment 2

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

Attachment: WIP 2

Figured out the offsets, added a test that passes with the solution in this patch. Need to take care of more use cases though, like inline scripts containing multiple function declarations with comments and setting a breakpoint in a comment.

Comment 3

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

Attachment: WIP 3

I've got it fully working now, fixed a protocol-related TODO (returning the actual location when skipping forward) and added more tests. I'll see if I can get even more corner cases covered and submit for review afterwards.

Comment 4

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

Attachment: Working patch

I added lots of new tests, for every bug I hit during testing. Breakpoint-related functionality should be fairly robust now.

Comment 5

[Dave Camp (:dcamp)]

Comment on attachment 591521 [details] [diff] [review]
Working patch

This looks ok to me at first glance, but I'd like jimb's opinion.

Comment 6

[Jim Blandy :jimb]

Comment on attachment 591521 [details] [diff] [review]
Working patch

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

Do the tests somewhere check that a breakpoint set at a line with multiple entry points triggers no matter which entry point we reach?

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +322,5 @@
> +        codeFound = true;
> +      } catch (e) {
> +        // setBreakpoint throws for invalid offsets.
> +        Cu.reportError(e);
> +      }

That try/catch around the setBreakpoint call is... surprising. Does s.getLineOffsets really return offsets at which s.setBreakpoint refuses to set a breakpoint? That seems perverse; what's the point of an API that returns bytecode offsets that can't be used? If you have an example, let's file a bug for that. Otherwise, let's dump the try/catch.

@@ +335,5 @@
> +          for (let i = 0; i < lines[line].length; i++) {
> +            try {
> +              script.setBreakpoint(lines[line][i], bpActor);
> +              actualLocation = location;
> +              actualLocation.line = line;

This mutates the original 'location' object's 'line' property, isn't that so? Is the original no longer needed?

@@ +344,5 @@
> +            }
> +          }
> +          break;
> +        }
> +      }

This process of searching forward for the next offset is... ugly. That API really needs to be re-designed so that operations like this are straightforward. It'll need to be re-worked anyway to support column numbers, so this is doubly due.

@@ +356,1 @@
>      return packet;

Why not just "return { actor: ... };"?

@@ +364,5 @@
> +   *        The source script.
> +   * @param aLine number
> +   *        The line number.
> +   */
> +  _getInnermostContainer: function TA__getInnermostContainer(aScript, aLine) {

The algorithm this function implements is the only way, at present, to find the innermost script. However, it's an ugly API, and I hope we'll improve it in the future.

@@ +365,5 @@
> +   * @param aLine number
> +   *        The line number.
> +   */
> +  _getInnermostContainer: function TA__getInnermostContainer(aScript, aLine) {
> +    if (aScript.getChildScripts().length > 0) {

script.getChildScripts returns a fresh array each time it's called; you should call it once and save the result.

@@ +369,5 @@
> +    if (aScript.getChildScripts().length > 0) {
> +      for (let i = 0; i < aScript.getChildScripts().length; i++) {
> +        let child = aScript.getChildScripts()[i];
> +        // Stop when the first script that contains this location is found.
> +        if (child.startLine < aLine &&

Not <=?

Comment 7

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

(In reply to Jim Blandy :jimb from comment #6)
> Comment on attachment 591521 [details] [diff] [review]
> Working patch
> 
> Review of attachment 591521 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do the tests somewhere check that a breakpoint set at a line with multiple
> entry points triggers no matter which entry point we reach?

Can't seem to find one, so I might as well add one. 

> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +322,5 @@
> > +        codeFound = true;
> > +      } catch (e) {
> > +        // setBreakpoint throws for invalid offsets.
> > +        Cu.reportError(e);
> > +      }
> 
> That try/catch around the setBreakpoint call is... surprising. Does
> s.getLineOffsets really return offsets at which s.setBreakpoint refuses to
> set a breakpoint? That seems perverse; what's the point of an API that
> returns bytecode offsets that can't be used? If you have an example, let's
> file a bug for that. Otherwise, let's dump the try/catch.

The original reason for putting the try/catch in there was that I hadn't figured out how child scripts worked yet, and I used the offsets from the wrong script to the setBreakpoint call, which caused it to throw. It's sole purpose now is to guard against such bugs in the debugger, but if, after having looked at this code, you feel confident it's not necessary any more, I can remove it.

> @@ +335,5 @@
> > +          for (let i = 0; i < lines[line].length; i++) {
> > +            try {
> > +              script.setBreakpoint(lines[line][i], bpActor);
> > +              actualLocation = location;
> > +              actualLocation.line = line;
> 
> This mutates the original 'location' object's 'line' property, isn't that
> so? Is the original no longer needed?

You are right, it does modify the argument in order to avoid creating a helper method for deep copying the location object that can handle all three of its currently specified forms. At that point we have no more use for the original value, but your question helped me observe that I pointlessly reset actualLocation for each offset in that line, instead of setting it once after the inner loop.

> @@ +344,5 @@
> > +            }
> > +          }
> > +          break;
> > +        }
> > +      }
> 
> This process of searching forward for the next offset is... ugly. That API
> really needs to be re-designed so that operations like this are
> straightforward. It'll need to be re-worked anyway to support column
> numbers, so this is doubly due.
> 
> @@ +356,1 @@
> >      return packet;
> 
> Why not just "return { actor: ... };"?

I think this used to include the "from" field as well, which caused it to cross the 80th column and need to be wrapped per the code style guidelines. I removed "from", since it's added automagically, but I neglected to inline packet.

> @@ +364,5 @@
> > +   *        The source script.
> > +   * @param aLine number
> > +   *        The line number.
> > +   */
> > +  _getInnermostContainer: function TA__getInnermostContainer(aScript, aLine) {
> 
> The algorithm this function implements is the only way, at present, to find
> the innermost script. However, it's an ugly API, and I hope we'll improve it
> in the future.
> 
> @@ +365,5 @@
> > +   * @param aLine number
> > +   *        The line number.
> > +   */
> > +  _getInnermostContainer: function TA__getInnermostContainer(aScript, aLine) {
> > +    if (aScript.getChildScripts().length > 0) {
> 
> script.getChildScripts returns a fresh array each time it's called; you
> should call it once and save the result.

OK. Another micro-optimization I shied away from is that we could avoid one recursive invocation if we checked for the existence of child scripts right before recursing, so in effect trading two consecutive checks for child scripts in the intermediate steps versus one less function call when we reach the bottom. Do you think this is also worth it, or would you prefer keeping the function simpler to understand?

> @@ +369,5 @@
> > +    if (aScript.getChildScripts().length > 0) {
> > +      for (let i = 0; i < aScript.getChildScripts().length; i++) {
> > +        let child = aScript.getChildScripts()[i];
> > +        // Stop when the first script that contains this location is found.
> > +        if (child.startLine < aLine &&
> 
> Not <=?

Oops, adding a test for this!

Comment 8

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #7)
> (In reply to Jim Blandy :jimb from comment #6)
> The original reason for putting the try/catch in there was that I hadn't
> figured out how child scripts worked yet, and I used the offsets from the
> wrong script to the setBreakpoint call, which caused it to throw. It's sole
> purpose now is to guard against such bugs in the debugger, but if, after
> having looked at this code, you feel confident it's not necessary any more,
> I can remove it.

Well, if you take it out, and something goes wrong, then it'll throw, which is what we want, I think, so that seems like the best thing. Do we have an uncaughtExceptionHook that displays such errors to the developer?

> > script.getChildScripts returns a fresh array each time it's called; you
> > should call it once and save the result.
> 
> OK. Another micro-optimization I shied away from is that we could avoid one
> recursive invocation if we checked for the existence of child scripts right
> before recursing, so in effect trading two consecutive checks for child
> scripts in the intermediate steps versus one less function call when we
> reach the bottom. Do you think this is also worth it, or would you prefer
> keeping the function simpler to understand?

I'd take the simplicity, definitely. I just brought up getChildScripts because it's quadratic behavior: one allocation of the child list for each child.

r=me with those changes.

Comment 9

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

Attachment: [in-fx-team] Working patch v2

Made all requested changes, added test_breakpoint-10.js for lines with multiple entry points and modified test_breakpoint-09.js to be sensitive to my broken range conditional check in TA__getInnermostContainer.

Try run:

https://tbpl.mozilla.org/?tree=Try&rev=817863933e76

(In reply to Jim Blandy :jimb from comment #8)
> Well, if you take it out, and something goes wrong, then it'll throw, which
> is what we want, I think, so that seems like the best thing. Do we have an
> uncaughtExceptionHook that displays such errors to the developer?

We do have a hook that dumps the error in the console (stdout) currently and it will be changed to log it into the error console soon.

Comment 10

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

https://hg.mozilla.org/integration/fx-team/rev/55243908fbdd

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/55243908fbdd