323376 - nsTextFormatter::ssprintf adds extra '\0' to result

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Steve Krulewitz]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

nsTextFormatter::ssprintf is appending an extra null to the end of the string it assigns to the passed in nsAString.  The following code:

nsAutoString value;
nsTextFormatter::ssprintf(value, NS_LITERAL_STRING("%d").get(), 1);
printf("%d\n", value.Length());

Will print 2 for the length of the string rather than 1.  This is because dosprintf() explicitly appends a null to the end of the output string, even when working with an nsAString.  The patch I am including simply changes the StringStuff function to do nothing when passed a null string.


Reproducible: Always

Steps to Reproduce:

Comment 1

[Steve Krulewitz]

Attachment: Test case

Comment 2

[Steve Krulewitz]

Attachment: The patch

Comment 3

[Eli Friedman]

Hmm, looks like this patch got abandoned.  (I just happened upon this bug).

To the reporter of this bug:
The way the system works, the person who writes a patch is supposed to ask for review (see http://www.mozilla.org/hacking/code-review-faq.html).  (Click edit next to the patch, set review to ? and put in the email address of the person you're asking for review).  To get an idea of who to ask, check http://www.mozilla.org/owners.html.  In this case, I suppose I'd suggest [e-mail]; although that's just a guess; they'll redirect you if you ask the wrong person.

Comment 4

[Philip Withnall (unavailable)]

Comment on attachment 208454 [details] [diff] [review]
The patch

Review requested.

Comment 5

[Darin Fisher]

Comment on attachment 208454 [details] [diff] [review]
The patch

I recommend returning early if !*sp.  That way you avoid indenting the rest of the function which helps make the code more readable IMO.

r=darin with that change

Comment 6

[Doug Turner (:dougt)]

mass reassigning to nobody.

Comment 8

[Jonathan Protzenko [:protz]]

Attachment: Un-bitrotted patch + comment taken into account

Carrying Darin Fisher's r+.

Comment 9

[Jonathan Protzenko [:protz]]

Comment on attachment 480082 [details] [diff] [review]
Un-bitrotted patch + comment taken into account

I just understood that I needed approval for this patch to be accepted in the 2.0 timeframe, so here's the request. The patch is relatively low-risk, it's been reviewed already, and it really is a two-liner.

The mailnews crowd would be definitely enlightened to see this checked in, as we currently have workarounds for this, that call .Cut() onto the nsStrings returned by the function.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 480082 [details] [diff] [review]
Un-bitrotted patch + comment taken into account

a=beltzner

Comment 11

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/mozilla-central/rev/5164a685fa81

Comment 12

[Jonathan Protzenko [:protz]]

Comment on attachment 480082 [details] [diff] [review]
Un-bitrotted patch + comment taken into account

Requesting approval for the 1.9.2 branch as Thunderbird 3.2 is likely going to be based on 1.9.2 and we'll need that fix in as well for the patches to work properly.

Comment 13

[LegNeato]

Will the removal of the workarounds have to be coordinated with the landing of this?

Comment 14

[Jonathan Protzenko [:protz]]

We checked-in a patch without the workarounds for comm-central, so as long as this patch is checked-in on the 1.9.2 branch Thunderbird 3.2 will be based on, I think it's fine. But Standard8 probably knows better.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 480082 [details] [diff] [review]
Un-bitrotted patch + comment taken into account

Approved for 1.9.2.13, a=dveditz for release-drivers

Comment 16

[Jonathan Protzenko [:protz]]

Thanks for the approval, marking as checkin-needed.

Comment 17

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dc359ac5af9a