1845865 - Wrong item underlined in debugger when Uncaught TypeError

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[jeremy.leland]

Attachment: ff_debug_wrong_underline.png

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.

Comment 1

[jeremy.leland]

Attachment: debug_underline.html

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 4

[Julian Descottes [:jdescottes]]

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

Comment 5

[Tooru Fujisawa [:arai]]

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.

Comment 6

[Tooru Fujisawa [:arai]]

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.

Comment 7

[Tooru Fujisawa [:arai]]

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.

Comment 8

[Tooru Fujisawa [:arai]]

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

Comment 9

[Julian Descottes [:jdescottes]]

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.

Comment 10

[Dennis Schubert [:denschub]]

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.

Comment 11

[Julian Descottes [:jdescottes]]

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

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: show underline until the next breakpoint

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.

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: WIP: Bug 1845865 - Prototype: Use the next break point offset as the end of underline for the error.

Comment 14

[Tooru Fujisawa [:arai]]

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.

Comment 15

[Tooru Fujisawa [:arai]]

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.

Comment 16

[Tooru Fujisawa [:arai]]

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 %

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Bug 1845865 - Part 1: Skip creating column source note for literals in non-static array. r?bthrall!

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

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Bug 1845865 - Part 2: Add column source notes to binary operator and function call arguments. r?bthrall!

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

Depends on D186924

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Bug 1845865 - Part 3: Add tests. r?jdescottes!

Depends on D186925

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2e42e3ee4c71
https://hg.mozilla.org/mozilla-central/rev/c05e07dd4f94
https://hg.mozilla.org/mozilla-central/rev/8b79570b07be