Closed Bug 1253902 Opened 4 years ago Closed 4 years ago

[DevTools][Debugger] Cannot add conditional breakpoint


(DevTools :: Debugger, defect, P1)



(firefox45 affected, firefox46 affected, firefox47 affected, firefox48 fixed, firefox-esr45 affected)

Firefox 48
Tracking Status
firefox45 --- affected
firefox46 --- affected
firefox47 --- affected
firefox48 --- fixed
firefox-esr45 --- affected


(Reporter:, Assigned: jlong)




(3 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160305030241

Steps to reproduce:

1. Start Nightly (reproduce in Firefox 45 or later version)
2. Go to any sites (e.g. about:home)
3. Open DevTools > Debugger
4. Select a source item
5. Add conditional breakpoint using right-click context menu on line number

Actual results:

Non-conditional breakpoint is added.
If right-click on current line, conditional breakpoint is added on wrong line. And also cannot added two or more conditional breakpoints. 

Expected results:

"Add Conditional Breakpoint" works correctly, and can add two or more conditional breakpoints.
Regression range:
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools: Debugger
Depends on: 1200798
Flags: needinfo?(jlong)
OS: Unspecified → All
Hardware: Unspecified → All
Version: 45 Branch → unspecified
I can't reproduce anything except the main symptom which is that right-click > add conditional breakpoint does not open the popup to enter a condition expression. The way conditional breakpoints work is it's considered a conditional breakpoint if the breakpoint has a conditional expression. So I just need to figure out why the popup isn't coming up (if you don't enter an expression in the popup, it's considered a normal breakpoint).

No idea why you can't add more than 1 or things like that.
Flags: needinfo?(jlong)
Attached patch 1253902.patch (obsolete) — Splinter Review
Hi James, I have uploaded two videos. If you have any questions, please let me know.
Thanks! Yeah, the only problem is that the popup doesn't open. This patch should fix that. I'm not sure why it opens that one time; it must be some interaction with the cursor. The current patch should make it always open.
Basic functionality that should just work, and James already has a patch for it, so marking this as P1.
Priority: -- → P1
Attached patch 1253902.patchSplinter Review
The tests were failing. I fixed them, the problem was that their weren't properly waiting for requests to be resolved. The behavior before didn't expose this because state happened to be what we were checking.

I did have to make one semantic change: if you add a breakpoint with an empty condition, disable and re-enable the breakpoint, it stays as a conditional breakpoint. The code is otherwise super confusing to try and "remove" the empty condition, and I deemed it not worth it. It makes more sense to me anyway... why would the breakpoint type change? If you really want it to change just remove it and add a new breakpoint.
Attachment #8727943 - Attachment is obsolete: true
Attachment #8730882 - Flags: review?(ejpbruel)
Comment on attachment 8730882 [details] [diff] [review]

Review of attachment 8730882 [details] [diff] [review]:

This is one of those subtle edge cases where code that seems to be correct at first glance is actually wrong (due to the falsyness of ""). I think it would be good practice if we started adding comments when we fix these edge cases, to explain what the problem was, and how we fixed it.

Patch looks good to me otherwise, so I'll leave it up to you how much commenting you think is prudent.

::: devtools/client/debugger/content/reducers/breakpoints.js
@@ +11,5 @@
>  const initialState = Immutable({
>    breakpoints: {}
>  });
> +function stringOr(...args) {

This name is not particularly descriptive of what this function does. It's not a big deal, since it's a small function, but I still think a one line comment about what this function does would be helpful.

@@ +12,5 @@
>    breakpoints: {}
>  });
> +function stringOr(...args) {
> +  for(var arg of args) {

Nit: space between for and (

@@ +13,5 @@
>  });
> +function stringOr(...args) {
> +  for(var arg of args) {
> +    if(typeof arg === "string") {

Nit: space between if and (

@@ +32,5 @@
>        state = setIn(state, ['breakpoints', id], bp.merge({
>          disabled: false,
>          loading: true,
> +        condition: stringOr(action.condition, bp.condition)

If I were new to this code and not familiar with the subtle problem that stringOr solves here, I might be tempted to change it back. Perhaps a comment here to explain the issue would be useful?

::: devtools/client/debugger/test/mochitest/browser_dbg_server-conditional-bp-02.js
@@ +114,5 @@
>                                  gDebugger.document.querySelectorAll(".dbg-breakpoint")[aIndex],
>                                  gDebugger);
>      }
> +    function waitForConditionUpdate() {

If you're using this in multiple tests it should probably be moved to head.js.
Attachment #8730882 - Flags: review?(ejpbruel) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Assignee: nobody → jlong
I have reproduced this bug with Nightly 47.0a1 (2016-03-05) on Windows 7, 64 Bit! 

The bug's fix has been verified in latest Nightly, Developer Edition, and Beta.

 Build ID    20160714030208
 User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

 Build ID    20160708004052

 User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0

 Build ID    20160714050942

 User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.