Bug 1095216 - Fix breakpoints when multiple of the 'same' source are loaded concurrently. r=ejpbruel

toolkit/devtools/server/actors/script.js

@@ -1436,25 +1436,35 @@ ThreadActor.prototype = {
    * @param object aLocation
    *        The location of the breakpoint (in the generated source, if source
    *        mapping).
+   * @param Debugger.Script aOnlyThisScript [optional]
+   *        If provided, only set breakpoints in this Debugger.Script, and
+   *        nowhere else.
    */
-  _setBreakpoint: function (aLocation) {
+  _setBreakpoint: function (aLocation, aOnlyThisScript=null) {
+    let location = {
+      url: aLocation.url,
+      line: aLocation.line,
+      column: aLocation.column,
+      condition: aLocation.condition
+    };
+
     let actor;
-    let storedBp = this.breakpointStore.getBreakpoint(aLocation);
+    let storedBp = this.breakpointStore.getBreakpoint(location);
     if (storedBp.actor) {
       actor = storedBp.actor;
-      actor.condition = aLocation.condition;
+      actor.condition = location.condition;
     } else {
       storedBp.actor = actor = new BreakpointActor(this, {
-        url: aLocation.url,
-        line: aLocation.line,
-        column: aLocation.column,
-        condition: aLocation.condition
+        url: location.url,
+        line: location.line,
+        column: location.column,
+        condition: location.condition
       });
       this.threadLifetimePool.addActor(actor);
     }
 
     // Find all scripts matching the given location
-    let scripts = this.dbg.findScripts(aLocation);
+    let scripts = this.dbg.findScripts(location);
     if (scripts.length == 0) {
       // Since we did not find any scripts to set the breakpoint on now, return
       // early. When a new script that matches this breakpoint location is
@@ -1474,13 +1484,17 @@ ThreadActor.prototype = {
     let scriptsAndOffsetMappings = new Map();
 
     for (let script of scripts) {
-      this._findClosestOffsetMappings(aLocation,
+      this._findClosestOffsetMappings(location,
                                       script,
                                       scriptsAndOffsetMappings);
     }
 
     if (scriptsAndOffsetMappings.size > 0) {
       for (let [script, mappings] of scriptsAndOffsetMappings) {
+        if (aOnlyThisScript && script !== aOnlyThisScript) {
+          continue;
+        }
+
         for (let offsetMapping of mappings) {
           script.setBreakpoint(offsetMapping.offset, actor);
         }
@@ -1515,62 +1529,68 @@ ThreadActor.prototype = {
     let found = false;
     for (let script of scripts) {
       let offsets = script.getAllOffsets();
-      for (let line = aLocation.line; line < offsets.length; ++line) {
+
+      for (let line = location.line; line < offsets.length; ++line) {
         if (offsets[line]) {
-          for (let offset of offsets[line]) {
-            script.setBreakpoint(offset, actor);
+          if (!aOnlyThisScript || script === aOnlyThisScript) {
+            for (let offset of offsets[line]) {
+              script.setBreakpoint(offset, actor);
+            }
+            actor.addScript(script, this);
           }
-          actor.addScript(script, this);
+
           if (!actualLocation) {
             actualLocation = {
-              url: aLocation.url,
+              url: location.url,
               line: line
             };
           }
+
           found = true;
           break;
         }
       }
     }
+
     if (found) {
       let existingBp = this.breakpointStore.hasBreakpoint(actualLocation);
 
       if (existingBp && existingBp.actor) {
         /**
-         * We already have a breakpoint actor for the actual location, so
-         * actor we created earlier is now redundant. Delete it, update the
-         * breakpoint store, and return the actor for the actual location.
+         * We already have a breakpoint actor for the actual location, so actor
+         * we created earlier is now redundant. Delete it, update the breakpoint
+         * store, and return the actor for the actual location.
          */
         actor.onDelete();
-        this.breakpointStore.removeBreakpoint(aLocation);
+        this.breakpointStore.removeBreakpoint(location);
         return {
           actor: existingBp.actor.actorID,
           actualLocation: actualLocation
         };
-      } else {
-        /**
-         * We don't have a breakpoint actor for the actual location yet.
-         * Instead or creating a new actor, reuse the actor we created earlier,
-         * and update the breakpoint store.
-         */
-        actor.location = actualLocation;
-        this.breakpointStore.addBreakpoint({
-          actor: actor,
-          url: actualLocation.url,
-          line: actualLocation.line,
-          column: actualLocation.column
-        });
-        this.breakpointStore.removeBreakpoint(aLocation);
-        return {
-          actor: actor.actorID,
-          actualLocation: actualLocation
-        };
       }
+
+      /**
+       * We don't have a breakpoint actor for the actual location yet. Instead
+       * or creating a new actor, reuse the actor we created earlier, and update
+       * the breakpoint store.
+       */
+      actor.location = actualLocation;
+      this.breakpointStore.addBreakpoint({
+        actor: actor,
+        url: actualLocation.url,
+        line: actualLocation.line,
+        column: actualLocation.column
+      });
+      this.breakpointStore.removeBreakpoint(location);
+      return {
+        actor: actor.actorID,
+        actualLocation: actualLocation
+      };
     }
 
     /**
-     * If we get here, no line matching the given line was found, so just
-     * fail epically.
+     * If we get here, no line matching the given line was found, so just fail
+     * epically.
      */
     return {
       error: "noCodeAtLineColumn",
@@ -2343,13 +2363,10 @@ ThreadActor.prototype = {
 
     let endLine = aScript.startLine + aScript.lineCount - 1;
     for (let bp of this.breakpointStore.findBreakpoints({ url: aScript.url })) {
-      // Only consider breakpoints that are not already associated with
-      // scripts, and limit search to the line numbers contained in the new
-      // script.
-      if (!bp.actor.scripts.length
-          && bp.line >= aScript.startLine
+      // Limit the search to the line numbers contained in the new script.
+      if (bp.line >= aScript.startLine
           && bp.line <= endLine) {
-        this._setBreakpoint(bp);
+        this._setBreakpoint(bp, aScript);
       }
     }
 
@@ -4510,7 +4527,10 @@ FrameActor.prototype.requestTypes = {
  */
 function BreakpointActor(aThreadActor, { url, line, column, condition })
 {
-  this.scripts = [];
+  // The set of Debugger.Script instances that this breakpoint has been set
+  // upon.
+  this.scripts = new Set();
+
   this.threadActor = aThreadActor;
   this.location = { url: url, line: line, column: column };
   this.condition = condition;
@@ -4522,7 +4542,7 @@ BreakpointActor.prototype = {
 
   /**
    * Called when this same breakpoint is added to another Debugger.Script
-   * instance, in the case of a page reload.
+   * instance.
    *
    * @param aScript Debugger.Script
    *        The new source script on which the breakpoint has been set.
@@ -4531,7 +4551,7 @@ BreakpointActor.prototype = {
    */
   addScript: function (aScript, aThreadActor) {
     this.threadActor = aThreadActor;
-    this.scripts.push(aScript);
+    this.scripts.add(aScript);
   },
 
   /**
@@ -4541,7 +4561,7 @@ BreakpointActor.prototype = {
     for (let script of this.scripts) {
       script.clearBreakpoint(this);
     }
-    this.scripts = [];
+    this.scripts.clear();
   },
 
   /**
@@ -4552,7 +4572,7 @@ BreakpointActor.prototype = {
    *        The frame to evaluate the condition in
    */
   isValidCondition: function(aFrame) {
-    if(!this.condition) {
+    if (!this.condition) {
       return true;
     }
     var res = aFrame.eval(this.condition);

toolkit/devtools/server/tests/unit/test_breakpoint-20.js

@@ -0,0 +1,112 @@
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Verify that when two of the "same" source are loaded concurrently (like e10s
+ * frame scripts), breakpoints get hit in scripts defined by all sources.
+ */
+
+var gDebuggee;
+var gClient;
+var gTraceClient;
+var gThreadClient;
+
+Components.utils.import('resource:///modules/devtools/SourceMap.jsm');
+
+function run_test()
+{
+  initTestTracerServer();
+  gDebuggee = addTestGlobal("test-tracer-actor");
+  gClient = new DebuggerClient(DebuggerServer.connectPipe());
+  gClient.connect(function() {
+    attachTestThread(gClient, "test-tracer-actor", testBreakpoint);
+  });
+  do_test_pending();
+}
+
+const testBreakpoint = Task.async(function* (threadResponse, tabClient, threadClient, tabResponse) {
+  evalSetupCode();
+
+  // Load the test source once.
+
+  evalTestCode();
+  equal(gDebuggee.functions.length, 1,
+        "The test code should have added a function.");
+
+  // Set a breakpoint in the test source.
+
+  yield interrupt(threadClient);
+
+  const [response, bpClient] = yield setBreakpoint(threadClient, {
+    url: "test.js",
+    line: 3
+  });
+  ok(!response.error, "Shouldn't get an error setting the BP.");
+  ok(!response.actualLocation,
+     "Shouldn't get an actualLocation, the location we provided was good.");
+  const bpActor = response.actor;
+
+  yield resume(threadClient);
+
+  // Load the test source again.
+
+  evalTestCode();
+  equal(gDebuggee.functions.length, 2,
+        "The test code should have added another function.");
+
+  // Should hit our breakpoint in a script defined by the first instance of the
+  // test source.
+
+  const bpPause1 = yield executeOnNextTickAndWaitForPause(gDebuggee.functions[0],
+                                                          gClient);
+  equal(bpPause1.why.type, "breakpoint",
+        "Should pause because of hitting our breakpoint (not debugger statement).");
+  equal(bpPause1.why.actors[0], bpActor,
+        "And the breakpoint actor should be correct.");
+  const dbgStmtPause1 = yield executeOnNextTickAndWaitForPause(() => resume(threadClient),
+                                                               gClient);
+  equal(dbgStmtPause1.why.type, "debuggerStatement",
+        "And we should hit the debugger statement after the pause.");
+  yield resume(threadClient);
+
+  // Should also hit our breakpoint in a script defined by the second instance
+  // of the test source.
+
+  const bpPause2 = yield executeOnNextTickAndWaitForPause(gDebuggee.functions[1],
+                                                          gClient);
+  equal(bpPause2.why.type, "breakpoint",
+        "Should pause because of hitting our breakpoint (not debugger statement).");
+  equal(bpPause2.why.actors[0], bpActor,
+        "And the breakpoint actor should be correct.");
+  const dbgStmtPause2 = yield executeOnNextTickAndWaitForPause(() => resume(threadClient),
+                                                               gClient);
+  equal(dbgStmtPause2.why.type, "debuggerStatement",
+        "And we should hit the debugger statement after the pause.");
+
+  finishClient(gClient);
+});
+
+function evalSetupCode() {
+  Cu.evalInSandbox(
+    "this.functions = [];",
+    gDebuggee,
+    "1.8",
+    "setup.js",
+    1
+  );
+}
+
+function evalTestCode() {
+  Cu.evalInSandbox(
+    `                                 // 1
+    this.functions.push(function () { // 2
+      var setBreakpointHere = 1;      // 3
+      debugger;                       // 4
+    });                               // 5
+    `,
+    gDebuggee,
+    "1.8",
+    "test.js",
+    1
+  );
+}

toolkit/devtools/server/tests/unit/xpcshell.ini

@@ -115,6 +115,7 @@ reason = bug 820380
 [test_breakpoint-17.js]
 [test_breakpoint-18.js]
 [test_breakpoint-19.js]
+[test_breakpoint-20.js]
 [test_conditional_breakpoint-01.js]
 [test_conditional_breakpoint-02.js]
 [test_conditional_breakpoint-03.js]