Closed Bug 1845865 Opened 1 year ago Closed 10 months ago

Wrong item underlined in debugger when Uncaught TypeError

Categories

(DevTools :: Debugger, defect, P3)

Firefox 117
defect

Tracking

(Webcompat Priority:P3, firefox119 fixed)

RESOLVED FIXED
119 Branch
Webcompat Priority P3
Tracking Status
firefox119 --- fixed

People

(Reporter: jeremy.leland, Assigned: arai)

References

(Depends on 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [devtools:relnote])

Attachments

(6 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/117.0

Steps to reproduce:

The following page causes an Uncaught TypeError:

<script>
  const a = null;
  const b = 100 + a.item;
</script>

Actual results:

When I open the debugger on this page, the "100" on line 3 is underlined. This is the wrong item. (see screenshot)

Expected results:

"a" or "a.item" on line 3 should be underlined, as this is the source of the TypeError.

Attached file debug_underline.html

The Bugbug bot thinks this bug should belong to the 'DevTools::Debugger' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → Debugger
Product: Firefox → DevTools

So the error points to line 3, column 13, which is the beginning of the 100 token.
The issue comes from the nsIScriptError.columnNumber

Hi Arai, do you know why we get a wrong column number for this error?

Flags: needinfo?(arai.unmht)

the reason is that we don't have column source note for a property operation alone (a.item), and the nearest source note is for the initializer of the declaration, which is 100 + a.item node, thus it points column 13.
I'll look into adding the source note for property operation and see how much increase happens in the bytecode size.

actually, it would be a problem around binary operator, given the similar issue happens with 100 + x where x doesn't exist.
I'll look into adding source note for rhs of binary operator, and similar for conditional operator.

Tested with adding source note for the following configurations:

(A)

  • non-literal binary operator RHS
  • non-literal in conditional expression

(B)

  • non-literal in binary operator RHS
  • non-literal in conditional expression
  • each non-literal argument in call

The size of the source notes generated during browser startup, in total, and some files with notable increase:

before A A increase B B increase
total 1,447,088 1,483,981 +36,893 (+3%) 1,736,512 +289,424 (+20%)
react-dom.js 33,506 40,093 +6,587 (+20%) 48,771 +15,265 (+46%)
activity-stream.bundle.js 66,292 67,945 +1,653 (+2%) 85,415 +19,123 (+29%)
react-redux.js 4,580 5,377 +797 (+17%) 6,378 +1,798 (+39%)
browser.js 37,346 37,971 +625 (+2%) 43,842 +6,496 (+17%)
SessionStore.sys.mjs 25,451 25,956 +505 (+2%) 30,701 +5,250 (+21%)
CustomizableUI.sys.mjs 22,567 23,117 +550 (+2%) 27,725 +5,158 (+23%)
tabbrowser.js 29,113 29,614 +501 (+2%) 34,223 +5,110 (+18%)

So, the effect of binary operator/conditional expression might be acceptable in some case, but adding notes for each argument has notable impact.
Adding source notes only for binary operator could solve this bug's case, but not sure if it makes sense not to add notes for arguments then.

Of course we can disable the extra source notes for browser internal code, but the same amount of increase will happen in web content.

The other possible options:

  • (1) underline the range from the specified column to the next column source note
  • (2) extract more info in debugger to locate the relevant expression
  • (3) re-compile with detailed source notes on demand when throwing error, to generate better error info
    • this is way more complex, and also this can slow down a code that utilizes try-catch in hot path

anyway, if this is causing web-compat issue, we'd need to just add more source notes, but if this happens only when visualizing the error in devtools, I'd like to go with (1) or (2) way.

Flags: needinfo?(arai.unmht)

how does debugger determine what range to underline in this case?
could it be modified to underline some expression instead of single token?

Flags: needinfo?(jdescottes)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [devtools-triage]

Thanks for the investigation. Let's get feedback from webcompat about this: the error message shows different column numbers in Firefox compared to chrome (13 vs 21 for the example attached here), so that might be a webcompat issue.

Webcompat Priority: --- → ?
Flags: needinfo?(jdescottes)

There is a chance that a site developer might be using the stack trace output for some internal use (like reporting errors to them, or similar things), so there is a interop issue in theory. We don't have any practical breakage on this. Since this can, in theory, break someone's error reporting, let's webcompat-priority-p3 this for now.

Webcompat Priority: ? → P3

Given that this is low priority for webcompat, we should not regress performance to fix this.

Instead it would be great to improve this on the debugger side, and try to underline the whole expression here.

(In reply to Tooru Fujisawa [:arai] from comment #8)

how does debugger determine what range to underline in this case?
could it be modified to underline some expression instead of single token?

:ochameau found the relevant code at https://searchfox.org/mozilla-central/rev/f1f50693655c093d974f026bd37860d939cd5529/devtools/client/debugger/src/components/Editor/Exception.js#38-52

Let us know if you have any suggestion to improve this on our side

Severity: -- → S3
Flags: needinfo?(arai.unmht)
Priority: -- → P3
Whiteboard: [devtools-triage]

I've experimented with using the next breakpoint column number, which works in some case, but doesn't work in some other cases.
I'll continue looking into some other options.

err, "before error's columnNumber" in the figure was wrong. the problematic case is that the breakpoint column numbers appears before the actual error position.

Flags: needinfo?(arai.unmht)
See Also: → 1849144
See Also: → 1643622
Depends on: 1849287

given the column source note for the function call arguments part is completely off and hard to recover from surrounding source notes, it would be better increasing source note at least for some case.
I'll look into adding source notes after compressing the existing source notes, in order to avoid huge increase.

Depends on: 1604121
No longer depends on: 1849287

Tried (B) in comment #7 after compressing the source note, it looks like it's almost okay now.

before after diff
react-dom.js 33544 39457 +18 %
activity-stream.bundle.js 66916 61508 -8 %
react-redux.js 4580 5210 +13 %
browser.js 36808 33365 -9 %
SessionStore.sys.mjs 25453 23979 -6 %
CustomizableUI.sys.mjs 22567 21833 -3 %
tabbrowser.js 28942 25986 -10 %
Duplicate of this bug: 1643622
Attachment #9349248 - Attachment is obsolete: true

Column source note is used for error reporting and debugger.
Literals don't cause any error, and also never be a breakpoint.

When emitting non-static array literal, any element with literal don't need
column source note.

Depends on D186923

Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED

Increases the precision of error column number for the binary operator's RHS
and each argument in the function call.

Depends on D186924

Depends on D186925

Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/2e42e3ee4c71
Part 1: Skip creating column source note for literals in non-static array. r=bthrall
https://hg.mozilla.org/integration/autoland/rev/c05e07dd4f94
Part 2: Add column source notes to binary operator and function call arguments. r=bthrall
https://hg.mozilla.org/integration/autoland/rev/8b79570b07be
Part 3: Add tests. r=devtools-reviewers,ochameau
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → 119 Branch
Regressions: 1853492
Whiteboard: [devtools:relnote]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: