Bug 1138975 - Refactor breakpoint sliding for non-source mapped sources;r=jlongster
☠☠ backed out by e807296e0ae2 ☠ ☠
authorEddy Bruël <ejpbruel@gmail.com>
Fri, 20 Mar 2015 12:11:54 +0100
changeset 251856 de24b63c6966fc65208604461ba3f5477e5acfa8
parent 251855 ef86c7c53d210dcfa697c010c34b655ca10ce232
child 251857 d00d886006d3d8fcd20ffbe8ba42467fa44cde42
push id1156
push userpbrosset@mozilla.com
push dateFri, 20 Mar 2015 16:00:24 +0000
reviewersjlongster
bugs1138975
milestone39.0a1
Bug 1138975 - Refactor breakpoint sliding for non-source mapped sources;r=jlongster
browser/devtools/debugger/test/browser_dbg_source-maps-04.js
toolkit/devtools/server/actors/script.js
toolkit/devtools/server/actors/utils/ScriptStore.js
--- a/browser/devtools/debugger/test/browser_dbg_source-maps-04.js
+++ b/browser/devtools/debugger/test/browser_dbg_source-maps-04.js
@@ -84,21 +84,21 @@ function disableIgnoreCaughtExceptions()
   return deferred.promise;
 }
 
 function testSetBreakpoint() {
   let deferred = promise.defer();
   let sourceForm = getSourceForm(gSources, JS_URL);
   let source = gDebugger.gThreadClient.source(sourceForm);
 
-  source.setBreakpoint({ line: 3, column: 61 }, aResponse => {
+  source.setBreakpoint({ line: 3, column: 60 }, aResponse => {
     ok(!aResponse.error,
       "Should be able to set a breakpoint in a js file.");
     ok(!aResponse.actualLocation,
-      "Should be able to set a breakpoint on line 3 and column 61.");
+      "Should be able to set a breakpoint on line 3 and column 60.");
 
     deferred.resolve();
   });
 
   return deferred.promise;
 }
 
 function reloadPage() {
@@ -110,30 +110,26 @@ 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, 2, "Should have two frames.");
+      is(gFrames.itemCount, 1, "Should have one frame.");
 
       // This is weird, but we need to let the debugger a chance to
       // update first
       executeSoon(() => {
         gDebugger.gThreadClient.resume(() => {
-          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();
-              });
-            });
+          // 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
@@ -2786,68 +2786,238 @@ SourceActor.prototype = {
 
     return this._setBreakpointForActor(actor);
   },
 
   /*
    * Ensure the given BreakpointActor is set as a breakpoint handler on all
    * scripts that match its location in the original source.
    *
-   * 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.
+   * If there are no scripts that match the location of the BreakpointActor,
+   * we slide its location to the next closest line (for line breakpoints) or
+   * column (for column breakpoint) that does.
    *
-   * 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 BreakpointActor is still
-   * pending (i.e. is not set as a breakpoint handler for any script).
+   * If breakpoint sliding fails, then either there are no scripts that contain
+   * any code for the given location, or they were all garbage collected before
+   * the debugger started running. We cannot distinguish between these two
+   * cases, so we insert the BreakpointActor in the BreakpointActorMap as
+   * a pending breakpoint. Whenever a new script is introduced, this method is
+   * called again for each pending breakpoint.
    *
    * @param BreakpointActor actor
    *        The BreakpointActor to be set as a breakpoint handler.
    *
    * @returns A Promise that resolves to the given BreakpointActor.
    */
   _setBreakpointForActor: function (actor) {
+    let { originalLocation } = actor;
+
     if (this.isSourceMapped) {
-      return this.threadActor.sources.getGeneratedLocation(
-        actor.originalLocation
-      ).then((generatedLocation) => {
+      // TODO: Refactor breakpoint sliding for source mapped sources.
+      return this.threadActor.sources.getGeneratedLocation(originalLocation)
+                                     .then((generatedLocation) => {
         return generatedLocation.generatedSourceActor
-                                ._setBreakpointForActorAtLocation(
+                                ._setBreakpointForActorAtLocationWithSliding(
           actor,
           generatedLocation
         );
       });
     } else {
-      return Promise.resolve(this._setBreakpointForActorAtLocation(
-        actor,
-        GeneratedLocation.fromOriginalLocation(actor.originalLocation)
-      ));
+      // If this is a non-source mapped source, the original location and
+      // generated location are the same, so we can safely convert between them.
+      let generatedLocation = GeneratedLocation.fromOriginalLocation(originalLocation);
+      let { generatedColumn } = generatedLocation;
+
+      // Try to set the breakpoint on the generated location directly. If this
+      // succeeds, we can avoid the more expensive breakpoint sliding algorithm
+      // below.
+      if (this._setBreakpointForActorAtLocation(actor, generatedLocation)) {
+        return Promise.resolve(actor);
+      }
+
+      // There were no scripts that matched the given location, so we need to
+      // perform breakpoint sliding.
+      if (generatedColumn === undefined) {
+        // To perform breakpoint sliding for line breakpoints, we need to build
+        // a map from line numbers to a list of entry points for each line,
+        // implemented as a sparse array. An entry point is a (script, offsets)
+        // pair, and represents all offsets in that script that are entry points
+        // for the corresponding line.
+        let lineToEntryPointsMap = [];
+
+        // Iterate over all scripts that correspond to this source actor.
+        let scripts = this.scripts.getScriptsBySourceActor(this);
+        for (let script of scripts) {
+          // Get all offsets for each line in the current script. This returns
+          // a map from line numbers fo a list of offsets for each line,
+          // implemented as a sparse array.
+          let lineToOffsetsMap = script.getAllOffsets();
+
+          // Iterate over each line, and add their list of offsets to the map
+          // from line numbers to entry points by forming a (script, offsets)
+          // pair, where script is the current script, and offsets is the list
+          // of offsets for the current line.
+          for (let line = 0; line < lineToOffsetsMap.length; ++line) {
+            let offsets = lineToOffsetsMap[line];
+            if (offsets) {
+              let entryPoints = lineToEntryPointsMap[line];
+              if (!entryPoints) {
+                // We dont have a list of entry points for the current line
+                // number yet, so create it and add it to the map.
+                entryPoints = [];
+                lineToEntryPointsMap[line] = entryPoints;
+              }
+              entryPoints.push({ script, offsets });
+            }
+          }
+        }
+
+        let {
+          originalSourceActor,
+          originalLine,
+          originalColumn
+        } = originalLocation;
+
+        // Now that we have a map from line numbers to a list of entry points
+        // for each line, we can use it to perform breakpoint sliding. Start
+        // at the original line of the breakpoint actor, and keep incrementing
+        // it by one, until either we find a line that has at least one entry
+        // point, or we go past the last line in the map.
+        //
+        // Note that by computing the entire map up front, and implementing it
+        // as a sparse array, we can easily tell when we went past the last line
+        // in the map.
+        let actualLine = originalLine;
+        while (actualLine < lineToEntryPointsMap.length) {
+          let entryPoints = lineToEntryPointsMap[actualLine];
+          if (entryPoints) {
+            setBreakpointForActorAtEntryPoints(actor, entryPoints);
+            break;
+          }
+          ++actualLine;
+        }
+        if (actualLine === lineToEntryPointsMap.length) {
+          // We went past the last line in the map, so breakpoint sliding
+          // failed. Keep the BreakpointActor in the BreakpointActorMap as a
+          // pending breakpoint, so we can try again whenever a new script is
+          // introduced.
+          return Promise.resolve(actor);
+        }
+
+        // If the actual line on which the BreakpointActor was set differs from
+        // the original line that was requested, the BreakpointActor and the
+        // BreakpointActorMap need to be updated accordingly.
+        if (actualLine !== originalLine) {
+          let actualLocation = new OriginalLocation(
+            originalSourceActor,
+            actualLine
+          );
+          let existingActor = this.breakpointActorMap.getActor(actualLocation);
+          if (existingActor) {
+            actor.onDelete();
+            this.breakpointActorMap.deleteActor(originalLocation);
+            actor = existingActor;
+          } else {
+            this.breakpointActorMap.deleteActor(originalLocation);
+            actor.originalLocation = actualLocation;
+            this.breakpointActorMap.setActor(actualLocation, actor);
+          }
+        }
+
+        return Promise.resolve(actor);
+      } else {
+        // TODO: Implement breakpoint sliding for column breakpoints
+        return Promise.resolve(actor);
+      }
     }
   },
 
   /*
    * Ensure the given BreakpointActor is set as breakpoint handler on all
    * scripts that match the given location in the generated source.
    *
    * @param BreakpointActor actor
    *        The BreakpointActor to be set as a breakpoint handler.
    * @param GeneratedLocation generatedLocation
    *        A GeneratedLocation representing the location in the generated
    *        source for which the given BreakpointActor is to be set as a
    *        breakpoint handler.
+   *
+   * @returns A Boolean that is true if the BreakpointActor was set as a
+   *          breakpoint handler on at least one script, and false otherwise.
    */
   _setBreakpointForActorAtLocation: function (actor, generatedLocation) {
+    let { generatedLine, generatedColumn } = generatedLocation;
+
+    // Find all scripts that match the given source actor and line number.
+    let scripts = this.scripts.getScriptsBySourceActorAndLine(
+      this,
+      generatedLine
+    ).filter((script) => !actor.hasScript(script));
+
+    // Find all entry points that correspond to the given location.
+    let entryPoints = [];
+    if (generatedColumn === undefined) {
+      // This is a line breakpoint, so we are interested in all offsets
+      // that correspond to the given line number.
+      for (let script of scripts) {
+        let offsets = script.getLineOffsets(generatedLine);
+        if (offsets.length > 0) {
+          entryPoints.push({ script, offsets });
+        }
+      }
+    } else {
+      // This is a column breakpoint, so we are interested in all column
+      // offsets that correspond to the given line *and* column number.
+      for (let script of scripts) {
+        let columnToOffsetMap = script.getAllColumnOffsets()
+                                      .filter(({ lineNumber }) => {
+          return lineNumber === generatedLine;
+        });
+        for (let { columnNumber: column, offset } of columnToOffsetMap) {
+          // TODO: What we are actually interested in here is a range of
+          // columns, rather than a single one.
+          if (column == generatedColumn) {
+            entryPoints.push({ script, offsets: [offset] });
+          }
+        }
+      }
+    }
+
+    if (entryPoints.length === 0) {
+      return false;
+    }
+    setBreakpointForActorAtEntryPoints(actor, entryPoints);
+    return true;
+  },
+
+  /*
+   * Ensure the given BreakpointActor is set as breakpoint handler on all
+   * scripts that match the given location in the generated source.
+   *
+   * TODO: This method is bugged, because it performs breakpoint sliding on
+   * generated locations. Breakpoint sliding should be performed on original
+   * locations, because there is no guarantee that the next line in the
+   * generated source corresponds to the next line in an original source.
+   *
+   * The only place this method is still used is from setBreakpointForActor
+   * when called for a source mapped source. Once that code has been refactored,
+   * this method can be removed.
+   *
+   * @param BreakpointActor actor
+   *        The BreakpointActor to be set as a breakpoint handler.
+   * @param GeneratedLocation generatedLocation
+   *        A GeneratedLocation representing the location in the generated
+   *        source for which the given BreakpointActor is to be set as a
+   *        breakpoint handler.
+   *
+   * @returns A Boolean that is true if the BreakpointActor was set as a
+   *          breakpoint handler on at least one script, and false otherwise.
+   */
+  _setBreakpointForActorAtLocationWithSliding: function (actor, generatedLocation) {
     let originalLocation = actor.originalLocation;
     let { generatedLine, generatedColumn } = generatedLocation;
 
     // 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.
     let scripts = this.scripts.getScriptsBySourceActorAndLine(this, generatedLine);
@@ -2962,20 +3132,16 @@ SourceActor.prototype = {
     }
 
     for (let [script, mappings] of scriptsAndOffsetMappings) {
       for (let offsetMapping of mappings) {
         script.setBreakpoint(offsetMapping.offset, actor);
       }
       actor.addScript(script, this.threadActor);
     }
-
-    return {
-      actor: actor.actorID
-    };
   },
 
   /**
    * Find the first line that is associated with bytecode offsets, and is
    * greater than or equal to the given start line.
    *
    * @param Array scripts
    *        The set of Debugger.Script instances to consider.
--- a/toolkit/devtools/server/actors/utils/ScriptStore.js
+++ b/toolkit/devtools/server/actors/utils/ScriptStore.js
@@ -74,16 +74,22 @@ ScriptStore.prototype = {
    *
    * NB: The ScriptStore retains ownership of the returned array, and the
    * ScriptStore's consumers MUST NOT MODIFY its contents!
    */
   getAllScripts() {
     return this._scripts.items;
   },
 
+  getScriptsBySourceActor(sourceActor) {
+    return sourceActor.source ?
+           this.getScriptsBySource(sourceActor.source) :
+           this.getScriptsByURL(sourceActor._originalUrl);
+  },
+
   getScriptsBySourceActorAndLine(sourceActor, line) {
     return sourceActor.source ?
            this.getScriptsBySourceAndLine(sourceActor.source, line) :
            this.getScriptsByURLAndLine(sourceActor._originalUrl, line);
   },
 
   /**
    * Get all scripts produced from the given source.