1490605 - Change ValueToSourceForError and GetFunctionNameBytes to use UTF-8 instead of Latin-1 encoding

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

Directly using UTF-8 encoded strings for error reporting is preferable over Latin-1, because it avoids the additional Latin-1 to UTF-8 transformation in [1].

[1] https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/js/src/vm/JSContext.cpp#644-657

Comment 1

[André Bargull [:anba]]

Attachment: bug1490605-part1-ctypes-error-value-stringifying.patch

Preparatory patch to reduce code duplication when ctypes code stringifies error arguments. 

- Adds |TypeSourceForError| for the call sequence |BuildTypeSource + EncodeLatin1|.
- Adds |FunctionTypeSourceForError| for the call sequence |BuildFunctionTypeSource + EncodeLatin1|.
- Adds |ConversionPositionForError| for the call sequence |BuildConversionPosition + EncodeLatin1|.
- Adds |IndexCString| for uint32_t/size_t to char* conversion instead of duplicating |char[] + SprintfLiteral| in a couple of places. Also changes the char buffer size to 21 to fit for size_t values.

Comment 2

[André Bargull [:anba]]

Attachment: bug1490605-part2-ValueToSourceForError.patch

Change ValueToSourceForError to use UTF-8 which more or less requires that ctypes uses UTF-8 for all its error reporting, too.

Also added |JS::StringIsASCII| assertions in ctypes error reporting functions when |char*| are passed, to ensure the |char*| can be used in UTF-8 error reporting.

Comment 3

[André Bargull [:anba]]

Attachment: bug1490605-part3-ctypes-utf8-encoding-functions.patch

ctypes near duplicates the UTF-8 encoding functions from CharacterEncoding.h. The only additional feature ctypes uses, is it disallows unpaired surrogates. Because I don't think this justifies duplicating the code, we should just move the unpaired surrogate detection into a separate function and then directly call into CharacterEncoding.h for UTF-8 encoding.

Drive-by changes:
- Replaced the remaining callers to ASSERT_OK with MOZ_ALWAYS_TRUE.
- Updated ctypes/Library.cpp to use JS::UniqueChars and avoid AutoStableStringChars when the result is unused.

Comment 4

[André Bargull [:anba]]

Attachment: bug1490605-part4-GetFunctionNameBytes.patch

Change GetFunctionNameBytes to use UTF-8 instead of Latin-1 encoding.

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 9008337 [details] [diff] [review]
bug1490605-part1-ctypes-error-value-stringifying.patch

Review of attachment 9008337 [details] [diff] [review]:
-----------------------------------------------------------------

Great! thank you :D

::: js/src/ctypes/CTypes.cpp
@@ +1207,5 @@
> +}
> +
> +class IndexCString final
> +{
> +    char indexStr[21];

might be nice to add a comment about the reason of "21"

Comment 6

[André Bargull [:anba]]

Attachment: bug1490605-part1-ctypes-error-value-stringifying.patch

Updated per review feedback, carrying r+.

Comment 7

[André Bargull [:anba]]

Attachment: bug1490605-part2-ValueToSourceForError.patch

Rebased after changing part 1, carrying r+.

Comment 8

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6acdb21c176442a01615d367050d873bff11e69

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e54bd8035b43
Part 1: Reduce code duplication when stringifying error arguments in ctypes code. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/e911dd693280
Part 2: Return UTF-8 encoded strings from ValueToSourceForError. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/61250a398db0
Part 3: Use existing UTF-8 functions from CharacterEncoding instead of near duplicating them. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/29c2fa7d40f1
Part 4: Return UTF-8 encoded strings from GetFunctionNameBytes. r=arai

Comment 10

[Andrei Ciure[:aciure]]

Backed out 4 changesets (bug 1490605) for build bustages CTypes.cpp

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=199068703&revision=29c2fa7d40f1067718eab4a94b0147e3d864b4a3

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/0c6b0c2d4c82ce208124e76e25a2e3b86d469f44

Comment 11

[André Bargull [:anba]]

Attachment: bug1490605-part1-ctypes-error-value-stringifying.patch

Change IndexCString to only have a single constructor accepting |size_t|, because -- surprise, surprise -- having two constructors where one accepts |uint32_t| and the other |size_t| doesn't work on 32bit.

Comment 12

[André Bargull [:anba]]

Attachment: bug1490605-part2-ValueToSourceForError.patch

Rebased after changing part 1.

Comment 13

[André Bargull [:anba]]

Attachment: bug1490605-part3-ctypes-utf8-encoding-functions.patch

Add missing rooting to fix rooting hazards.

Comment 14

[Jan de Mooij [:jandem]]

FWIW, the ctypes patches may conflict with bug 1490630 (still on autoland).

Comment 15

[André Bargull [:anba]]

Attachment: bug1490605-part1-ctypes-error-value-stringifying.patch

Rebased after braces changes.

Comment 16

[André Bargull [:anba]]

Attachment: bug1490605-part1-ctypes-error-value-stringifying.patch

Rebased after braces changes.

Comment 17

[André Bargull [:anba]]

Attachment: bug1490605-part2-ValueToSourceForError.patch

Rebased after braces changes.

Comment 18

[André Bargull [:anba]]

Attachment: bug1490605-part3-ctypes-utf8-encoding-functions.patch

Rebased after braces changes.

Comment 19

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69d967d129028190703497f289e308a5beabb8b0

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28bebbb1d4cb
Part 4: Return UTF-8 encoded strings from GetFunctionNameBytes. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/096c2c7d4326
Part 1: Reduce code duplication when stringifying error arguments in ctypes code. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a993fa3e3d8
Part 2: Return UTF-8 encoded strings from ValueToSourceForError. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fed430fe00c
Part 3: Use existing UTF-8 functions from CharacterEncoding instead of near duplicating them. r=arai

Comment 21

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/28bebbb1d4cb
https://hg.mozilla.org/mozilla-central/rev/096c2c7d4326
https://hg.mozilla.org/mozilla-central/rev/6a993fa3e3d8
https://hg.mozilla.org/mozilla-central/rev/0fed430fe00c