Bug 1141507 - Refactor breakpoint sliding for source mapped sources;r=jlong
authorEddy Bruël <ejpbruel@gmail.com>
Fri, 27 Mar 2015 07:49:16 +0100
changeset 235965 a752e16a5251cd9b047f41dcef33f1c8d8187517
parent 235964 bf843cab375a04c7823cf8cac76b3f132555ca01
child 235966 ac13bb5f374caff54cf6f71368232a24141c7e16
push id28487
push userryanvm@gmail.com
push dateFri, 27 Mar 2015 15:15:42 +0000
treeherdermozilla-central@94c247bc2480 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjlong
bugs1141507
milestone39.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1141507 - Refactor breakpoint sliding for source mapped sources;r=jlong
toolkit/devtools/server/actors/common.js
toolkit/devtools/server/actors/script.js
toolkit/devtools/server/actors/utils/TabSources.js
toolkit/devtools/server/tests/unit/test_sourcemaps-03.js
--- a/toolkit/devtools/server/actors/common.js
+++ b/toolkit/devtools/server/actors/common.js
@@ -379,21 +379,22 @@ exports.OriginalLocation = OriginalLocat
  *
  * @param SourceActor actor
  *        A SourceActor representing a generated source.
  * @param Number line
  *        A line within the given source.
  * @param Number column
  *        A column within the given line.
  */
-function GeneratedLocation(actor, line, column) {
+function GeneratedLocation(actor, line, column, lastColumn) {
   this._connection = actor ? actor.conn : null;
   this._actorID = actor ? actor.actorID : undefined;
   this._line = line;
   this._column = column;
+  this._lastColumn = (lastColumn !== undefined) ? lastColumn : column + 1;
 }
 
 GeneratedLocation.fromOriginalLocation = function (originalLocation) {
   return new GeneratedLocation(
     originalLocation.originalSourceActor,
     originalLocation.originalLine,
     originalLocation.originalColumn
   );
@@ -425,16 +426,34 @@ GeneratedLocation.prototype = {
   },
 
   get generatedLine() {
     return this._line;
   },
 
   get generatedColumn() {
     return this._column;
+  },
+
+  get generatedLastColumn() {
+    return this._lastColumn;
+  },
+
+  equals: function (other) {
+    return this.generatedSourceActor.url == other.generatedSourceActor.url &&
+           this.generatedLine === other.originalLine;
+  },
+
+  toJSON: function () {
+    return {
+      source: this.generatedSourceActor.form(),
+      line: this.generatedLine,
+      column: this.generatedColumn,
+      lastColumn: this.generatedLastColumn
+    };
   }
 };
 
 exports.GeneratedLocation = GeneratedLocation;
 
 // TODO bug 863089: use Debugger.Script.prototype.getOffsetColumn when it is
 // implemented.
 exports.getOffsetColumn = function getOffsetColumn(aOffset, aScript) {
--- a/toolkit/devtools/server/actors/script.js
+++ b/toolkit/devtools/server/actors/script.js
@@ -2816,17 +2816,17 @@ SourceActor.prototype = {
    *
    * @param BreakpointActor actor
    *        The BreakpointActor to be set as a breakpoint handler.
    *
    * @returns A Promise that resolves to the given BreakpointActor.
    */
   _setBreakpoint: function (actor) {
     let { originalLocation } = actor;
-    let { originalColumn } = originalLocation;
+    let { originalSourceActor, originalLine, originalColumn } = originalLocation;
 
     return this._setBreakpointAtOriginalLocation(actor, originalLocation)
                .then((actualLocation) => {
       if (actualLocation) {
         return actualLocation;
       }
 
       if (!this.isSourceMapped) {
@@ -2862,32 +2862,26 @@ SourceActor.prototype = {
                   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;
+          let actualLine = originalLine + 1;
           while (actualLine < lineToEntryPointsMap.length) {
             let entryPoints = lineToEntryPointsMap[actualLine];
             if (entryPoints) {
               setBreakpointAtEntryPoints(actor, entryPoints);
               break;
             }
             ++actualLine;
           }
@@ -2903,25 +2897,51 @@ SourceActor.prototype = {
             originalSourceActor,
             actualLine
           );
         } else {
           // TODO: Implement breakpoint sliding for column breakpoints
           return originalLocation;
         }
       } else {
-        // TODO: Refactor breakpoint sliding for source mapped sources.
-        return this.threadActor.sources.getGeneratedLocation(originalLocation)
-                                       .then((generatedLocation) => {
-          return generatedLocation.generatedSourceActor
-                                  ._setBreakpointAtLocationWithSliding(
-            actor,
-            generatedLocation
-          );
-        });
+        if (originalColumn === undefined) {
+          let loop = (actualLocation) => {
+            let {
+              originalLine: actualLine,
+              originalColumn: actualColumn
+            } = actualLocation;
+
+            return this.threadActor.sources.getAllGeneratedLocations(actualLocation)
+                                           .then((generatedLocations) => {
+              // Because getAllGeneratedLocations will always return the list of
+              // generated locations for the closest line that is greater than
+              // the one we are searching for if no exact match can be found, if
+              // the list of generated locations is empty, we've reached the end
+              // of the original source, and breakpoint sliding failed.
+              if (generatedLocations.length === 0) {
+                return originalLocation;
+              }
+
+              // If at least one script has an offset that matches one of the
+              // generated locations in the list, then breakpoint sliding
+              // succeeded.
+              if (this._setBreakpointAtAllGeneratedLocations(actor, generatedLocations)) {
+                return this.threadActor.sources.getOriginalLocation(generatedLocations[0]);
+              }
+
+              // Try the next line in the original source.
+              return loop(new OriginalLocation(this, actualLine + 1));
+            });
+          };
+
+          return loop(new OriginalLocation(this, originalLine + 1));
+        } else {
+          // TODO: Implement breakpoint sliding for column breakpoints
+          return originalLocation;
+        }
       }
     }).then((actualLocation) => {
       // If the actual location on which the BreakpointActor ended up being
       // set differs from the original line that was requested, both the
       // BreakpointActor and the BreakpointActorMap need to be updated
       // accordingly.
       if (!actualLocation.equals(originalLocation)) {
         let existingActor = this.breakpointActorMap.getActor(actualLocation);
@@ -2988,21 +3008,26 @@ SourceActor.prototype = {
    *        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.
    */
   _setBreakpointAtGeneratedLocation: function (actor, generatedLocation) {
-    let { generatedLine, generatedColumn } = generatedLocation;
+    let {
+      generatedSourceActor,
+      generatedLine,
+      generatedColumn,
+      generatedLastColumn
+    } = generatedLocation;
 
     // Find all scripts that match the given source actor and line number.
     let scripts = this.scripts.getScriptsBySourceActorAndLine(
-      this,
+      generatedSourceActor,
       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.
@@ -3018,263 +3043,28 @@ SourceActor.prototype = {
       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) {
+          if (column >= generatedColumn && column <= generatedLastColumn) {
             entryPoints.push({ script, offsets: [offset] });
           }
         }
       }
     }
 
     if (entryPoints.length === 0) {
       return false;
     }
     setBreakpointAtEntryPoints(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 setBreakpoint
-   * 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.
-   */
-  _setBreakpointAtLocationWithSliding: 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);
-    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;
-    }
-
-    // Ignore scripts for which the BreakpointActor is already a breakpoint
-    // handler.
-    scripts = scripts.filter((script) => !actor.hasScript(script));
-
-    let actualGeneratedLocation;
-
-    // If generatedColumn is something other than 0, assume this is a column
-    // breakpoint and do not perform breakpoint sliding.
-    if (generatedColumn) {
-      this._setBreakpointAtColumn(scripts, generatedLocation, actor);
-      actualGeneratedLocation = generatedLocation;
-    } else {
-      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, generatedLine);
-      } 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 = findEntryPointsForLine(scripts, generatedLine)
-        if (entryPoints) {
-          result = {
-            line: generatedLine,
-            entryPoints: entryPoints
-          };
-        }
-      }
-
-      if (!result) {
-        return actor;
-      }
-
-      if (result.line !== generatedLine) {
-        actualGeneratedLocation = new GeneratedLocation(
-          generatedLocation.generatedSourceActor,
-          result.line,
-          generatedLocation.generatedColumn
-        );
-      } else {
-        actualGeneratedLocation = generatedLocation;
-      }
-
-      setBreakpointAtEntryPoints(actor, result.entryPoints);
-    }
-
-    return Promise.resolve().then(() => {
-      if (actualGeneratedLocation.generatedSourceActor.source) {
-        return this.threadActor.sources.getOriginalLocation(actualGeneratedLocation);
-      } else {
-        return OriginalLocation.fromGeneratedLocation(actualGeneratedLocation);
-      }
-    });
-  },
-
-  /**
-   * Set breakpoints at the offsets closest to our target location's column.
-   *
-   * @param Array scripts
-   *        The set of Debugger.Script instances to consider.
-   * @param Object location
-   *        The target location.
-   * @param BreakpointActor actor
-   *        The BreakpointActor to handle hitting the breakpoints we set.
-   * @returns Object
-   *          The RDP response.
-   */
-  _setBreakpointAtColumn: function (scripts, generatedLocation, actor) {
-    // Debugger.Script -> array of offset mappings
-    const scriptsAndOffsetMappings = new Map();
-
-    for (let script of scripts) {
-      this._findClosestOffsetMappings(generatedLocation,
-                                      script,
-                                      scriptsAndOffsetMappings);
-    }
-
-    for (let [script, mappings] of scriptsAndOffsetMappings) {
-      for (let offsetMapping of mappings) {
-        script.setBreakpoint(offsetMapping.offset, actor);
-      }
-      actor.addScript(script, this.threadActor);
-    }
-  },
-
-  /**
-   * 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.
-   * @param Number startLine
-   *        The target line.
-   * @return Object|null
-   *         If we can't find a line matching our constraints, return
-   *         null. Otherwise, return an object of the form:
-   *           {
-   *             line: Number,
-   *             entryPoints: [
-   *               { script: Debugger.Script, offsets: [offset, ...] },
-   *               ...
-   *             ]
-   *           }
-   */
-  _findNextLineWithOffsets: function (scripts, startLine) {
-    const maxLine = Math.max(...scripts.map(s => s.startLine + s.lineCount));
-
-    for (let line = startLine; line < maxLine; line++) {
-      const entryPoints = findEntryPointsForLine(scripts, line);
-      if (entryPoints.length) {
-        return { line, entryPoints };
-      }
-    }
-
-    return null;
-  },
-
-  /**
-   * Find all of the offset mappings associated with `aScript` that are closest
-   * to `aTargetLocation`. If new offset mappings are found that are closer to
-   * `aTargetOffset` than the existing offset mappings inside
-   * `aScriptsAndOffsetMappings`, we empty that map and only consider the
-   * closest offset mappings.
-   *
-   * In many cases, but not all, this method finds only one closest offset.
-   * Consider the following case, where multiple offsets will be found:
-   *
-   *     0         1         2         3
-   *     0123456789012345678901234567890
-   *    +-------------------------------
-   *   1|function f() {
-   *   2|  return g() + h();
-   *   3|}
-   *
-   * The Debugger reports three offsets on line 2 upon which we could set a
-   * breakpoint: the `return` statement at column 2, the call expression `g()`
-   * at column 9, and the call expression `h()` at column 15. (Careful readers
-   * will note that complete source location information isn't saved by
-   * SpiderMonkey's frontend, and we don't get an offset associated specifically
-   * with the `+` operation.)
-   *
-   * If our target location is line 2 column 12, the offset for the call to `g`
-   * is 3 columns to the left and the offset for the call to `h` is 3 columns to
-   * the right. Because they are equally close, we will return both offsets to
-   * have breakpoints set upon them.
-   *
-   * @param Object aTargetLocation
-   *        An object of the form { url, line[, column] }.
-   * @param Debugger.Script aScript
-   *        The script in which we are searching for offsets.
-   * @param Map aScriptsAndOffsetMappings
-   *        A Map object which maps Debugger.Script instances to arrays of
-   *        offset mappings. This is an out param.
-   */
-  _findClosestOffsetMappings: function (aTargetLocation,
-                                aScript,
-                                aScriptsAndOffsetMappings) {
-    let offsetMappings = aScript.getAllColumnOffsets()
-      .filter(({ lineNumber }) => lineNumber === aTargetLocation.generatedLine);
-
-    // Attempt to find the current closest offset distance from the target
-    // location by grabbing any offset mapping in the map by doing one iteration
-    // and then breaking (they all have the same distance from the target
-    // location).
-    let closestDistance = Infinity;
-    if (aScriptsAndOffsetMappings.size) {
-      for (let mappings of aScriptsAndOffsetMappings.values()) {
-        closestDistance = Math.abs(aTargetLocation.generatedColumn - mappings[0].columnNumber);
-        break;
-      }
-    }
-
-    for (let mapping of offsetMappings) {
-      let currentDistance = Math.abs(aTargetLocation.generatedColumn - mapping.columnNumber);
-
-      if (currentDistance > closestDistance) {
-        continue;
-      } else if (currentDistance < closestDistance) {
-        closestDistance = currentDistance;
-        aScriptsAndOffsetMappings.clear();
-        aScriptsAndOffsetMappings.set(aScript, [mapping]);
-      } else {
-        if (!aScriptsAndOffsetMappings.has(aScript)) {
-          aScriptsAndOffsetMappings.set(aScript, []);
-        }
-        aScriptsAndOffsetMappings.get(aScript).push(mapping);
-      }
-    }
   }
 };
 
 SourceActor.prototype.requestTypes = {
   "source": SourceActor.prototype.onSource,
   "blackbox": SourceActor.prototype.onBlackBox,
   "unblackbox": SourceActor.prototype.onUnblackBox,
   "prettyPrint": SourceActor.prototype.onPrettyPrint,
--- a/toolkit/devtools/server/actors/utils/TabSources.js
+++ b/toolkit/devtools/server/actors/utils/TabSources.js
@@ -586,26 +586,59 @@ TabSources.prototype = {
       }
 
       // No source map
       return OriginalLocation.fromGeneratedLocation(generatedLocation);
     });
   },
 
   getAllGeneratedLocations: function (originalLocation) {
-    return this.getGeneratedLocation(originalLocation)
-               .then((generatedLocation) => {
-      if (generatedLocation.generatedLine === null &&
-          generatedLocation.generatedColumn === null) {
-        return [];
-      }
-      return [generatedLocation];
-    });
+    let {
+      originalSourceActor,
+      originalLine,
+      originalColumn
+    } = originalLocation;
+
+    if (originalColumn === undefined) {
+      let source = originalSourceActor.source ||
+                   originalSourceActor.generatedSource;
+
+      return this.fetchSourceMap(source).then((map) => {
+        if (map) {
+          map.computeColumnSpans();
+
+          return map.allGeneratedPositionsFor({
+            source: originalSourceActor.url,
+            line: originalLine
+          }).map(({ line, column, lastColumn }) => {
+            return new GeneratedLocation(
+              this.createNonSourceMappedActor(source),
+              line,
+              column,
+              lastColumn
+            );
+          });
+        }
+
+        return [GeneratedLocation.fromOriginalLocation(originalLocation)];
+      });
+    } else {
+      // TODO: Some source maps have multiple mappings for the same column
+      return this.getGeneratedLocation(originalLocation)
+                 .then((generatedLocation) => {
+        if (generatedLocation.generatedLine === null &&
+            generatedLocation.generatedColumn === null) {
+          return [];
+        }
+        return [generatedLocation];
+      });
+    }
   },
 
+
   /**
    * Returns a promise of the location in the generated source corresponding to
    * the original source and line given.
    *
    * When we pass a script S representing generated code to `sourceMap`,
    * above, that returns a promise P. The process of resolving P populates
    * the tables this function uses; thus, it won't know that S's original
    * source URLs map to S until P is resolved.
--- a/toolkit/devtools/server/tests/unit/test_sourcemaps-03.js
+++ b/toolkit/devtools/server/tests/unit/test_sourcemaps-03.js
@@ -31,18 +31,17 @@ function testBreakpointMapping(aName, aC
   gThreadClient.addOneTimeListener("paused", function (aEvent, aPacket) {
     do_check_true(!aPacket.error);
     do_check_eq(aPacket.why.type, "debuggerStatement");
 
     getSource(gThreadClient, "http://example.com/www/js/" + aName + ".js").then(source => {
       source.setBreakpoint({
         // Setting the breakpoint on an empty line so that it is pushed down one
         // line and we can check the source mapped actualLocation later.
-        line: 3,
-        column: 0
+        line: 3
       }, function (aResponse) {
         do_check_true(!aResponse.error);
 
         // Actual location should come back source mapped still so that
         // breakpoints are displayed in the UI correctly, etc.
         do_check_eq(aResponse.actualLocation.line, 4);
         do_check_eq(aResponse.actualLocation.source.url,
                     "http://example.com/www/js/" + aName + ".js");