812869 - Breakpoint clients sometimes don't know how to remove themselves

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

STR:
1. Go to http://htmlpad.org/debugger/ and start the debugger
2. Add a breakpoint on line 8 (the function declaration)
3. The breakpoint got placed on line 9 (which is correct)
4. Add a breakpoint again on line 8
5. Click me

Now it takes 2 resumes to step over line 9.

This didn't happen until now, because in debugger-controller.js@BP_addBreakpoint we have:

> // Prevent this new breakpoint from being repositioned on top of an
> // already existing one.
> if (this.getBreakpoint(url, line)) {
>   this._hideBreakpoint(aBreakpointClient);
>   aBreakpointClient.remove();
>   return;
> }

It seems that aBreakpointClient.remove() is indeed getting called, but has no effect. If my bisecting is correct, this started happening after bug 793214.

Comment 1

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

I haven't looked into it yet, but bug 793214 didn't touch the breakpoint removal path. Last time we touched that part was in bug 737803 IIRC. Also, shouldn't this._hideBreakpoint remove the existing breakpoint in the editor regardless of what happens in the server?

Comment 2

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

(In reply to Panos Astithas [:past] from comment #1)
> I haven't looked into it yet, but bug 793214 didn't touch the breakpoint
> removal path. Last time we touched that part was in bug 737803 IIRC. 

Well, all I can say is that it worked on the fx-team before a0c6d66368a3 or the 4639da479a93 merge. I'm not entirely sure what the cause is, but it certainly needs investigating.

Also,
> shouldn't this._hideBreakpoint remove the existing breakpoint in the editor
> regardless of what happens in the server?

No, it only hides it from the editor and the breakpints pane. Moreover, hideBreakpoint(aBreakpointClient) makes sure the breakpoint we placed in the wrong location doesn't remain there.

Comment 3

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

(In reply to Victor Porof [:vp] from comment #2)
> (In reply to Panos Astithas [:past] from comment #1)
> 
> Also,
> > shouldn't this._hideBreakpoint remove the existing breakpoint in the editor
> > regardless of what happens in the server?
> 
> No, it only hides it from the editor and the breakpints pane. Moreover,
> hideBreakpoint(aBreakpointClient) makes sure the breakpoint we placed in the
> wrong location doesn't remain there.

[I probably haven't answered your question :)]
The second breakpoint is, indeed, removed from the editor, as it should be. But it takes 2 resumes the get past the one placed correctly, one line below.

Comment 4

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

This issue disappeared in the current nightly using the steps described in comment 0, since line 8 (the function declaration) is now considered a valid breakpoint placement location. However, the problem is still reproducible by placing the breakpoint in the comment on lines 21-23. Thus:

1. Go to http://htmlpad.org/debugger/ and start the debugger
2. Add a breakpoint on line 22 (the comment)
3. The breakpoint got placed on line 24 (which is correct)
4. Add a breakpoint again on line 22
5. Click me

Comment 5

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

(In reply to Victor Porof [:vp] from comment #4)
> This issue disappeared in the current nightly using the steps described in
> comment 0, since line 8 (the function declaration) is now considered a valid
> breakpoint placement location. However, the problem is still reproducible by
> placing the breakpoint in the comment on lines 21-23. Thus:
> 
> 1. Go to http://htmlpad.org/debugger/ and start the debugger
> 2. Add a breakpoint on line 22 (the comment)
> 3. The breakpoint got placed on line 24 (which is correct)
> 4. Add a breakpoint again on line 22
> 5. Click me

Still happens for me. The breakpoint will not move to the next line of the response from the server is 'noScript'. That would have been the case if the script was GC'd before the debugger was open. Not sure why that would happen in this case though.

Comment 6

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

Possibly related to this issue:

1) In the same page, set a breakpoint at line 18
2) Remove the breakpoint from line 19 where it was moved
3) Click 'click me' and then resume once:

DBG-CLIENT: TypeError: breakpointClient is null: SF__onFrames@chrome://browser/content/debugger-controller.js:472
EV_notify@resource://gre/modules/devtools/dbg-client.jsm:145
TC_fillFrames/<@resource://gre/modules/devtools/dbg-client.jsm:946
DC_onPacket@resource://gre/modules/devtools/dbg-client.jsm:493
@chrome://global/content/devtools/dbg-transport.js:212

Comment 7

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

(In reply to Panos Astithas [:past] from comment #6)
> Possibly related to this issue:
> 
> 1) In the same page, set a breakpoint at line 18
> 2) Remove the breakpoint from line 19 where it was moved
> 3) Click 'click me' and then resume once:
> 
> DBG-CLIENT: TypeError: breakpointClient is null:
> SF__onFrames@chrome://browser/content/debugger-controller.js:472
> EV_notify@resource://gre/modules/devtools/dbg-client.jsm:145
> TC_fillFrames/<@resource://gre/modules/devtools/dbg-client.jsm:946
> DC_onPacket@resource://gre/modules/devtools/dbg-client.jsm:493
> @chrome://global/content/devtools/dbg-transport.js:212

It's indeed related. The breakpoint doesn't get removed at step 2 (I assume "add" means click on the gutter, and "remove" means clicking again on the gutter on line 19).

I filed bug 813697 to deal with this for now, but it seems that the client never gets removed from the Breakpoints.prototype.store map. This should have happened when calling aBreakpointClient.remove in BP_removeBreakpoint. It seems it *does* happen when the breakpoint is placed at a correct initial line, but it *doesn't* when the breakpoint is moved to an |actualLocation|. Which means the breakpoint is still hit, hence the TypeError you mentioned.

Comment 8

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

After some dumping around :), I arrived to the following analysis:

> 1) In the same page, set a breakpoint at line 18
A breakpoint tries to set itself on line 18 and correctly receives the |actualLocation| placement on line 19. It is displayed correctly in the editor and the client with the updated location is saved in the Breakpoints.prototype.store map

> 2) Remove the breakpoint from line 19 where it was moved
BP_removeBreakpoint gets called from BP__onEditorBreakpointRemove with the correct breakpoint client from the store map. The actor is verified and present in it, so aBreakpointClient.remove is invoked afterwards. Even the callback fires correctly and we perform the delete this.store[breakpointActor].

> 3) Click 'click me' and then resume once:
Even if the breakpoint registered as removed in the debugger frontend and it's not present anymore in the store map (the source of the TypeError described in comment 6), the breakpoint is still hit.

Comment 9

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

Attachment: Patch v1

Thankfully, this problem was confined to cases where the breakpoint would skip forward, but unfortunately it was multi-faceted:
- we weren't guarding against the case of a breakpoint stomping on an existing one after skipping forward
- even though we were overwriting the previous breakpoint when skipping forward, we were only removing the breakpoint actor from the cache, not the actual breakpoint from the script
- we weren't storing a script reference to the breakpoint actor, so even if we did remove the breakpoint actor, the breakpoint would have remained in place
- the frontend upon detection of the conflict removed the new breakpoint instead of the old one, essentially doing the opposite of the server

The good news is that it's a simple fix, less than 10 lines of code (if we don't count the test).

The small protocol addition is that in such cases the server will return a success packet to the setBreakpoint request (since a breakpoint was actually set and in accordance with Postel's law), with the same content as the first setBreakpoint request, essentially rendering the second one a no-op. I'll add a note to the docs to clarify this previously unspecified case.

Comment 10

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

(In reply to Panos Astithas [:past] from comment #9)
> Created attachment 685193 [details] [diff] [review]
> Patch v1
> 

Hmm, this doesn't seem to entirely fix the problem when trying a small variation of the steps described in comment 4 (or there's some other dependent patch I'm not applying).

2. Add a breakpoint on line 22 (the comment)
3. The breakpoint got placed on line 24 (which is correct)
4. Add a breakpoint again on line 22
5. Add a breakpoint *again* on line 22
6. Click me

Now it takes 2 resumes to step over line 24. The equation seems to be: n-1 steps required for n breakpoints added on lines without bytecode.

Other than that, the following comment is a bit nonsensical now that the client isn't removed in the frontend anymore.
> // Prevent this new breakpoint from being repositioned on top of an
> // already existing one.

I'd reword the comment to something like "Prevent this new breakpoint from being shown in the source editor at an incorrect position."

Comment 11

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

Attachment: Patch v2

Excellent catch! The check needed to move one line up. I changed the test to check for that issue as well.

Comment 12

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

Comment on attachment 685582 [details] [diff] [review]
Patch v2

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

Excellent. I like this new protocol behavior.

One problem, unrelated to this patch, is that I can't seem to be able to place a breakpoint inside an eval block, not even while it is being evaluated. This also seemed to have regressed recently, although I can't seem to pinpoint a changeset range. Another (possibly related) confusing behavior with eval blocks: go to our beloved debugger page, set a breakpoint on line 14, click me and start stepping in; it seems we're going through the block twice.

Comment 13

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

https://hg.mozilla.org/integration/fx-team/rev/5effa8505da6

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/5effa8505da6

Comment 15

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

(In reply to Victor Porof [:vp] from comment #12)
> One problem, unrelated to this patch, is that I can't seem to be able to
> place a breakpoint inside an eval block, not even while it is being
> evaluated. This also seemed to have regressed recently, although I can't
> seem to pinpoint a changeset range. Another (possibly related) confusing
> behavior with eval blocks: go to our beloved debugger page, set a breakpoint
> on line 14, click me and start stepping in; it seems we're going through the
> block twice.

Filed bug 815992 for this.

Comment 16

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

@#%^&*@<>#$% bugzilla...

Comment 17

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

(In reply to Panos Astithas [:past] from comment #9)
> 
> The small protocol addition is that in such cases the server will return a
> success packet to the setBreakpoint request (since a breakpoint was actually
> set and in accordance with Postel's law), with the same content as the first
> setBreakpoint request, essentially rendering the second one a no-op. I'll
> add a note to the docs to clarify this previously unspecified case.


I assume that once we'll get column support, this check will include the column number as well. What happens when two breakpoints overlap? We can either allow the two to live together, or combine them into a single, larger ("longer?") one.

But we'll burn this bridge when we get there :)

Comment 18

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

How can two breakpoints overlap if they are triplets of the form {url, line, column}?

Comment 19

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

(In reply to Panos Astithas [:past] from comment #18)
> How can two breakpoints overlap if they are triplets of the form {url, line,
> column}?

So by specifying a column, one must necessarily mean "from here to the end of the line"? Isn't there a columnStart and a columnEnd?

Comment 20

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

Breakpoints are set at precise bytecode offsets, which are not ranges by definition. Whether it will make sense to expose a range-breakpoint functionality to the user when we have columns, I'm not so sure.

Comment 21

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

Let's get this patch into Aurora, since it's affected as well.

Comment 22

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

Comment on attachment 685582 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): recent regression in breakpoint handling
User impact if declined: adding multiple breakpoints in empty or comment lines will cause more than one breakpoint to be added in the next line with bytecode in it
Testing completed (on m-c, etc.): fx-team and m-c
Risk to taking this patch (and alternatives if risky): not much risk, devtools-only, less than 10 lines of code change
String or UUID changes made by this patch: none

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7fd045044009