Closed Bug 995252 Opened 6 years ago Closed 6 years ago

Conditional breakpoints are always hit

Categories

(DevTools :: Debugger, defect, P2)

x86
macOS
defect

Tracking

(firefox29 unaffected, firefox30 unaffected, firefox31 fixed, firefox32 fixed)

RESOLVED FIXED
Firefox 32
Tracking Status
firefox29 --- unaffected
firefox30 --- unaffected
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: past, Assigned: jlong)

References

Details

(Keywords: regression)

Attachments

(1 file, 6 obsolete files)

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.
Indeed backing out cset 0ae1f25fe06d fixes the problem.
Blocks: 812172
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"
    ]
  }
}
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.
Conditional evaluation does work in most cases, but I can reproduce your problem. Thanks for filing, I'll look into it and fix!
Attached patch 995252-cond-bp.patch (obsolete) — Splinter Review
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.
Attachment #8408585 - Flags: review?(past)
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.
Attachment #8408585 - Flags: review?(past) → review+
Assignee: nobody → jlong
Status: NEW → ASSIGNED
Priority: -- → P2
Attached patch 995252-cond-bp.patch (obsolete) — Splinter Review
Haha, minor fixes for OCD.
Attachment #8408585 - Attachment is obsolete: true
Attached patch 995252-cond-bp.patch (obsolete) — Splinter Review
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.
Attachment #8411011 - Attachment is obsolete: true
Attachment #8412180 - Flags: review?(past)
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.
Attachment #8412180 - Flags: review?(past) → review-
Attached patch 995252-cond-bp.patch (obsolete) — Splinter Review
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.
Attachment #8414793 - Flags: review?(past)
Attachment #8412180 - Attachment is obsolete: true
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.
Attachment #8414793 - Flags: review?(past) → review+
Attached patch 995252-cond-bp.patch (obsolete) — Splinter Review
Good points, past, fixed.
Attachment #8414793 - Attachment is obsolete: true
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.
Keywords: checkin-needed
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
Blocks: 991797
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
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
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?
Keywords: checkin-needed
I've triggered more dt runs on that Try push. Let's see how they go first.
Keywords: checkin-needed
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.
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.
Attached patch 995252-cond-bp.patch (obsolete) — Splinter Review
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
Attachment #8415469 - Attachment is obsolete: true
Forgot to add the bug # in the patch
Attachment #8418773 - Attachment is obsolete: true
\o/ land-ho!
Well played, sir :)

https://hg.mozilla.org/integration/fx-team/rev/047828c6ff69
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/047828c6ff69
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Don't forget to request approval for Aurora!
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:
Attachment #8418874 - Flags: approval-mozilla-aurora?
Attachment #8418874 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.