1997216 - DomException column number is always zero

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Zijie Zhao]

Attachment: column_zero.zip

Steps to reproduce:

Steps to reproduce:

• Unzip the attached column_zero.zip file.
• Open the test_column_zero.html with the latest Firefox

Dear Firefox Developers,

Our static analysis tool reported a potentially unimplemented functionality in DOMException::ColumnNumber()

uint32_t Exception::ColumnNumber() const { return 0; }

Currently, accessing the columnNumber property from a caught DOM exception in JavaScript always returns zero, even though the correct information is available in mLocation. Interestingly, the stack property does include the correct column number.

We were wondering if this behavior is intentional. If not, it appears that fixing it would be straightforward, as shown in the attached column_fix.patch.

Please let us know if we missed any key information or feedback! Thank you!

Actual results:

• The exception.columnNumber is always 0, contradicting the column number shown by exception.stack
• As shown in before_screenshot.png

Expected results:

• The exception.columnNumber should show the correct column where the exception is thrown
• As shown in after_screenshot.png

Comment 1

[Tooru Fujisawa (away) [:arai]]

Thank you for your investigation and the patch!

Indeed it's strange that we're always returning 0.

The return 0 itself dates back to the older implementation, and at that point there was no column number in JSStackFrame struct.
The column number was added in bug 1069490, as https://hg-edge.mozilla.org/mozilla-central/rev/b0dd65a5d40b6b7031c8a773b03f410159ef3ecd ,
but the DOM exception columnNumber getter implementation wasn't touched at that point, but at the same time I don't see any comment mentions it, so I assume it's not that there was some reason to skip it, but just that DOM exception wasn't the main target of the fix.

Also, given that columnNumber is not a part of the spec https://webidl.spec.whatwg.org/#idl-DOMException , returning non-zero value there would be okay (unless there's some website that relies on the value being zero, which is less likely).

Can you follow the https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html to submit the patch for review?
So that the commit is properly associated with your name/email, and the patch is handled in the review/landing processes.
Also it would be nice if there's an automated testcase that verifies the columnNumber property returns an expected value.

Let me know if there's anything unclear in the patch submission.

Comment 2

[Zijie Zhao]

Attachment: Bug 1997216 - Fix Exception::ColumnNumber to return actual column number. r=arai

Comment 3

[Zijie Zhao]

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

Also, given that columnNumber is not a part of the spec https://webidl.spec.whatwg.org/#idl-DOMException , returning non-zero value there would be okay (unless there's some website that relies on the value being zero, which is less likely).

Thanks for checking the spec and confirming the behavior!

Can you follow the https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html to submit the patch for review?
So that the commit is properly associated with your name/email, and the patch is handled in the review/landing processes.
Also it would be nice if there's an automated testcase that verifies the columnNumber property returns an expected value.

I submitted the patch with a mochitest that checks for the line number and column number.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2b92c4f86d82
https://hg.mozilla.org/integration/autoland/rev/da948edf8b2d
Fix Exception::ColumnNumber to return actual column number. r=arai,emilio

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/76065bbb6c97
https://hg.mozilla.org/integration/autoland/rev/28c03f52a1c4
Revert "Bug 1997216 - Fix Exception::ColumnNumber to return actual column number. r=arai,emilio" for causing mochitests-plain failures in test_exception_line_column.html.

Comment 6

[Serban Stanca [:SerbanS]]

Reverted this because it was causing mochitests-plain failures in test_exception_line_column.html.

• Revert link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | dom/bindings/test/test_exception_line_column.html | appendChild exception should be on line 35 - got 34, expected 35

Comment 7

[Zijie Zhao]

(In reply to Serban Stanca [:SerbanS] from comment #6)

Reverted this because it was causing mochitests-plain failures in test_exception_line_column.html.

• Revert link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | dom/bindings/test/test_exception_line_column.html | appendChild exception should be on line 35 - got 34, expected 35

Oh, I removed a line with an unreachable assertion and forgot to update the line number in this later assertion. It should be an easy fix by just changing the expected value. Sorry for missing this change in the beginning.

Should I push the change to the same Phabricator revision?

Comment 8

[Tooru Fujisawa (away) [:arai]]

Yes, please push to the same revision.

Comment 9

[Zijie Zhao]

Thanks for the help! I submitted the fix.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/459880d45518
https://hg.mozilla.org/integration/autoland/rev/d76f2c8613a9
Fix Exception::ColumnNumber to return actual column number. r=arai,emilio

Comment 11

[amarc]

https://hg.mozilla.org/mozilla-central/rev/d76f2c8613a9