Bug 1862693 - Use 1-origin column number in debugger API. r=iain,ocha…

…meau,devtools-reviewers

This includes the following API:
  * Debugger.Object.prototype.createSource
     * startColumn field of the parameter object
  * Debugger.Script.prototype.startColumn
     * return value
  * Debugger.Script.prototype.getPossibleBreakpoints
     * minColumn and maxColumn property of the query
     * columnNumber property of the returned array elements
  * Debugger.Script.prototype.getOffsetMetadata
     * columnNumber property of the returned array elements
  * Debugger.Script.prototype.getOffsetLocation
     * columnNumber property of the returned array elements
  * Debugger.Script.prototype.getAllColumnOffsets
     * columnNumber property of the returned array elements
  * Debugger.Script.prototype.getOffsetsCoverage
     * columnNumber property of the returned array elements
  * Debugger.Source.prototype.startColumn
     * return value

This patch modifies DevTools code to convert the column number from/to 1-origin,
while keep using 0-origin on their side.

One exception is the WASM's column number, which had been using 1-origin 1.
Each consumer in DevTools handles the WASM case, and the code can be removed
once DevTools internal also switches to 1-origin column number.

The other exception is to use 1-based column number in logCustomFormatterError,
which is folded from bug 1864783 patch.

Differential Revision: https://phabricator.services.mozilla.com/D193270

devtools/client/debugger/src/utils/location.js

@@ -27,10 +27,10 @@ export function createLocation({
     sourceActor,
     sourceActorId: sourceActor?.id,
 
-    // `line` is 1-based while `column` is 0-based.
+    // `line` and `column` are 1-based.
     // This data is mostly coming from and driven by
     // JSScript::lineno and JSScript::column
-    // https://searchfox.org/mozilla-central/rev/d81e60336d9f498ad3985491dc17c2b77969ade4/js/src/vm/JSScript.h#1544-1547
+    // https://searchfox.org/mozilla-central/rev/90dce6b0223b4dc17bb10f1125b44f70951585f9/js/src/vm/JSScript.h#1545-1548
     line,
     column,
   };

devtools/client/webconsole/test/browser/browser_webconsole_custom_formatters_errors.js

@@ -47,7 +47,7 @@ async function testHeaderNotReturningJsonMl(hud) {
   info(`Test for "header" not returning JsonML`);
   await testCustomFormatting(hud, {
     messageText: `Custom formatter failed: devtoolsFormatters[1].header should return an array, got number`,
-    source: "test-console-custom-formatters-errors.html:19:18",
+    source: "test-console-custom-formatters-errors.html:19:19",
   });
 }
 

devtools/docs/user/debugger-api/debugger.script/index.rst

@@ -174,7 +174,7 @@ The functions described below may only be called with a ``this`` value referring
   The elements of the array are objects, each of which describes a single entry point, and contains the following properties:
 
   - lineNumber: the line number for which offset is an entry point
-  - columnNumber: the column number for which offset is an entry point
+  - columnNumber: the 1-based column number for which offset is an entry point
   - offset: the bytecode instruction offset of the entry point
 
 
@@ -191,10 +191,10 @@ The functions described below may only be called with a ``this`` value referring
 
   .. code-block:: javascript
 
-    [{ lineNumber: 0, columnNumber: 0, offset: 0 },
-     { lineNumber: 1, columnNumber: 5, offset: 5 },
-     { lineNumber: 1, columnNumber: 10, offset: 20 },
-     { lineNumber: 3, columnNumber: 4, offset: 10 }]
+    [{ lineNumber: 0, columnNumber: 1, offset: 0 },
+     { lineNumber: 1, columnNumber: 6, offset: 5 },
+     { lineNumber: 1, columnNumber: 11, offset: 20 },
+     { lineNumber: 3, columnNumber: 5, offset: 10 }]
 
   **If the instance refers to WebAssembly code**, throw a ``TypeError``.
 
@@ -205,15 +205,15 @@ The functions described below may only be called with a ``this`` value referring
   **If the instance refers to a JSScript**, return an object describing the source code location responsible for the bytecode at *offset* in this script. The object has the following properties:
 
   - lineNumber: the line number for which offset is an entry point
-  - columnNumber: the column number for which offset is an entry point
+  - columnNumber: the 1-based column number for which offset is an entry point
   - isEntryPoint: true if the offset is a column entry point, as would be reported by getAllColumnOffsets(); otherwise false.
 
 
 ``getOffsetsCoverage()``:
   **If the instance refers to a JSScript**, return ``null`` or an array which contains information about the coverage of all opcodes. The elements of the array are objects, each of which describes a single opcode, and contains the following properties:
 
   - lineNumber: the line number of the current opcode.
-  - columnNumber: the column number of the current opcode.
+  - columnNumber: the 1-based column number of the current opcode.
   - offset: the bytecode instruction offset of the current opcode.
   - count: the number of times the current opcode got executed.
 

devtools/docs/user/debugger-api/debugger/index.rst

@@ -238,8 +238,6 @@ The functions described below may only be called with a ``this`` value referring
     The script’s ``source`` property must be equal to this value.
   ``line``
     The script must at least partially cover the given source line. If this property is present, the ``url`` property must be present as well.
-  ``column``
-    The script must include given column on the line given by the ``line`` property. If this property is present, the ``url`` and ``line`` properties must both be present as well.
   ``innermost``
     If this property is present and true, the script must be the innermost script covering the given source location; scripts of enclosing code are omitted.
   ``global``

devtools/server/actors/inspector/event-collector.js

@@ -968,8 +968,13 @@ class EventCollector {
       if (script) {
         const scriptSource = script.source.text;
 
+        // NOTE: Debugger.Script.prototype.startColumn is 1-based.
+        //       Convert to 0-based, while keeping the wasm's column (1) as is.
+        //       (bug 1863878)
+        const columnBase = script.format === "wasm" ? 0 : 1;
+
         line = script.startLine;
-        column = script.startColumn;
+        column = script.startColumn - columnBase;
         url = script.url;
         const actor = this.targetActor.sourcesManager.getOrCreateSourceActor(
           script.source

devtools/server/actors/inspector/node.js

@@ -506,10 +506,15 @@ class NodeActor extends Actor {
       return undefined;
     }
 
+    // NOTE: Debugger.Script.prototype.startColumn is 1-based.
+    //       Convert to 0-based, while keeping the wasm's column (1) as is.
+    //       (bug 1863878)
+    const columnBase = customElementDO.script.format === "wasm" ? 0 : 1;
+
     return {
       url: customElementDO.script.url,
       line: customElementDO.script.startLine,
-      column: customElementDO.script.startColumn,
+      column: customElementDO.script.startColumn - columnBase,
     };
   }
 

devtools/server/actors/object/previewers.js

@@ -156,10 +156,14 @@ const previewers = {
       grip.isGenerator = obj.isGeneratorFunction;
 
       if (obj.script) {
+        // NOTE: Debugger.Script.prototype.startColumn is 1-based.
+        //       Convert to 0-based, while keeping the wasm's column (1) as is.
+        //       (bug 1863878)
+        const columnBase = obj.script.format === "wasm" ? 0 : 1;
         grip.location = {
           url: obj.script.url,
           line: obj.script.startLine,
-          column: obj.script.startColumn,
+          column: obj.script.startColumn - columnBase,
         };
       }
 

devtools/server/actors/source.js

@@ -193,6 +193,11 @@ class SourceActor extends Actor {
       introductionType = "scriptElement";
     }
 
+    // NOTE: Debugger.Source.prototype.startColumn is 1-based.
+    //       Convert to 0-based, while keeping the wasm's column (1) as is.
+    //       (bug 1863878)
+    const columnBase = source.introductionType === "wasm" ? 0 : 1;
+
     return {
       actor: this.actorID,
       extensionName: this.extensionName,
@@ -206,7 +211,7 @@ class SourceActor extends Actor {
       introductionType,
       isInlineSource: this._isInlineSource,
       sourceStartLine: source.startLine,
-      sourceStartColumn: source.startColumn,
+      sourceStartColumn: source.startColumn - columnBase,
       sourceLength: source.text?.length,
     };
   }
@@ -411,18 +416,23 @@ class SourceActor extends Actor {
         return false;
       }
 
+      // NOTE: Debugger.Script.prototype.startColumn is 1-based.
+      //       Convert to 0-based, while keeping the wasm's column (1) as is.
+      //       (bug 1863878)
+      const columnBase = script.format === "wasm" ? 0 : 1;
       if (
         script.startLine > endLine ||
         script.startLine + lineCount <= startLine ||
-        (script.startLine == endLine && script.startColumn > endColumn)
+        (script.startLine == endLine &&
+          script.startColumn - columnBase > endColumn)
       ) {
         return false;
       }
 
       if (
         lineCount == 1 &&
         script.startLine == startLine &&
-        script.startColumn + script.sourceLength <= startColumn
+        script.startColumn - columnBase + script.sourceLength <= startColumn
       ) {
         return false;
       }
@@ -469,20 +479,25 @@ class SourceActor extends Actor {
       end: { line: endLine = Infinity, column: endColumn = Infinity } = {},
     } = query || {};
 
+    // NOTE: Debugger.Script.prototype.startColumn is 1-based.
+    //       Convert to 0-based, while keeping the wasm's column (1) as is.
+    //       (bug 1863878)
+    const columnBase = script.format === "wasm" ? 0 : 1;
+
     const offsets = script.getPossibleBreakpoints();
     for (const { lineNumber, columnNumber } of offsets) {
       if (
         lineNumber < startLine ||
-        (lineNumber === startLine && columnNumber < startColumn) ||
+        (lineNumber === startLine && columnNumber - columnBase < startColumn) ||
         lineNumber > endLine ||
-        (lineNumber === endLine && columnNumber >= endColumn)
+        (lineNumber === endLine && columnNumber - columnBase >= endColumn)
       ) {
         continue;
       }
 
       positions.push({
         line: lineNumber,
-        column: columnNumber,
+        column: columnNumber - columnBase,
       });
     }
   }
@@ -610,6 +625,11 @@ class SourceActor extends Actor {
         script => !actor.hasScript(script)
       );
 
+      // NOTE: Debugger.Script.prototype.getPossibleBreakpoints returns
+      //       columnNumber in 1-based.
+      //       The following code uses columnNumber only for comparing against
+      //       other columnNumber, and we don't need to convert to 0-based.
+
       // This is a line breakpoint, so we add a breakpoint on the first
       // breakpoint on the line.
       const lineMatches = [];
@@ -644,13 +664,19 @@ class SourceActor extends Actor {
       );
 
       for (const script of scripts) {
+        // NOTE: getPossibleBreakpoints's minColumn/maxColumn parameters are
+        //       1-based.
+        //       Convert to 1-based, while keeping the wasm's column (1) as is.
+        //       (bug 1863878)
+        const columnBase = script.format === "wasm" ? 0 : 1;
+
         // Check to see if the script contains a breakpoint position at
         // this line and column.
         const possibleBreakpoint = script
           .getPossibleBreakpoints({
             line,
-            minColumn: column,
-            maxColumn: column + 1,
+            minColumn: column + columnBase,
+            maxColumn: column + columnBase + 1,
           })
           .pop();
 

devtools/server/actors/thread.js

@@ -2254,7 +2254,11 @@ class ThreadActor extends Actor {
           ...content.substring(0, scriptStartOffset).matchAll("\n"),
         ];
         const startLine = 1 + allLineBreaks.length;
+        // NOTE: Debugger.Source.prototype.startColumn is 1-based.
+        //       Create 1-based column here for the following comparison,
+        //       and also the createSource call below.
         const startColumn =
+          1 +
           scriptStartOffset -
           (allLineBreaks.length ? allLineBreaks.at(-1).index - 1 : 0);
 
@@ -2270,6 +2274,7 @@ class ThreadActor extends Actor {
 
         try {
           const global = this.dbg.getDebuggees()[0];
+          // NOTE: Debugger.Object.prototype.createSource takes 1-based column.
           this._addSource(
             global.createSource({
               text,

devtools/server/actors/tracer.js

@@ -149,8 +149,20 @@ class TracerActor extends Actor {
     const { lineNumber, columnNumber } = script.getOffsetMetadata(frame.offset);
     const url = script.source.url;
 
+    // NOTE: Debugger.Script.prototype.getOffsetMetadata returns
+    //       columnNumber in 1-based.
+    //       Convert to 0-based, while keeping the wasm's column (1) as is.
+    //       (bug 1863878)
+    const columnBase = script.format === "wasm" ? 0 : 1;
+
     // Ignore blackboxed sources
-    if (this.sourcesManager.isBlackBoxed(url, lineNumber, columnNumber)) {
+    if (
+      this.sourcesManager.isBlackBoxed(
+        url,
+        lineNumber,
+        columnNumber - columnBase
+      )
+    ) {
       return false;
     }
 
@@ -185,7 +197,7 @@ class TracerActor extends Actor {
     this.throttledConsoleMessages.push({
       filename: url,
       lineNumber,
-      columnNumber,
+      columnNumber: columnNumber - columnBase,
       arguments: args,
       styles: CONSOLE_ARGS_STYLES,
       level: "logTrace",

devtools/server/actors/utils/sources-manager.js

@@ -228,10 +228,14 @@ class SourcesManager extends EventEmitter {
    */
   getScriptOffsetLocation(script, offset) {
     const { lineNumber, columnNumber } = script.getOffsetMetadata(offset);
+    // NOTE: Debugger.Source.prototype.startColumn is 1-based.
+    //       Convert to 0-based, while keeping the wasm's column (1) as is.
+    //       (bug 1863878)
+    const columnBase = script.format === "wasm" ? 0 : 1;
     return new SourceLocation(
       this.createSourceActor(script.source),
       lineNumber,
-      columnNumber
+      columnNumber - columnBase
     );
   }
 

devtools/server/tracer/tracer.jsm

@@ -291,6 +291,11 @@ class JavaScriptTracer {
       // but if DevTools are closed, stdout is the only way to log the traces.
       if (shouldLogToStdout) {
         const { script } = frame;
+        // NOTE: Debugger.Script.prototype.getOffsetMetadata returns
+        //       columnNumber in 1-based.
+        //       Convert to 0-based, while keeping the wasm's column (1) as is.
+        //       (bug 1863878)
+        const columnBase = script.format === "wasm" ? 0 : 1;
         const { lineNumber, columnNumber } = script.getOffsetMetadata(
           frame.offset
         );
@@ -305,7 +310,9 @@ class JavaScriptTracer {
 
         // Use a special URL, including line and column numbers which Firefox
         // interprets as to be opened in the already opened DevTool's debugger
-        const href = `${script.source.url}:${lineNumber}:${columnNumber}`;
+        const href = `${script.source.url}:${lineNumber}:${
+          columnNumber - columnBase
+        }`;
 
         // Use special characters in order to print working hyperlinks right from the terminal
         // See https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda

js/src/debugger/Object.cpp

@@ -1246,6 +1246,9 @@ bool DebuggerObject::CallData::createSource() {
   if (!ToUint32(cx, v, &startColumn)) {
     return false;
   }
+  if (startColumn == 0) {
+    startColumn = 1;
+  }
 
   if (!JS_GetProperty(cx, options, "sourceMapURL", &v)) {
     return false;
@@ -1267,8 +1270,7 @@ bool DebuggerObject::CallData::createSource() {
 
   JS::CompileOptions compileOptions(cx);
   compileOptions.lineno = startLine;
-  compileOptions.column =
-      JS::ColumnNumberOneOrigin::fromZeroOrigin(startColumn);
+  compileOptions.column = JS::ColumnNumberOneOrigin(startColumn);
 
   if (!JS::StringHasLatin1Chars(url)) {
     JS_ReportErrorASCII(cx, "URL must be a narrow string");