Bug 1122064 and bug 1107682 - Fix breakpoints on deeply nested scripts. r=fitzgen, a=sledru
authorJames Long <longster@gmail.com>
Fri, 23 Jan 2015 10:17:00 -0500
changeset 240334 9200568035366e02c1efcc12d6690491aebc0261
parent 240333 a6bc0837b6d38bef073f93e1f581bfe2009d8df4
child 240335 978caf0fb187dc8e21c6413cae146dfaefac4682
push id7520
push userryanvm@gmail.com
push dateMon, 26 Jan 2015 16:56:30 +0000
treeherdermozilla-aurora@978caf0fb187 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfitzgen, sledru
bugs1122064, 1107682
milestone37.0a2
Bug 1122064 and bug 1107682 - Fix breakpoints on deeply nested scripts. r=fitzgen, a=sledru
browser/devtools/debugger/test/browser_dbg_source-maps-04.js
toolkit/devtools/server/actors/script.js
toolkit/devtools/server/tests/unit/test_breakpoint-21.js
toolkit/devtools/server/tests/unit/xpcshell.ini
--- a/browser/devtools/debugger/test/browser_dbg_source-maps-04.js
+++ b/browser/devtools/debugger/test/browser_dbg_source-maps-04.js
@@ -110,26 +110,30 @@ function reloadPage() {
 function testHitBreakpoint() {
   let deferred = promise.defer();
 
   gDebugger.gThreadClient.resume(aResponse => {
     ok(!aResponse.error, "Shouldn't get an error resuming.");
     is(aResponse.type, "resumed", "Type should be 'resumed'.");
 
     waitForDebuggerEvents(gPanel, gDebugger.EVENTS.FETCHED_SCOPES).then(() => {
-      is(gFrames.itemCount, 1, "Should have one frame.");
+      is(gFrames.itemCount, 2, "Should have two frames.");
 
       // This is weird, but we need to let the debugger a chance to
       // update first
       executeSoon(() => {
         gDebugger.gThreadClient.resume(() => {
-          // We also need to make sure the next step doesn't add a
-          // "resumed" handler until this is completely finished
-          executeSoon(() => {
-            deferred.resolve();
+          gDebugger.gThreadClient.addOneTimeListener("paused", () => {
+            gDebugger.gThreadClient.resume(() => {
+              // We also need to make sure the next step doesn't add a
+              // "resumed" handler until this is completely finished
+              executeSoon(() => {
+                deferred.resolve();
+              });
+            });
           });
         });
       });
     });
   });
 
   return deferred.promise;
 }
--- a/toolkit/devtools/server/actors/script.js
+++ b/toolkit/devtools/server/actors/script.js
@@ -1963,24 +1963,24 @@ ThreadActor.prototype = {
    *        A Debugger.Object instance whose referent is the global object.
    */
   onNewScript: function (aScript, aGlobal) {
     // XXX: The scripts must be added to the ScriptStore before restoring
     // breakpoints in _addScript. If we try to add them to the ScriptStore
     // inside _addScript, we can accidentally set a breakpoint in a top level
     // script as a "closest match" because we wouldn't have added the child
     // scripts to the ScriptStore yet.
-    this.scripts.addScript(aScript);
-    this.scripts.addScripts(aScript.getChildScripts());
+    this.scripts.addScripts(this.dbg.findScripts({ source: aScript.source }));
 
     this._addScript(aScript);
 
-    // |onNewScript| is only fired for top level scripts (AKA staticLevel == 0),
-    // so we have to make sure to call |_addScript| on every child script as
-    // well to restore breakpoints in those scripts.
+    // `onNewScript` is only fired for top-level scripts (AKA staticLevel == 0),
+    // but top-level scripts have the wrong `lineCount` sometimes (bug 979094),
+    // so iterate over the immediate children to activate breakpoints for now
+    // (TODO bug 1124258: don't do this when `lineCount` bug is fixed)
     for (let s of aScript.getChildScripts()) {
       this._addScript(s);
     }
   },
 
   onNewSource: function (aSource) {
     this.conn.send({
       from: this.actorID,
@@ -2028,17 +2028,17 @@ ThreadActor.prototype = {
 
     // Set any stored breakpoints.
     let endLine = aScript.startLine + aScript.lineCount - 1;
     let source = this.sources.createNonSourceMappedActor(aScript.source);
     for (let bpActor of this.breakpointActorMap.findActors({ source: source.form() })) {
       // Limit the search to the line numbers contained in the new script.
       if (bpActor.location.line >= aScript.startLine
           && bpActor.location.line <= endLine) {
-        source.setBreakpoint(bpActor.location, aScript);
+        source.setBreakpoint(bpActor.location);
       }
     }
 
     // Go ahead and establish the source actors for this script, which
     // fetches sourcemaps if available and sends onNewSource
     // notifications
     this.sources.createSourceActors(aScript.source);
 
@@ -2898,67 +2898,97 @@ SourceActor.prototype = {
         return { line, entryPoints };
       }
     }
 
     return null;
   },
 
   /**
-   * Set a breakpoint using the Debugger API. If the line on which the
-   * breakpoint is being set contains no code, then the breakpoint will slide
-   * down to the next line that has runnable code. In this case the server
-   * breakpoint cache will be updated, so callers that iterate over the
-   * breakpoint cache should take that into account.
+   * Get or create a BreakpointActor for the given location, and set it as a
+   * breakpoint handler on all scripts that match the given location for which
+   * the BreakpointActor is not already a breakpoint handler.
+   *
+   * It is possible that no scripts match the given location, because they have
+   * all been garbage collected. In that case, the BreakpointActor is not set as
+   * a breakpoint handler for any script, but is still inserted in the
+   * BreakpointActorMap as a pending breakpoint. Whenever a new script is
+   * introduced, we call this method again to see if there are now any scripts
+   * that matches the given location.
+   *
+   * The first time we find one or more scripts that matches the given location,
+   * we check if any of these scripts has any entry points for the given
+   * location. If not, we assume that the given location does not have any code.
+   *
+   * If the given location does not contain any code, we slide the breakpoint
+   * down to the next closest line that does, and update the BreakpointActorMap
+   * accordingly. Note that we only do so if the breakpoint actor is still
+   * pending (i.e. is not set as a breakpoint handler for any script).
    *
    * @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, aOnlyThisScript=null) {
+  setBreakpoint: function (aLocation) {
     const location = {
       source: this.form(),
       line: aLocation.line,
       column: aLocation.column,
       condition: aLocation.condition
     };
     const actor = location.actor = this._getOrCreateBreakpointActor(location);
 
     // Find all scripts matching the given location. We will almost always have
     // a `source` object to query, but multiple inline HTML scripts are all
     // represented by a single SourceActor even though they have separate source
     // objects, so we need to query based on the url of the page for them.
-    const scripts = this.source
+    let scripts = this.source
       ? this.scripts.getScriptsBySourceAndLine(this.source, location.line)
       : this.scripts.getScriptsByURLAndLine(this._originalUrl, location.line);
 
     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 the breakpoint will be set at that time. This is similar to
       // GDB's "pending" breakpoints for shared libraries that aren't loaded
       // yet.
       return {
         actor: actor.actorID
       }
     }
 
+    // Ignore scripts for which the BreakpointActor is already a breakpoint
+    // handler.
+    scripts = scripts.filter((script) => !actor.hasScript(script));
+
     if (location.column) {
       return this._setBreakpointAtColumn(scripts, location, actor);
     }
 
-    // Select the first line that has offsets, and is greater than or equal to
-    // the requested line. Set breakpoints on each of the offsets that is an
-    // entry point to our selected line.
-
-    const result = this._findNextLineWithOffsets(scripts, location.line);
+    let result;
+    if (actor.scripts.size === 0) {
+      // If the BreakpointActor is not a breakpoint handler for any script, its
+      // location is not yet fixed. Use breakpoint sliding to select the first
+      // line greater than or equal to the requested line that has one or more
+      // offsets.
+      result = this._findNextLineWithOffsets(scripts, location.line);
+    } else {
+      // If the BreakpointActor is a breakpoint handler for at least one script,
+      // breakpoint sliding has already been carried out, so select the
+      // requested line, even if it does not have any offsets.
+      let entryPoints = this._findEntryPointsForLine(scripts, location.line)
+      if (entryPoints) {
+        result = {
+          line: location.line,
+          entryPoints: entryPoints
+        };
+      }
+    }
+
     if (!result) {
       return {
         error: "noCodeAtLineColumn",
         actor: actor.actorID
       };
     }
 
     const { line, entryPoints } = result;
@@ -2986,22 +3016,17 @@ SourceActor.prototype = {
           sourceActor: this,
           line: actualLocation.line
         };
         this.breakpointActorMap.deleteActor(location);
         this.breakpointActorMap.setActor(actualLocation, actor);
       }
     }
 
-    this._setBreakpointOnEntryPoints(
-      actor,
-      aOnlyThisScript
-        ? entryPoints.filter(o => o.script === aOnlyThisScript)
-        : entryPoints
-    );
+    this._setBreakpointOnEntryPoints(actor, entryPoints);
 
     return {
       actor: actor.actorID,
       actualLocation
     };
   },
 
   /**
@@ -4689,16 +4714,20 @@ function BreakpointActor(aThreadActor, {
   this.location = { sourceActor, line, column };
   this.condition = condition;
 }
 
 BreakpointActor.prototype = {
   actorPrefix: "breakpoint",
   condition: null,
 
+  hasScript: function (aScript) {
+    return this.scripts.has(aScript);
+  },
+
   /**
    * Called when this same breakpoint is added to another Debugger.Script
    * instance.
    *
    * @param aScript Debugger.Script
    *        The new source script on which the breakpoint has been set.
    * @param ThreadActor aThreadActor
    *        The parent thread actor that contains this breakpoint.
new file mode 100644
--- /dev/null
+++ b/toolkit/devtools/server/tests/unit/test_breakpoint-21.js
@@ -0,0 +1,85 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Bug 1122064 - make sure that scripts introduced via onNewScripts
+ * properly populate the `ScriptStore` with all there nested child
+ * scripts, so you can set breakpoints on deeply nested scripts
+ */
+
+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);
+  gClient = new DebuggerClient(aServer.connectPipe());
+  gClient.connect(function () {
+    attachTestTabAndResume(gClient,
+                           "test-breakpoints",
+                           function (aResponse, aTabClient, aThreadClient) {
+      gThreadClient = aThreadClient;
+      test();
+    });
+  });
+}
+
+const test = Task.async(function*() {
+  // Populate the `ScriptStore` so that we only test that the script
+  // is added through `onNewScript`
+  yield getSources(gThreadClient);
+
+  let packet = yield executeOnNextTickAndWaitForPause(evalCode, gClient);
+  let source = gThreadClient.source(packet.frame.where.source);
+  let location = {
+    line: gDebuggee.line0 + 8
+  };
+
+  let [res, bpClient] = yield setBreakpoint(source, location);
+  ok(!res.error);
+
+  yield resume(gThreadClient);
+  packet = yield waitForPause(gClient);
+  do_check_eq(packet.type, "paused");
+  do_check_eq(packet.why.type, "breakpoint");
+  do_check_eq(packet.why.actors[0], bpClient.actor);
+  do_check_eq(packet.frame.where.source.actor, source.actor);
+  do_check_eq(packet.frame.where.line, location.line);
+
+  yield resume(gThreadClient);
+  finishClient(gClient);
+});
+
+function evalCode() {
+  // Start a new script
+  Components.utils.evalInSandbox(
+    "var line0 = Error().lineNumber;\n(" + function() {
+      debugger;
+      var a = (function() {
+        return (function() {
+          return (function() {
+            return (function() {
+              return (function() {
+                var x = 10; // This line gets a breakpoint
+                return 1;
+              })();
+            })();
+          })();
+        })();
+      })();
+    } + ")()",
+    gDebuggee
+  );
+}
--- a/toolkit/devtools/server/tests/unit/xpcshell.ini
+++ b/toolkit/devtools/server/tests/unit/xpcshell.ini
@@ -116,16 +116,17 @@ reason = bug 820380
 [test_breakpoint-15.js]
 [test_breakpoint-16.js]
 [test_breakpoint-17.js]
 [test_breakpoint-18.js]
 [test_breakpoint-19.js]
 skip-if = true
 reason = bug 1104838
 [test_breakpoint-20.js]
+[test_breakpoint-21.js]
 [test_conditional_breakpoint-01.js]
 [test_conditional_breakpoint-02.js]
 [test_conditional_breakpoint-03.js]
 [test_eventlooplag_actor.js]
 run-if = toolkit == "gonk"
 [test_listsources-01.js]
 [test_listsources-02.js]
 [test_listsources-03.js]