1864786 - Audit nsIScriptError.columnNumber consumers in DevTools

Status: RESOLVED WORKSFORME

(DevTools :: General, task)

Description

[Tooru Fujisawa [:arai]]

nsIScriptError has been using a mix of 0-based and 1-based column number.

bug 1864168 and bug 1864783 are going to switch some consumer of 0-based column number to 1-based.

If there's any code in DevTools that listens to nsIScriptErrors and expects the columnNumber being 0-based, it should be rewritten to expect 1-based column number,
or maybe expect that "the column number base is unknown".

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Call graph of nsIScriptError::init* methods and the data flow, the column base

As a first step, here's the data flow into the nsIScriptError.columnNumber field.

Some consumers had been using 1-based column number,
other consumers had been using 0-based column number, and some of them are switched to 1-based column number by the patch stack for bug 1862693, bug 1862814, bug 1864155, and bug 1864168.

Comment 2

[Tooru Fujisawa [:arai]]

• Regular JavaScript is logged with 1-based column number in the console
• nsIScriptError error/warnings generated from Gecko C++ code is logged with 0-based column number, which is inconsistent with other regular JavaScript error reported to the same file
• CSS error/warning is logged with 1-based column number in the console
• during the conversion from nsIScriptError to resource or message or action, there's no place that converts the base
• The consumer of the nsIScriptError.columnNumber expects the 1-based and converts between 0-based (below 2 code)
  • I don't see anywhere else that expects the base

https://searchfox.org/mozilla-central/rev/766a338d53cd29e9851107f9e6a62d333c7d2c51/devtools/client/debugger/src/actions/preview.js#130,142

export function getExceptionPreview(target, tokenPos, codeMirror) {
...
    const tokenColumnStart = match.location.start.column + 1;

https://searchfox.org/mozilla-central/rev/766a338d53cd29e9851107f9e6a62d333c7d2c51/devtools/client/debugger/src/components/Editor/Exception.js#54,67

addEditorExceptionLine() {
...
    column: columnNumber - 1,

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Consumer of nsIScriptError

Comment 4

[Tooru Fujisawa [:arai]]

Given the above, I think the code that's using 0-based column number for nsIScriptError.columnNumber just need to be fixed to 1-based (like bug 1862814 patches), and there's no change necessary to devtools' side.

Comment 5

[Tooru Fujisawa [:arai]]

same for Console API mentioned in https://phabricator.services.mozilla.com/D193370
most consumers have been using 1-based, and only service worker-related warning have been using 0-based,
and switching to 1-based should be fine.