Bug 995252 - Conditional breakpoints are always hit. r=past
☠☠ backed out by 4021cb194737 ☠ ☠
authorJames Long <jlong>
Wed, 30 Apr 2014 11:42:00 -0400
changeset 181672 2b48d8dcac21190c20a9e0e146e7abe1106ea76d
parent 181671 36d74d4c5c47b7a8a58465715587a7f91f8a25fc
child 181673 90980ce65b540b05f54470c58b353cf52df54bb3
push id272
push userpvanderbeken@mozilla.com
push dateMon, 05 May 2014 16:31:18 +0000
reviewerspast
bugs995252
milestone32.0a1
Bug 995252 - Conditional breakpoints are always hit. r=past
browser/devtools/debugger/debugger-controller.js
browser/devtools/debugger/test/browser_dbg_breakpoints-highlight.js
browser/devtools/debugger/test/browser_dbg_server-conditional-bp-03.js
browser/devtools/debugger/test/browser_dbg_server-conditional-bp-04.js
toolkit/devtools/client/dbg-client.jsm
toolkit/devtools/server/actors/script.js
--- a/browser/devtools/debugger/debugger-controller.js
+++ b/browser/devtools/debugger/debugger-controller.js
@@ -85,16 +85,17 @@ const FRAME_TYPE = {
   CONDITIONAL_BREAKPOINT_EVAL: 1,
   WATCH_EXPRESSIONS_EVAL: 2,
   PUBLIC_CLIENT_EVAL: 3
 };
 
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/devtools/event-emitter.js");
+Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource:///modules/devtools/SimpleListWidget.jsm");
 Cu.import("resource:///modules/devtools/BreadcrumbsWidget.jsm");
 Cu.import("resource:///modules/devtools/SideMenuWidget.jsm");
 Cu.import("resource:///modules/devtools/VariablesView.jsm");
 Cu.import("resource:///modules/devtools/VariablesViewController.jsm");
 Cu.import("resource:///modules/devtools/ViewHelpers.jsm");
 
 const require = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools.require;
@@ -604,22 +605,22 @@ StackFrames.prototype = {
     if (!client.mainRoot.traits.conditionalBreakpoints) {
       // Conditional breakpoints are { breakpoint, expression } tuples. The
       // boolean evaluation of the expression decides if the active thread
       // automatically resumes execution or not.
       if (breakLocation) {
         // Make sure a breakpoint actually exists at the specified url and line.
         let breakpointPromise = DebuggerController.Breakpoints._getAdded(breakLocation);
         if (breakpointPromise) {
+          waitForNextPause = true;
           breakpointPromise.then(({ conditionalExpression: e }) => { if (e) {
             // Evaluating the current breakpoint's conditional expression will
             // cause the stack frames to be cleared and active thread to pause,
             // sending a 'clientEvaluated' packed and adding the frames again.
             this.evaluate(e, { depth: 0, meta: FRAME_TYPE.CONDITIONAL_BREAKPOINT_EVAL });
-            waitForNextPause = true;
           }});
         }
       }
       // We'll get our evaluation of the current breakpoint's conditional
       // expression the next time the thread client pauses...
       if (waitForNextPause) {
         return;
       }
@@ -1830,90 +1831,94 @@ Breakpoints.prototype = {
    *        Additional options or flags supported by this operation:
    *          - openPopup: tells if the expression popup should be shown.
    *          - noEditorUpdate: tells if you want to skip editor updates.
    *          - noPaneUpdate: tells if you want to skip breakpoint pane updates.
    * @return object
    *         A promise that is resolved after the breakpoint is added, or
    *         rejected if there was an error.
    */
-  addBreakpoint: function(aLocation, aOptions = {}) {
+  addBreakpoint: Task.async(function*(aLocation, aOptions = {}) {
     // Make sure a proper location is available.
     if (!aLocation) {
-      return promise.reject(new Error("Invalid breakpoint location."));
+      throw new Error("Invalid breakpoint location.");
     }
+    let addedPromise, removingPromise;
 
     // If the breakpoint was already added, or is currently being added at the
     // specified location, then return that promise immediately.
-    let addedPromise = this._getAdded(aLocation);
-    if (addedPromise) {
+    if ((addedPromise = this._getAdded(aLocation))) {
       return addedPromise;
     }
 
     // If the breakpoint is currently being removed from the specified location,
-    // then wait for that to finish and retry afterwards.
-    let removingPromise = this._getRemoving(aLocation);
-    if (removingPromise) {
-      return removingPromise.then(() => this.addBreakpoint(aLocation, aOptions));
+    // then wait for that to finish.
+    if ((removingPromise = this._getRemoving(aLocation))) {
+      yield removingPromise;
     }
 
     let deferred = promise.defer();
 
     // Remember the breakpoint initialization promise in the store.
     let identifier = this.getIdentifier(aLocation);
     this._added.set(identifier, deferred.promise);
 
     // Try adding the breakpoint.
-    gThreadClient.setBreakpoint(aLocation, (aResponse, aBreakpointClient) => {
+    gThreadClient.setBreakpoint(aLocation, Task.async(function*(aResponse, aBreakpointClient) {
       // If the breakpoint response has an "actualLocation" attached, then
       // the original requested placement for the breakpoint wasn't accepted.
       if (aResponse.actualLocation) {
         // Remember the initialization promise for the new location instead.
         let oldIdentifier = identifier;
         let newIdentifier = identifier = this.getIdentifier(aResponse.actualLocation);
         this._added.delete(oldIdentifier);
         this._added.set(newIdentifier, deferred.promise);
-
-        // Store the originally requested location in case it's ever needed
-        // and update the breakpoint client with the actual location.
-        aBreakpointClient.requestedLocation = aLocation;
-        aBreakpointClient.location = aResponse.actualLocation;
       }
 
       // By default, new breakpoints are always enabled. Disabled breakpoints
       // are, in fact, removed from the server but preserved in the frontend,
       // so that they may not be forgotten across target navigations.
       let disabledPromise = this._disabled.get(identifier);
       if (disabledPromise) {
-        disabledPromise.then((aPrevBreakpointClient) => {
-          let condition = aPrevBreakpointClient.getCondition();
-          if (condition) {
-            aBreakpointClient.setCondition(gThreadClient, condition);
-          }
-        });
+        let aPrevBreakpointClient = yield disabledPromise;
+        let condition = aPrevBreakpointClient.getCondition();
         this._disabled.delete(identifier);
+
+        if (condition) {
+          aBreakpointClient = yield aBreakpointClient.setCondition(
+            gThreadClient,
+            condition
+          );
+        }
+      }
+
+      if (aResponse.actualLocation) {
+        // Store the originally requested location in case it's ever needed
+        // and update the breakpoint client with the actual location.
+        aBreakpointClient.requestedLocation = aLocation;
+        aBreakpointClient.location = aResponse.actualLocation;
       }
 
       // Preserve information about the breakpoint's line text, to display it
       // in the sources pane without requiring fetching the source (for example,
       // after the target navigated). Note that this will get out of sync
       // if the source text contents change.
       let line = aBreakpointClient.location.line - 1;
       aBreakpointClient.text = DebuggerView.editor.getText(line).trim();
 
       // Show the breakpoint in the editor and breakpoints pane, and resolve.
       this._showBreakpoint(aBreakpointClient, aOptions);
 
       // Notify that we've added a breakpoint.
       window.emit(EVENTS.BREAKPOINT_ADDED, aBreakpointClient);
       deferred.resolve(aBreakpointClient);
-    });
+    }.bind(this)));
 
     return deferred.promise;
-  },
+  }),
 
   /**
    * Remove a breakpoint.
    *
    * @param object aLocation
    *        @see DebuggerController.Breakpoints.addBreakpoint
    * @param object aOptions [optional]
    *        @see DebuggerController.Breakpoints.addBreakpoint
@@ -2027,22 +2032,24 @@ Breakpoints.prototype = {
    */
   updateCondition: function(aLocation, aCondition) {
     let addedPromise = this._getAdded(aLocation);
     if (!addedPromise) {
       return promise.reject(new Error('breakpoint does not exist ' +
                                       'in specified location'));
     }
 
-    return addedPromise.then(aBreakpointClient => {
+    var promise = addedPromise.then(aBreakpointClient => {
       return aBreakpointClient.setCondition(gThreadClient, aCondition);
-    }, err => {
-      DevToolsUtils.reportException("Breakpoints.prototype.updateCondition",
-                                    err);
     });
+
+    // `setCondition` returns a new breakpoint that has the condition,
+    // so we need to update the store
+    this._added.set(this.getIdentifier(aLocation), promise);
+    return promise;
   },
 
   /**
    * Update the editor and breakpoints pane to show a specified breakpoint.
    *
    * @param object aBreakpointData
    *        Information about the breakpoint to be shown.
    *        This object must have the following properties:
--- a/browser/devtools/debugger/test/browser_dbg_breakpoints-highlight.js
+++ b/browser/devtools/debugger/test/browser_dbg_breakpoints-highlight.js
@@ -21,17 +21,18 @@ function test() {
 
     waitForSourceShown(gPanel, "-01.js")
       .then(addBreakpoints)
       .then(() => clickBreakpointAndCheck(0, 0, 5))
       .then(() => clickBreakpointAndCheck(1, 1, 6))
       .then(() => clickBreakpointAndCheck(2, 1, 7))
       .then(() => clickBreakpointAndCheck(3, 1, 8))
       .then(() => clickBreakpointAndCheck(4, 1, 9))
-      .then(() => resumeDebuggerThenCloseAndFinish(gPanel))
+      .then(() => ensureThreadClientState(gPanel, "resumed"))
+      .then(() => closeDebuggerAndFinish(gPanel))
       .then(null, aError => {
         ok(false, "Got an error: " + aError.message + "\n" + aError.stack);
       });
   });
 
   function addBreakpoints() {
     return promise.resolve(null)
       .then(() => initialChecks(0, 1))
--- a/browser/devtools/debugger/test/browser_dbg_server-conditional-bp-03.js
+++ b/browser/devtools/debugger/test/browser_dbg_server-conditional-bp-03.js
@@ -50,17 +50,17 @@ function test() {
     gDebuggee.ermahgerd();
   });
 
   function addBreakpoint() {
     return gPanel.addBreakpoint(gLocation);
   }
 
   function setConditional(aClient) {
-    aClient.condition = "hello";
+    return gBreakpoints.updateCondition(aClient.location, "hello");
   }
 
   function toggleBreakpoint() {
     EventUtils.sendMouseEvent({ type: "click" },
       gDebugger.document.querySelector(".dbg-breakpoint-checkbox"),
       gDebugger);
   }
 
--- a/browser/devtools/debugger/test/browser_dbg_server-conditional-bp-04.js
+++ b/browser/devtools/debugger/test/browser_dbg_server-conditional-bp-04.js
@@ -53,17 +53,17 @@ function test() {
 
   function addBreakpoint() {
     return gPanel.addBreakpoint(gLocation);
   }
 
   function setDummyConditional(aClient) {
     // This happens when a conditional expression input popup is shown
     // but the user doesn't type anything into it.
-    aClient.condition = "";
+    return gBreakpoints.updateCondition(aClient.location, '');
   }
 
   function toggleBreakpoint() {
     EventUtils.sendMouseEvent({ type: "click" },
       gDebugger.document.querySelector(".dbg-breakpoint-checkbox"),
       gDebugger);
   }
 
--- a/toolkit/devtools/client/dbg-client.jsm
+++ b/toolkit/devtools/client/dbg-client.jsm
@@ -1432,21 +1432,20 @@ ThreadClient.prototype = {
         if (aOnResponse) {
           let root = this.client.mainRoot;
           let bpClient = new BreakpointClient(
             this.client,
             aResponse.actor,
             location,
             root.traits.conditionalBreakpoints ? condition : undefined
           );
-          if (aCallback) {
-            aCallback(aOnResponse(aResponse, bpClient));
-          } else {
-            aOnResponse(aResponse, bpClient);
-          }
+          aOnResponse(aResponse, bpClient);
+        }
+        if (aCallback) {
+          aCallback();
         }
       }.bind(this));
     }.bind(this);
 
     // If the debuggee is paused, just set the breakpoint.
     if (this.paused) {
       doSetBreakpoint();
       return;
@@ -2253,29 +2252,45 @@ BreakpointClient.prototype = {
   setCondition: function(gThreadClient, aCondition) {
     let root = this._client.mainRoot;
     let deferred = promise.defer();
 
     if (root.traits.conditionalBreakpoints) {
       let info = {
         url: this.location.url,
         line: this.location.line,
+        column: this.location.column,
         condition: aCondition
       };
-      gThreadClient.setBreakpoint(info, (aResponse, ignoredBreakpoint) => {
-        if(aResponse && aResponse.error) {
+
+      // Remove the current breakpoint and add a new one with the
+      // condition.
+      this.remove(aResponse => {
+        if (aResponse && aResponse.error) {
           deferred.reject(aResponse);
-        } else {
-          this.condition = aCondition;
-          deferred.resolve(null);
+          return;
         }
+
+        gThreadClient.setBreakpoint(info, (aResponse, aNewBreakpoint) => {
+          if (aResponse && aResponse.error) {
+            deferred.reject(aResponse);
+          } else {
+            deferred.resolve(aNewBreakpoint);
+          }
+        });
       });
     } else {
-      this.conditionalExpression = aCondition;
-      deferred.resolve(null);
+      // The property shouldn't even exist if the condition is blank
+      if(aCondition === "") {
+        delete this.conditionalExpression;
+      }
+      else {
+        this.conditionalExpression = aCondition;
+      }
+      deferred.resolve(this);
     }
 
     return deferred.promise;
   }
 };
 
 eventSource(BreakpointClient.prototype);
 
--- a/toolkit/devtools/server/actors/script.js
+++ b/toolkit/devtools/server/actors/script.js
@@ -109,34 +109,26 @@ BreakpointStore.prototype = {
 
     if (column != null) {
       if (!this._breakpoints[url]) {
         this._breakpoints[url] = [];
       }
       if (!this._breakpoints[url][line]) {
         this._breakpoints[url][line] = [];
       }
-      if(this._breakpoints[url][line][column]) {
-        updating = true;
-      }
       this._breakpoints[url][line][column] = aBreakpoint;
     } else {
       // Add a breakpoint that breaks on the whole line.
       if (!this._wholeLineBreakpoints[url]) {
         this._wholeLineBreakpoints[url] = [];
       }
-      if(this._wholeLineBreakpoints[url][line]) {
-        updating = true;
-      }
       this._wholeLineBreakpoints[url][line] = aBreakpoint;
     }
 
-    if (!updating) {
-      this._size++;
-    }
+    this._size++;
   },
 
   /**
    * Remove a breakpoint from the breakpoint store.
    *
    * @param Object aBreakpoint
    *        The breakpoint to be removed. It is an object with the following
    *        properties: