Bug 897567 - JS debugger: setting breakpoints can be confused by script GC. r=fitzgen

toolkit/devtools/client/dbg-client.jsm

@@ -1757,14 +1757,17 @@ ThreadClient.prototype = {
       this.client.request(packet, (aResponse) => {
         // Ignoring errors, since the user may be setting a breakpoint in a
         // dead script that will reappear on a page reload.
-        if (aOnResponse) {
+        let bpClient;
+        if (aResponse.actor) {
           let root = this.client.mainRoot;
-          let bpClient = new BreakpointClient(
+          bpClient = new BreakpointClient(
             this.client,
             aResponse.actor,
             location,
             root.traits.conditionalBreakpoints ? condition : undefined
           );
+        }
+        if (aOnResponse) {
           aOnResponse(aResponse, bpClient);
         }
         if (aCallback) {

toolkit/devtools/server/actors/script.js

@@ -98,7 +98,7 @@ BreakpointStore.prototype = {
   get size() { return this._size; },
 
   /**
-   * Add a breakpoint to the breakpoint store.
+   * Add a breakpoint to the breakpoint store if it doesn't already exist.
    *
    * @param Object aBreakpoint
    *        The breakpoint to be added (not copied). It is an object with the
@@ -109,10 +109,11 @@ BreakpointStore.prototype = {
    *            the whole line)
    *          - condition (optional)
    *          - actor (optional)
+   * @returns Object aBreakpoint
+   *          The new or existing breakpoint.
    */
   addBreakpoint: function (aBreakpoint) {
     let { url, line, column } = aBreakpoint;
-    let updating = false;
 
     if (column != null) {
       if (!this._breakpoints[url]) {
@@ -121,16 +122,22 @@ BreakpointStore.prototype = {
       if (!this._breakpoints[url][line]) {
         this._breakpoints[url][line] = [];
       }
-      this._breakpoints[url][line][column] = aBreakpoint;
+      if (!this._breakpoints[url][line][column]) {
+        this._breakpoints[url][line][column] = aBreakpoint;
+        this._size++;
+      }
+      return this._breakpoints[url][line][column];
     } else {
       // Add a breakpoint that breaks on the whole line.
       if (!this._wholeLineBreakpoints[url]) {
         this._wholeLineBreakpoints[url] = [];
       }
-      this._wholeLineBreakpoints[url][line] = aBreakpoint;
+      if (!this._wholeLineBreakpoints[url][line]) {
+        this._wholeLineBreakpoints[url][line] = aBreakpoint;
+        this._size++;
+      }
+      return this._wholeLineBreakpoints[url][line];
     }
-
-    this._size++;
   },
 
   /**
@@ -1439,9 +1446,7 @@ ThreadActor.prototype = {
 
     let locationPromise = this.sources.getGeneratedLocation(aRequest.location);
     return locationPromise.then(({url, line, column}) => {
-      if (line == null ||
-          line < 0 ||
-          this.dbg.findScripts({ url: url }).length == 0) {
+      if (line == null || line < 0) {
         return {
           error: "noScript",
           message: "Requested setting a breakpoint on "
@@ -1537,12 +1542,11 @@ ThreadActor.prototype = {
     // Find all scripts matching the given location
     let scripts = this.dbg.findScripts(aLocation);
     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
+      // introduced, the breakpoint actor will already be in the breakpoint store
+      // and will be set at that time.
       return {
-        error: "noScript",
-        message: "Requested setting a breakpoint on "
-          + aLocation.url + ":" + aLocation.line
-          + (aLocation.column != null ? ":" + aLocation.column : "")
-          + " but there is no Debugger.Script at that location",
         actor: actor.actorID
       };
     }

toolkit/devtools/server/tests/unit/head_dbg.js

@@ -510,6 +510,17 @@ function resume(threadClient) {
   return rdpRequest(threadClient, threadClient.resume);
 }
 
+/**
+ * Interrupt JS execution for the specified thread.
+ *
+ * @param ThreadClient threadClient
+ * @returns Promise
+ */
+function interrupt(threadClient) {
+  dumpn("Interrupting.");
+  return rdpRequest(threadClient, threadClient.interrupt);
+}
+
 /**
  * Resume JS execution for the specified thread and then wait for the next pause
  * event.

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

@@ -8,62 +8,65 @@
 var gDebuggee;
 var gClient;
 var gThreadClient;
-var gCallback;
 
 function run_test()
 {
-  run_test_with_server(DebuggerServer, function () {
-    run_test_with_server(WorkerDebuggerServer, do_test_finished);
-  });
-  do_test_pending();
-};
-
-function run_test_with_server(aServer, aCallback)
-{
-  gCallback = aCallback;
-  initTestDebuggerServer(aServer);
-  gDebuggee = addTestGlobal("test-stack", aServer);
-  gClient = new DebuggerClient(aServer.connectPipe());
+  initTestDebuggerServer();
+  gDebuggee = addTestGlobal("test-stack");
+  gClient = new DebuggerClient(DebuggerServer.connectPipe());
   gClient.connect(function () {
     attachTestTabAndResume(gClient, "test-stack", function (aResponse, aTabClient, aThreadClient) {
       gThreadClient = aThreadClient;
-      test_same_breakpoint();
+      testSameBreakpoint();
     });
   });
+  do_test_pending();
 }
 
-function test_same_breakpoint()
-{
-  gThreadClient.addOneTimeListener("paused", function (aEvent, aPacket) {
-    let path = getFilePath('test_breakpoint-01.js');
-    let location = {
-      url: path,
-      line: gDebuggee.line0 + 3
-    };
-    gThreadClient.setBreakpoint(location, function (aResponse, bpClient) {
-      gThreadClient.setBreakpoint(location, function (aResponse, secondBpClient) {
-        do_check_eq(bpClient.actor, secondBpClient.actor,
-                    "Should get the same actor w/ whole line breakpoints");
-        let location = {
-          url: path,
-          line: gDebuggee.line0 + 2,
-          column: 6
-        };
-        gThreadClient.setBreakpoint(location, function (aResponse, bpClient) {
-          gThreadClient.setBreakpoint(location, function (aResponse, secondBpClient) {
-            do_check_eq(bpClient.actor, secondBpClient.actor,
-                        "Should get the same actor column breakpoints");
-            gClient.close(gCallback);
-          });
-        });
-      });
-    });
 
-  });
+const SOURCE_URL = "http://example.com/source.js";
 
-  Components.utils.evalInSandbox("var line0 = Error().lineNumber;\n" +
-                                 "debugger;\n" +   // line0 + 1
-                                 "var a = 1;\n" +  // line0 + 2
-                                 "var b = 2;\n",   // line0 + 3
-                                 gDebuggee);
-}
+const testSameBreakpoint = Task.async(function* () {
+  yield executeOnNextTickAndWaitForPause(evalCode, gClient);
+
+  // Whole line
+
+  let wholeLineLocation = {
+    url: SOURCE_URL,
+    line: 2
+  };
+
+  let [firstResponse, firstBpClient] = yield setBreakpoint(gThreadClient, wholeLineLocation);
+  let [secondResponse, secondBpClient] = yield setBreakpoint(gThreadClient, wholeLineLocation);
+
+  do_check_eq(firstBpClient.actor, secondBpClient.actor, "Should get the same actor w/ whole line breakpoints");
+
+  // Specific column
+
+  let columnLocation = {
+    url: SOURCE_URL,
+    line: 2,
+    column: 6
+  };
+
+  let [firstResponse, firstBpClient] = yield setBreakpoint(gThreadClient, columnLocation);
+  let [secondResponse, secondBpClient] = yield setBreakpoint(gThreadClient, columnLocation);
+
+  do_check_eq(secondBpClient.actor, secondBpClient.actor, "Should get the same actor column breakpoints");
+
+  finishClient(gClient);
+});
+
+function evalCode() {
+  Components.utils.evalInSandbox(
+    "" + function doStuff(k) { // line 1
+      let arg = 15;            // line 2 - Step in here
+      k(arg);                  // line 3
+    } + "\n"                   // line 4
+    + "debugger;",             // line 5
+    gDebuggee,
+    "1.8",
+    SOURCE_URL,
+    1
+  );
+}

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

@@ -0,0 +1,69 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Make sure that setting a breakpoint in a not-yet-existing script doesn't throw
+ * an error (see bug 897567). Also make sure that this breakpoint works.
+ */
+
+var gDebuggee;
+var gClient;
+var gThreadClient;
+var gCallback;
+
+function run_test()
+{
+  run_test_with_server(DebuggerServer, function () {
+    run_test_with_server(WorkerDebuggerServer, do_test_finished);
+  });
+  do_test_pending();
+};
+
+function run_test_with_server(aServer, aCallback)
+{
+  gCallback = aCallback;
+  initTestDebuggerServer(aServer);
+  gDebuggee = addTestGlobal("test-breakpoints", aServer);
+  gDebuggee.console = { log: x => void x };
+  gClient = new DebuggerClient(aServer.connectPipe());
+  gClient.connect(function () {
+    attachTestTabAndResume(gClient,
+                           "test-breakpoints",
+                           function (aResponse, aTabClient, aThreadClient) {
+      gThreadClient = aThreadClient;
+      testBreakpoint();
+    });
+  });
+}
+
+const URL = "test.js";
+
+function setUpCode() {
+  Cu.evalInSandbox(
+    "" + function test() { // 1
+      var a = 1;           // 2
+      debugger;            // 3
+    } +                    // 4
+    "\ndebugger;",         // 5
+    gDebuggee,
+    "1.8",
+    URL
+  );
+}
+
+const testBreakpoint = Task.async(function* () {
+  const [response, bpClient] = yield setBreakpoint(gThreadClient, {url: URL, line: 2});
+  ok(!response.error);
+
+  const actor = response.actor;
+  ok(actor);
+
+  yield executeOnNextTickAndWaitForPause(setUpCode, gClient);
+  yield resume(gThreadClient);
+
+  const packet = yield executeOnNextTickAndWaitForPause(gDebuggee.test, gClient);
+  equal(packet.why.type, "breakpoint")
+  notEqual(packet.why.actors.indexOf(actor), -1);
+
+  finishClient(gClient);
+});

toolkit/devtools/server/tests/unit/test_breakpointstore.js

@@ -16,6 +16,7 @@ function run_test()
   test_add_breakpoint();
   test_remove_breakpoint();
   test_find_breakpoints();
+  test_duplicate_breakpoints();
 }
 
 function test_has_breakpoint() {
@@ -165,3 +166,28 @@ function test_find_breakpoints() {
   do_check_eq(bpSet.size, 0,
               "Should be able to filter the iteration by url and line");
 }
+
+function test_duplicate_breakpoints() {
+  let bpStore = new BreakpointStore();
+
+  // Breakpoint with column
+  let location = {
+    url: "http://example.com/foo.js",
+    line: 10,
+    column: 9
+  };
+  bpStore.addBreakpoint(location);
+  bpStore.addBreakpoint(location);
+  do_check_eq(bpStore.size, 1, "We should have only 1 column breakpoint");
+  bpStore.removeBreakpoint(location);
+
+  // Breakpoint without column (whole line breakpoint)
+  location = {
+    url: "http://example.com/foo.js",
+    line: 15
+  };
+  bpStore.addBreakpoint(location);
+  bpStore.addBreakpoint(location);
+  do_check_eq(bpStore.size, 1, "We should have only 1 whole line breakpoint");
+  bpStore.removeBreakpoint(location);
+}

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

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