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)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: zjz, Assigned: zjz)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 1454207 - Part 1: Changes js::ReportIsNotDefined from a value-returning function to a Non-value-returning function
59 bytes, text/x-review-board-request
arai
: review+
Details
Bug 1454207 - Part 2: Renames |ValueToPrintable| to |ValueToPrintableLatin1|, in preparation for introducing a new function |ValueToPrintableUtf8|
59 bytes, text/x-review-board-request
arai
: review+
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
59 bytes, text/x-review-board-request
arai
: review+
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.
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
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: nobody → zjz
Status: NEW → ASSIGNED
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]
Attachment #8968020 - Attachment is obsolete: true
Attachment #8968020 - Flags: review?(nchevobbe)
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 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 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)
> 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.
Blocks: 1302358
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+
(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 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 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 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+
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
Keywords: checkin-needed
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
To both of you: Thank you for your reviews
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: