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

RESOLVED FIXED in Firefox 64

Status

()

enhancement
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(4 attachments, 9 obsolete attachments)

3.55 KB, patch
arai
: review+
Details | Diff | Splinter Review
21.52 KB, patch
anba
: review+
Details | Diff | Splinter Review
32.66 KB, patch
anba
: review+
Details | Diff | Splinter Review
21.05 KB, patch
anba
: review+
Details | Diff | Splinter Review
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
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.
Attachment #9008337 - Flags: review?(arai.unmht)
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.
Attachment #9008339 - Flags: review?(arai.unmht)
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.
Attachment #9008340 - Flags: review?(arai.unmht)
Change GetFunctionNameBytes to use UTF-8 instead of Latin-1 encoding.
Attachment #9008341 - Flags: review?(arai.unmht)
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"
Attachment #9008337 - Flags: review?(arai.unmht) → review+
Attachment #9008339 - Flags: review?(arai.unmht) → review+
Attachment #9008340 - Flags: review?(arai.unmht) → review+
Attachment #9008341 - Flags: review?(arai.unmht) → review+
Updated per review feedback, carrying r+.
Attachment #9008337 - Attachment is obsolete: true
Attachment #9008698 - Flags: review+
Rebased after changing part 1, carrying r+.
Attachment #9008339 - Attachment is obsolete: true
Attachment #9008699 - Flags: review+
Pushed by apavel@mozilla.com:
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
Keywords: checkin-needed
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.
Attachment #9008698 - Attachment is obsolete: true
Flags: needinfo?(andrebargull)
Attachment #9008779 - Flags: review+
Rebased after changing part 1.
Attachment #9008699 - Attachment is obsolete: true
Attachment #9008780 - Flags: review+
Add missing rooting to fix rooting hazards.
Attachment #9008340 - Attachment is obsolete: true
Attachment #9008781 - Flags: review+
FWIW, the ctypes patches may conflict with bug 1490630 (still on autoland).
Rebased after braces changes.
Attachment #9008779 - Attachment is obsolete: true
Attachment #9009176 - Flags: review+
Rebased after braces changes.
Attachment #9009176 - Attachment is obsolete: true
Attachment #9009177 - Flags: review+
Rebased after braces changes.
Attachment #9008780 - Attachment is obsolete: true
Attachment #9009178 - Flags: review+
Rebased after braces changes.
Attachment #9008781 - Attachment is obsolete: true
Attachment #9009179 - Flags: review+
Pushed by btara@mozilla.com:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.