Closed
Bug 1454207
Opened 6 years ago
Closed 6 years ago
js::ReportIsNotDefined should report readable Unicode characters instead of unreadable Unicode escape sequences
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: zjz, Assigned: zjz)
References
Details
Attachments
(4 files, 1 obsolete file)
Create an HTML file with the following content: <!DOCTYPE html> <meta charset="utf-8"> <title>Use an undefined Unicode variable</title> <script>測</script> then open the file in Firefox, and then open the console, you'll see an error report with an unreadable Unicode escape sequence: ReferenceError: \u6E2C is not defined The report should have shown a readable Unicode character instead of the escape sequence: ReferenceError: 測 is not defined
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → zjz
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8968020 [details] Bug 1454207 - Part 4: Adds a mochitest in the Web Console to check if a referenceError is reported correctly with an undefined Unicode identifier. https://reviewboard.mozilla.org/r/236700/#review242460 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/webconsole/test/mochitest/browser_console_error_with_unicode.js:8 (Diff revision 1) > + > +// Check if an error with Unicode characters is reported correctly. > + > +"use strict"; > + > +const TEST_URI = "data:text/html;charset=utf8,<script>\u6e2c</script>"; Error: 'test_uri' is assigned a value but never used. [eslint: no-unused-vars]
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8968020 -
Attachment is obsolete: true
Attachment #8968020 -
Flags: review?(nchevobbe)
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8968017 [details] Bug 1454207 - Part 1: Changes js::ReportIsNotDefined from a value-returning function to a Non-value-returning function https://reviewboard.mozilla.org/r/236694/#review242496 Thank you for fixing this :) Please address the following comments. ::: js/src/vm/JSContext.cpp:887 (Diff revision 1) > - if (ValueToPrintable(cx, IdToValue(id), &printable)) { > - JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr, JSMSG_NOT_DEFINED, > + if (!ValueToPrintable(cx, IdToValue(id), &printable)) { > + return; > - printable.ptr()); > } Please remove braces, for single-line then-block.
Attachment #8968017 -
Flags: review?(arai.unmht)
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8968018 [details] Bug 1454207 - Part 2: Renames |ValueToPrintable| to |ValueToPrintableLatin1|, in preparation for introducing a new function |ValueToPrintableUtf8| https://reviewboard.mozilla.org/r/236696/#review242498 ::: js/src/vm/StringType.h:1598 (Diff revision 1) > + * characters. If there are any non-ASCII characters in the original value, then those characters > + * will be changed to Unicode escape sequences(I.e. \udddd, dddd are 4 hex digits) in the printable > + * string. > */ > extern const char* > -ValueToPrintable(JSContext* cx, const Value&, JSAutoByteString* bytes, bool asSource = false); > +ValueToPrintableAscii(JSContext* cx, const Value&, JSAutoByteString* bytes, bool asSource = false); This method converts a value to Latin1 string, not ASCII string (see bug 1302358) (characters between 0x80 and 0xff are not converted to \uXXXX form) So, please rename this to "ValueToPrintableLatin1".
Attachment #8968018 -
Flags: review?(arai.unmht)
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8968019 [details] Bug 1454207 - Part 3: Introduces a new function |ValueToPrintableUtf8| so that we report ReferenceError with readable Unicode characters instead of unreadable Unicode escape sequences https://reviewboard.mozilla.org/r/236698/#review242500 ::: js/src/vm/StringType.h:1604 (Diff revision 1) > > /* > + * Convert a value to a printable C string encoded in UTF-8. > + */ > +extern const char* > +ValueToPrintableUtf8(JSContext* cx, const Value&, JSAutoByteString* bytes, bool asSource = false); Please use all capital "UTF8" for the function name. (there are some "Utf8" methods, but I think it's better using "UTF8" for new functions, and maybe rename existing functions eventually, (of course not in this bug))
Attachment #8968019 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 10•6 years ago
|
||
> Comment on attachment 8968018 [details] > This method converts a value to Latin1 string, not ASCII string (see bug > 1302358) Thanks for pointing that out. Funny, I originally renamed it to |ValueToPrintableLatin1|, but then I read the |QuoteString| https://searchfox.org/mozilla-central/source/js/src/jit/CacheIRSpewer.cpp#109 and saw the code 127, so I thought ValueToPrintable is only for ASCII. Since you pointed that out, so I just went back to double-check the |QuoteString|, and found I had overlooked something the last time: there is one more condition isprint(c) with c >= 127. Yes, it's for all Latin1 codes.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8968021 [details] Bug 1454207 - Part 4: Adds a mochitest in the Web Console to check if a referenceError is reported correctly with an undefined Unicode identifier. https://reviewboard.mozilla.org/r/236702/#review242556 Is the desired behavior for Browser console only ? If not, we usually test things in the webconsole (so the test here should be renamed to browser_webconsole_error_with_unicode.js , and use the webconsole) ::: devtools/client/webconsole/test/mochitest/browser_console_error_with_unicode.js:9 (Diff revision 2) > +// Check if an error with Unicode characters is reported correctly. > + > +"use strict"; > + > +const TEST_URI = "data:text/html;charset=utf8,<script>\u6e2c</script>"; > +const EXPECTED_REPORT = "ReferenceError: \u6e2c is not defined"; Is there an issue if we have "ReferenceError: 測 is not defined" instead ?
Attachment #8968021 -
Flags: review?(nchevobbe) → review+
Assignee | ||
Comment 16•6 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #15) > Comment on attachment 8968021 [details] > Bug 1454207 - Part 4: Adds a mochitest in the Web Console to check if a > referenceError is reported correctly with an undefined Unicode identifier. > > https://reviewboard.mozilla.org/r/236702/#review242556 > > Is the desired behavior for Browser console only ? > If not, we usually test things in the webconsole (so the test here should be > renamed to browser_webconsole_error_with_unicode.js , and use the webconsole) > Okay, will rename it. > ::: > devtools/client/webconsole/test/mochitest/browser_console_error_with_unicode. > js:9 > (Diff revision 2) > > +// Check if an error with Unicode characters is reported correctly. > > + > > +"use strict"; > > + > > +const TEST_URI = "data:text/html;charset=utf8,<script>\u6e2c</script>"; > > +const EXPECTED_REPORT = "ReferenceError: \u6e2c is not defined"; > > Is there an issue if we have "ReferenceError: 測 is not defined" instead ? Because different developers may have different default encoding for their code editors, someone whose editor's default encoding isn't UTF-8(Though it's very unlikely) may encounter messy code, so I think it's safer to write an escape sequence here.
Comment hidden (mozreview-request) |
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8968017 [details] Bug 1454207 - Part 1: Changes js::ReportIsNotDefined from a value-returning function to a Non-value-returning function https://reviewboard.mozilla.org/r/236694/#review242880 Great! Thank you!
Attachment #8968017 -
Flags: review?(arai.unmht) → review+
Comment 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8968018 [details] Bug 1454207 - Part 2: Renames |ValueToPrintable| to |ValueToPrintableLatin1|, in preparation for introducing a new function |ValueToPrintableUtf8| https://reviewboard.mozilla.org/r/236696/#review242882
Attachment #8968018 -
Flags: review?(arai.unmht) → review+
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8968019 [details] Bug 1454207 - Part 3: Introduces a new function |ValueToPrintableUtf8| so that we report ReferenceError with readable Unicode characters instead of unreadable Unicode escape sequences https://reviewboard.mozilla.org/r/236698/#review242884
Attachment #8968019 -
Flags: review?(arai.unmht) → review+
Assignee | ||
Comment 21•6 years ago
|
||
Here is the PUSH try for the previous commit, since the last commit doesn't involve any functionality change but some function name changes and the coding style, so I think the try result for the previous commit is enough: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e54f41fe1c8ba3656024e48ca23f61b07a646028
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 22•6 years ago
|
||
Pushed by cbrindusan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/16eb62f2681a Part 1: Changes js::ReportIsNotDefined from a value-returning function to a Non-value-returning function r=arai https://hg.mozilla.org/integration/autoland/rev/4a202b773692 Part 2: Renames |ValueToPrintable| to |ValueToPrintableLatin1|, in preparation for introducing a new function |ValueToPrintableUtf8| r=arai https://hg.mozilla.org/integration/autoland/rev/9e8c23aa94e4 Part 3: Introduces a new function |ValueToPrintableUtf8| so that we report ReferenceError with readable Unicode characters instead of unreadable Unicode escape sequences r=arai https://hg.mozilla.org/integration/autoland/rev/8b0ba3f7d099 Part 4: Adds a mochitest in the Web Console to check if a referenceError is reported correctly with an undefined Unicode identifier. r=nchevobbe
Keywords: checkin-needed
Assignee | ||
Comment 23•6 years ago
|
||
To both of you: Thank you for your reviews
Comment 24•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/16eb62f2681a https://hg.mozilla.org/mozilla-central/rev/4a202b773692 https://hg.mozilla.org/mozilla-central/rev/9e8c23aa94e4 https://hg.mozilla.org/mozilla-central/rev/8b0ba3f7d099
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
QA Whiteboard: [good first verify]
You need to log in
before you can comment on or make changes to this bug.
Description
•