614931 - Number.toLocaleString adds trailing NULL-byte

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jan de Mooij [:jandem]]

---
js> (3.14).toLocaleString()
"3.14\0"
js> (3.14).toLocaleString().length
5
---
I only see this in the shell (OS X, x86/x64, GCC 4.2.1). Tinderbox browser build works OK. Can anybody reproduce?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yes, I can reproduce.

I don't see how this could possibly not happen everywhere, though.  The code added in bug 446494 is just broken:


760     size = digits + (*nint ? strlen(nint + 1) + 1 : 0);
761     if (*nint == '.')
762         size += decimalLength;

It needs to take out the "+1" after the strlen call, as far as I can see, since it's accounting for the decimalLength separately...

Brian, am I just missing something?

Comment 2

[Brian Crowder]

bz: Yeah, looks like you're right.  Removing that + 1 doesn't regress the other case, either.  r=me  :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

The reason this doesn't happen in the browser is that the browser impl of cx->localeCallbacks->localeToUnicode goes through intl code, which I bet treats that null byte as the actual end of string.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Number.toLocaleString shouldn't miscumpute the string length.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 495762 [details] [diff] [review]
Number.toLocaleString shouldn't miscumpute the string length.

size is simply the wrong name -- it's the canonical name (along with nbytes) for a size_ allocation size, not net length in scaled elements. Suggest len, length, or buflen instead. r=me with that.

/be

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, will rename to buflen.

Is this something I should try to get in for 2.0, given that it doesn't affect browser?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Note that this probably does affect some parts of the browser (XPCOM components, maybe JS modules?), given bug 441370.

Comment 8

[Boris Zbarsky [:bzbarsky]]

That's a dup of bug 616841, which is a blocker.  So it really won't be much of a problem.  But ok, I'll try to get this landed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to review comments

Comment 10

[Boris Zbarsky [:bzbarsky]]

Given the comments in bug 616841, we're not likely to fix it in various contexts like workers...  So this should probably block.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/tracemonkey/rev/921fb4d04aca and then pushed http://hg.mozilla.org/tracemonkey/rev/e02379fdaf42 for the test.

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/e02379fdaf42?revcount=500
http://hg.mozilla.org/mozilla-central/rev/921fb4d04aca