995252 - Conditional breakpoints are always hit

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

It seems that the condition evaluation is broken in Nightly:

1. Open the debugger in http://htmlpad.org/debugger/
2. Set a conditional breakpoint at line 15, with the condition: 1 === 0
3. Click the page button.

Nightly pauses at that breakpoint, whereas Aurora continues to the debugger statement. My guess is that this is a regression from bug 812172.

Comment 1

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

Indeed backing out cset 0ae1f25fe06d fixes the problem.

Comment 2

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

Here is a protocol exchange that illustrates the problem. There are 2 breakpoints created, an unconditional and a conditional one, but the former is not removed when the latter is created:

DBG-SERVER: Received packet 25: {
  "to": "conn0.context54",
  "type": "setBreakpoint",
  "location": {
    "url": "http://htmlpad.org/debugger/",
    "line": 15
  }
}
DBG-SERVER: Received packet 26: {
  "actor": "conn0.breakpoint59",
  "from": "conn0.context54"
}

[...]

DBG-SERVER: Received packet 33: {
  "to": "conn0.context54",
  "type": "setBreakpoint",
  "location": {
    "url": "http://htmlpad.org/debugger/",
    "line": 15
  },
  "condition": "1 === 0"
}
DBG-SERVER: Received packet 34: {
  "actor": "conn0.breakpoint61",
  "from": "conn0.context54"
}

[...]

DBG-SERVER: Received packet 37: {
  "from": "conn0.context54",
  "type": "paused",
  "actor": "conn0.pause62",
[...]
  "why": {
    "type": "breakpoint",
    "actors": [
      "conn0.breakpoint59"
    ]
  }
}

Comment 3

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

I think I've found the cause of the bug, see bug 812172 comment 65. But I'll wait for James to verify this hypothesis when he gets back.

Comment 4

[James Long (:jlongster)]

Conditional evaluation does work in most cases, but I can reproduce your problem. Thanks for filing, I'll look into it and fix!

Comment 5

[James Long (:jlongster)]

Attachment: 995252-cond-bp.patch

So I guess it's not safe to change `setBreakpoint` on the server to only update breakpoints if they already exist. I fought it for a while but couldn't get it to reliably work. I took out some code in scripts.js that was added before to get that to work, but it turns out it was fickle.

The client now removes the breakpoint first and adds a new one with the updated condition.

Comment 6

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

Comment on attachment 8408585 [details] [diff] [review]
995252-cond-bp.patch

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

Thanks for the patch James, looks fine to me!

::: browser/devtools/debugger/debugger-controller.js
@@ +2037,3 @@
>      });
> +
> +    // `setCondition` returns a new breakpoint that has the condition

If you could end the comments with a period it would help enormously with my OCD.

::: toolkit/devtools/client/dbg-client.jsm
@@ +2262,5 @@
>          condition: aCondition
>        };
> +
> +      // Remove the current breakpoint and add a new one with the
> +      // condition

Oh, there is another one! *faints*

@@ +2271,4 @@
>          }
> +
> +        gThreadClient.setBreakpoint(info, (aResponse, aNewBreakpoint) => {
> +          if(aResponse && aResponse.error) {

Space between "if" and paren, because that's all the commentary I can provide in this patch.

Aaaand now that this is all I can see, there is another instance in the previous if block.

I need a nap.

Comment 7

[James Long (:jlongster)]

Attachment: 995252-cond-bp.patch

Haha, minor fixes for OCD.

Comment 8

[James Long (:jlongster)]

https://tbpl.mozilla.org/?tree=Try&rev=4be7a5c9ab25

Comment 9

[James Long (:jlongster)]

Attachment: 995252-cond-bp.patch

Argh, so my previous patch wasn't really working. Since updating the condition is always creating a new breakpoint, we need to make sure that we keep track of that and updating the stores in all the right places. Previously it wasn't doing that everywhere it should be, and tests were failing. (I thought I ran tests on the previous patch, but I guess I didn't, my bad).

All the tests seem to pass on this patch, but there's enough code changes that it needs another review. I really don't like how complicated `addBreakpoint` is. Ideally the debugger protocol could handle updating a breakpoint with a new condition without creating a new breakpoint, but that's probably out of scope for now.

Comment 10

[James Long (:jlongster)]

Comment on attachment 8412180 [details] [diff] [review]
995252-cond-bp.patch

Hold off on the review actually, I'm going to try to convert everything to Task.jsm.

Comment 11

[James Long (:jlongster)]

Attachment: 995252-cond-bp.patch

This patch is final (barring any reviews). All tests pass and I'm going to throw it on try. I changed `addBreakpoint` to use Task.async which makes it much nicer. It could be even cleaner, but I don't want to change too much too quickly.

I've also noticed some failures in tests that *aren't* actually reported as errors (why are some functions wrapped with `makeInfallible?). They are usually an inconsequential race condition (like when trying to shutdown the debugger), but I'm fixing them as I see them. That's why there's a tweak to dbg-client in this patch that tweaks how it calls callbacks in `setBreakpoint`. It was simply wrong before; the result of aOnResponse should not be passed to aCallback.

I'm working on converting the frontend debugger to Task.async (bug 991797) but landing this first because I had already changed some stuff to it.

Comment 12

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

Comment on attachment 8414793 [details] [diff] [review]
995252-cond-bp.patch

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

The reason for using makeInfallible is that in some cases the engine would swallow the error, leaving us in the dark as to what actually happened. We definitely don't need to use it all over the place.

::: browser/devtools/debugger/debugger-controller.js
@@ +1849,5 @@
>        return addedPromise;
>      }
>  
>      // If the breakpoint is currently being removed from the specified location,
> +    // then wait for that to finish

Nit: missing period ;-)

@@ +2046,1 @@
>    },

updateCondition() no longer returns a promise in the common case. You probably meant to add a "return promise;" in the end.

::: toolkit/devtools/client/dbg-client.jsm
@@ +1441,2 @@
>            if (aCallback) {
> +            aCallback();

Good catch, but I think we should also move the callback handling code outside the "if (aOnResponse) {...}" block. Otherwise calling setBreakpoint({...}) when not paused, will fail to resume execution.

@@ +2279,4 @@
>        });
>      } else {
> +      // The client-side implementation forced this property to not
> +      // even exist if the condition was blank, so keep that behavior.

I find the way this comment is worded slightly confusing. We don't need to do this for compatibility purposes, but just because it's the right thing to do, correct? Otherwise we would just fix any tests that depend on that behavior.

Comment 13

[James Long (:jlongster)]

Attachment: 995252-cond-bp.patch

Good points, past, fixed.

Comment 14

[James Long (:jlongster)]

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

The few failures shouldn't be related to this at all. Hopefully I did everything right to queue this for check-in.

Comment 15

[James Long (:jlongster)]

The try link in the previous comment didn't include past's revisions, I forgot that I pushed the patch with minor revisions here: https://tbpl.mozilla.org/?tree=Try&rev=20b7a3a8f3c8

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/2b48d8dcac21

Friendly reminder that your commit message should be reflecting what the patch is actually doing, not restating the problem it's solving. Thanks!
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out for linux debug mochitest-dt failures.
https://hg.mozilla.org/integration/fx-team/rev/4021cb194737

https://tbpl.mozilla.org/php/getParsedLog.php?id=38888711&tree=Fx-Team

Comment 18

[James Long (:jlongster)]

No idea why that test failed. Here is the exact same patch run again on Linux opt & debug: https://tbpl.mozilla.org/?tree=Try&rev=827f19b9b50d

It passes fine. Retry I guess?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

I've triggered more dt runs on that Try push. Let's see how they go first.

Comment 20

[James Long (:jlongster)]

Still looking into the failure on linux debug. I'll probably poke around on the weekend; so far the only thing I can think of is to add more logging and keep running that specific test on try. There is an odd ordering of some of the log messaging so hopefully I'll spot whatever race condition is going on with more logging.

Comment 21

[James Long (:jlongster)]

I'm pretty sure I figured out the race condition. It existed before my code. The problem is that, in that test, the last breakpoint should not be hit because it's conditional expression evaluates to undefined. But since the client-side handling of conditional breakpoints switches modes, and sends an `evaluate` packet to evaluate the expression, and then switches back (OMG), it fires off certain events. The test waits for the "frames cleared" event to recognize when the script is done, and reloads the page. When the script is resumed from the next-to-last breakpoint, that event is fired, and the last breakpoint is hit and does the client-side evaluating, and automatically resumes and the script finished. Another "frames cleared" event is fired.

The "frames cleared" event is throttled to 100ms though, so it only actually fires twice is the test runs slow enough. But in certain cases, when it is slow enough, it will reload the page in the middle of handling the last breakpoint, so we'll get stuff back from the server after the page is reloaded.

Man, I'd really like to start moving to React.js, basically most of these problems would be non-existant. I've pushed a new patch to try and will update.

Comment 22

[James Long (:jlongster)]

Attachment: 995252-cond-bp.patch

Updated patch which fixes the old conditional bp test that had a race condition. This was not introduced in my original patch; I don't know why it hasn't shown up until now. But this fix makes ~75 runs pass: https://tbpl.mozilla.org/?tree=Try&rev=10169e2d76f6

New try for this bug: https://tbpl.mozilla.org/?tree=Try&rev=09e1e110f0f0

Comment 23

[James Long (:jlongster)]

Try is all green! https://tbpl.mozilla.org/?tree=Try&rev=09e1e110f0f0

Comment 24

[James Long (:jlongster)]

Attachment: 995252-cond-bp.patch

Forgot to add the bug # in the patch

Comment 25

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

\o/ land-ho!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Well played, sir :)

https://hg.mozilla.org/integration/fx-team/rev/047828c6ff69

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/047828c6ff69

Comment 28

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

Don't forget to request approval for Aurora!

Comment 29

[James Long (:jlongster)]

Comment on attachment 8418874 [details] [diff] [review]
995252-cond-bp.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 812172
User impact if declined: in the devtools debugger, in some cases conditional breakpoints will always be hit, regardless if the condition was truthy or not
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): already tested on m-c, so not much risk, but it does touch code for adding breakpoints
String or IDL/UUID changes made by this patch:

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/62ed3167d295