Bug 995252 - Always remove a breakpoint and create a new one when setting the condition. r=past

browser/devtools/debugger/debugger-controller.js

@@ -90,6 +90,7 @@ const FRAME_TYPE = {
 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");
@@ -1835,24 +1836,23 @@ Breakpoints.prototype = {
    *         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();
@@ -1862,7 +1862,7 @@ Breakpoints.prototype = {
     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) {
@@ -1871,11 +1871,6 @@ Breakpoints.prototype = {
         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
@@ -1883,13 +1878,23 @@ Breakpoints.prototype = {
       // 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
@@ -1905,10 +1910,10 @@ Breakpoints.prototype = {
       // Notify that we've added a breakpoint.
       window.emit(EVENTS.BREAKPOINT_ADDED, aBreakpointClient);
       deferred.resolve(aBreakpointClient);
-    });
+    }.bind(this)));
 
     return deferred.promise;
-  },
+  }),
 
   /**
    * Remove a breakpoint.
@@ -2032,12 +2037,14 @@ Breakpoints.prototype = {
                                       '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;
   },
 
   /**