Bug 832231 - After a reload, breakpoints require multiple resumes to allow execution to continue; r=vporof

browser/devtools/debugger/test/Makefile.in

@@ -61,6 +61,7 @@ MOCHITEST_BROWSER_TESTS = \
 	browser_dbg_location-changes.js \
 	browser_dbg_location-changes-new.js \
 	browser_dbg_location-changes-blank.js \
+	browser_dbg_location-changes-bp.js \
 	browser_dbg_sources-cache.js \
 	browser_dbg_scripts-switching.js \
 	browser_dbg_scripts-sorting.js \
@@ -133,6 +134,8 @@ MOCHITEST_BROWSER_PAGES = \
 	binary_search.coffee \
 	binary_search.js \
 	binary_search.map \
+	test-location-changes-bp.js \
+	test-location-changes-bp.html \
 	$(NULL)
 
 MOCHITEST_BROWSER_FILES_PARTS = MOCHITEST_BROWSER_TESTS MOCHITEST_BROWSER_PAGES

browser/devtools/debugger/test/browser_dbg_location-changes-bp.js

@@ -0,0 +1,163 @@
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Make sure that reloading a page with a breakpoint set does not cause it to
+ * fire more than once.
+ */
+
+const TAB_URL = EXAMPLE_URL + "test-location-changes-bp.html";
+const SCRIPT_URL = EXAMPLE_URL + "test-location-changes-bp.js";
+
+var gPane = null;
+var gTab = null;
+var gDebuggee = null;
+var gDebugger = null;
+var sourcesShown = false;
+var tabNavigated = false;
+
+function test()
+{
+  debug_tab_pane(TAB_URL, function(aTab, aDebuggee, aPane) {
+    gTab = aTab;
+    gDebuggee = aDebuggee;
+    gPane = aPane;
+    gDebugger = gPane.panelWin;
+
+    testAddBreakpoint();
+  });
+}
+
+function testAddBreakpoint()
+{
+  let controller = gDebugger.DebuggerController;
+  controller.activeThread.addOneTimeListener("framesadded", function() {
+    Services.tm.currentThread.dispatch({ run: function() {
+
+      var frames = gDebugger.DebuggerView.StackFrames._container._list;
+
+      is(controller.activeThread.state, "paused",
+         "The debugger statement was reached.");
+
+      is(frames.querySelectorAll(".dbg-stackframe").length, 1,
+         "Should have one frame.");
+
+      gPane.addBreakpoint({ url: SCRIPT_URL, line: 5 }, testResume);
+    }}, 0);
+  });
+
+  gDebuggee.runDebuggerStatement();
+}
+
+function testResume()
+{
+  is(gDebugger.DebuggerController.activeThread.state, "paused",
+    "The breakpoint wasn't hit yet.");
+
+  let thread = gDebugger.DebuggerController.activeThread;
+  thread.addOneTimeListener("resumed", function() {
+    thread.addOneTimeListener("paused", function() {
+      executeSoon(testBreakpointHit);
+    });
+
+    EventUtils.sendMouseEvent({ type: "click" },
+      content.document.querySelector("button"));
+  });
+
+  thread.resume();
+}
+
+function testBreakpointHit()
+{
+  is(gDebugger.DebuggerController.activeThread.state, "paused",
+    "The breakpoint was hit.");
+
+  let thread = gDebugger.DebuggerController.activeThread;
+  thread.addOneTimeListener("paused", function test(aEvent, aPacket) {
+    thread.addOneTimeListener("resumed", function() {
+      executeSoon(testReloadPage);
+    });
+
+    is(aPacket.why.type, "debuggerStatement", "Execution has advanced to the next line.");
+    isnot(aPacket.why.type, "breakpoint", "No ghost breakpoint was hit.");
+    thread.resume();
+  });
+
+  thread.resume();
+}
+
+function testReloadPage()
+{
+  let controller = gDebugger.DebuggerController;
+  controller._target.once("navigate", function onTabNavigated(aEvent, aPacket) {
+    tabNavigated = true;
+    ok(true, "tabNavigated event was fired.");
+    info("Still attached to the tab.");
+    clickAgain();
+  });
+
+  gDebugger.addEventListener("Debugger:SourceShown", function onSourcesShown() {
+    sourcesShown = true;
+    gDebugger.removeEventListener("Debugger:SourceShown", onSourcesShown);
+    clickAgain();
+  });
+
+  content.location.reload();
+}
+
+function clickAgain()
+{
+  if (!sourcesShown || !tabNavigated) {
+    return;
+  }
+
+  let controller = gDebugger.DebuggerController;
+  controller.activeThread.addOneTimeListener("framesadded", function() {
+    is(gDebugger.DebuggerController.activeThread.state, "paused",
+      "The breakpoint was hit.");
+
+    let thread = gDebugger.DebuggerController.activeThread;
+    thread.addOneTimeListener("paused", function test(aEvent, aPacket) {
+      thread.addOneTimeListener("resumed", function() {
+        executeSoon(closeDebuggerAndFinish);
+      });
+
+      is(aPacket.why.type, "debuggerStatement", "Execution has advanced to the next line.");
+      isnot(aPacket.why.type, "breakpoint", "No ghost breakpoint was hit.");
+      thread.resume();
+    });
+
+    thread.resume();
+  });
+
+  EventUtils.sendMouseEvent({ type: "click" },
+    content.document.querySelector("button"));
+}
+
+function testBreakpointHitAfterReload()
+{
+  is(gDebugger.DebuggerController.activeThread.state, "paused",
+    "The breakpoint was hit.");
+
+  let thread = gDebugger.DebuggerController.activeThread;
+  thread.addOneTimeListener("paused", function test(aEvent, aPacket) {
+    thread.addOneTimeListener("resumed", function() {
+      executeSoon(closeDebuggerAndFinish);
+    });
+
+    is(aPacket.why.type, "debuggerStatement", "Execution has advanced to the next line.");
+    isnot(aPacket.why.type, "breakpoint", "No ghost breakpoint was hit.");
+    thread.resume();
+  });
+
+  thread.resume();
+}
+
+registerCleanupFunction(function() {
+  removeTab(gTab);
+  gPane = null;
+  gTab = null;
+  gDebuggee = null;
+  gDebugger = null;
+});

browser/devtools/debugger/test/test-location-changes-bp.html

@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<html>
+<head>
+<meta charset='utf-8'/>
+<script type="text/javascript" src="test-location-changes-bp.js"></script>
+<script type="text/javascript">
+function runDebuggerStatement() {
+  debugger;
+}
+</script>
+</head>
+<body>
+
+<button type="button" onclick="myFunction()">Run</button>
+
+</body>
+</html>

browser/devtools/debugger/test/test-location-changes-bp.js

@@ -0,0 +1,7 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+function myFunction() {
+  var a = 1;
+  debugger;
+}

toolkit/devtools/debugger/server/dbg-browser-actors.js

@@ -668,9 +668,6 @@ DebuggerProgressListener.prototype = {
     }
 
     if (isStart && aRequest instanceof Ci.nsIChannel) {
-      // If the request is about to happen in a new window, we are not concerned
-      // about the request.
-
       // Proceed normally only if the debuggee is not paused.
       if (this._tabActor.threadActor.state == "paused") {
         aRequest.suspend();
@@ -679,6 +676,7 @@ DebuggerProgressListener.prototype = {
         this._tabActor._pendingNavigation = aRequest;
       }
 
+      this._tabActor.threadActor.disableAllBreakpoints();
       this._tabActor.conn.send({
         from: this._tabActor.actorID,
         type: "tabNavigated",

toolkit/devtools/debugger/server/dbg-script-actors.js

@@ -740,6 +740,22 @@ ThreadActor.prototype = {
     });
   },
 
+  /**
+   * Disassociate all breakpoint actors from their scripts and clear the
+   * breakpoint handlers. This method can be used when the thread actor intends
+   * to keep the breakpoint store, but needs to clear any actual breakpoints,
+   * e.g. due to a page navigation. This way the breakpoint actors' script
+   * caches won't hold on to the Debugger.Script objects leaking memory.
+   */
+  disableAllBreakpoints: function () {
+    for (let url in this._breakpointStore) {
+      for (let line in this._breakpointStore[url]) {
+        let bp = this._breakpointStore[url][line];
+        bp.actor.removeScripts();
+      }
+    }
+  },
+
   /**
    * Handle a protocol request to pause the debuggee.
    */
@@ -1268,8 +1284,11 @@ ThreadActor.prototype = {
         // affect the loop.
         for (let line = existing.length - 1; line >= 0; line--) {
           let bp = existing[line];
-          // Limit search to the line numbers contained in the new script.
-          if (bp && line >= aScript.startLine && line <= endLine) {
+          // Only consider breakpoints that are not already associated with
+          // scripts, and limit search to the line numbers contained in the new
+          // script.
+          if (bp && !bp.actor.scripts.length &&
+              line >= aScript.startLine && line <= endLine) {
             this._setBreakpoint(bp);
           }
         }
@@ -2050,6 +2069,16 @@ BreakpointActor.prototype = {
     this.scripts.push(aScript);
   },
 
+  /**
+   * Remove the breakpoints from associated scripts and clear the script cache.
+   */
+  removeScripts: function () {
+    for (let script of this.scripts) {
+      script.clearBreakpoint(this);
+    }
+    this.scripts = [];
+  },
+
   /**
    * A function that the engine calls when a breakpoint has been hit.
    *
@@ -2079,12 +2108,9 @@ BreakpointActor.prototype = {
     // Remove from the breakpoint store.
     let scriptBreakpoints = this.threadActor._breakpointStore[this.location.url];
     delete scriptBreakpoints[this.location.line];
-    // Remove the actual breakpoint.
     this.threadActor._hooks.removeFromParentPool(this);
-    for (let script of this.scripts) {
-      script.clearBreakpoint(this);
-    }
-    this.scripts = null;
+    // Remove the actual breakpoint from the associated scripts.
+    this.removeScripts();
 
     return { from: this.actorID };
   }