Closed Bug 323376 Opened 16 years ago Closed 11 years ago

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

Categories

(Core :: XPCOM, defect)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
status1.9.2 --- .13-fixed
status1.9.1 --- wontfix

People

(Reporter: skrulx, Assigned: protz)

References

Details

(Keywords: testcase)

Attachments

(2 files, 1 obsolete file)

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:
Attached file Test case
Attached patch The patch (obsolete) — Splinter Review
Keywords: testcase
Status: UNCONFIRMED → NEW
Ever confirmed: true
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 darin@meer.net; although that's just a guess; they'll redirect you if you ask the wrong person.
Comment on attachment 208454 [details] [diff] [review]
The patch

Review requested.
Attachment #208454 - Flags: review?(darin)
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
Attachment #208454 - Flags: review?(darin) → review+
mass reassigning to nobody.
Assignee: dougt → nobody
Duplicate of this bug: 601096
Carrying Darin Fisher's r+.
Attachment #208454 - Attachment is obsolete: true
Attachment #480082 - Flags: review+
Keywords: checkin-needed
Blocks: 559559
Assignee: nobody → jonathan.protzenko
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.
Attachment #480082 - Flags: approval2.0?
Comment on attachment 480082 [details] [diff] [review]
Un-bitrotted patch + comment taken into account

a=beltzner
Attachment #480082 - Flags: approval2.0? → approval2.0+
Checked in: http://hg.mozilla.org/mozilla-central/rev/5164a685fa81
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
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.
Attachment #480082 - Flags: approval1.9.2.13?
Will the removal of the workarounds have to be coordinated with the landing of this?
Target Milestone: mozilla2.0b8 → mozilla2.0b7
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 on attachment 480082 [details] [diff] [review]
Un-bitrotted patch + comment taken into account

Approved for 1.9.2.13, a=dveditz for release-drivers
Attachment #480082 - Flags: approval1.9.2.13? → approval1.9.2.13+
Thanks for the approval, marking as checkin-needed.
Keywords: checkin-needed
No longer blocks: 559559
Blocks: 559559
You need to log in before you can comment on or make changes to this bug.