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

RESOLVED FIXED in mozilla2.0b7

Status

()

RESOLVED FIXED
13 years ago
7 years ago

People

(Reporter: skrulx, Assigned: protz)

Tracking

({testcase})

Trunk
mozilla2.0b7
Other
Linux
testcase
Points:
---

Firefox Tracking Flags

(status1.9.2 .13-fixed, status1.9.1 wontfix)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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:
(Reporter)

Comment 1

13 years ago
Created attachment 208453 [details]
Test case
(Reporter)

Comment 2

13 years ago
Created attachment 208454 [details] [diff] [review]
The patch

Updated

13 years ago
Keywords: testcase
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

12 years ago
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 5

12 years ago
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+

Comment 6

11 years ago
mass reassigning to nobody.
Assignee: dougt → nobody
(Assignee)

Updated

8 years ago
Duplicate of this bug: 601096
(Assignee)

Comment 8

8 years ago
Created attachment 480082 [details] [diff] [review]
Un-bitrotted patch + comment taken into account

Carrying Darin Fisher's r+.
Attachment #208454 - Attachment is obsolete: true
Attachment #480082 - Flags: review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Updated

8 years ago
Blocks: 559559

Updated

8 years ago
Assignee: nobody → jonathan.protzenko
(Assignee)

Comment 9

8 years ago
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
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
(Assignee)

Comment 12

8 years ago
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?

Comment 13

8 years ago
Will the removal of the workarounds have to be coordinated with the landing of this?
Target Milestone: mozilla2.0b8 → mozilla2.0b7
(Assignee)

Comment 14

8 years ago
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+
(Assignee)

Comment 16

8 years ago
Thanks for the approval, marking as checkin-needed.
Keywords: checkin-needed
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dc359ac5af9a
status1.9.2: --- → .13-fixed
Keywords: checkin-needed

Updated

8 years ago
status1.9.1: --- → wontfix

Updated

7 years ago
No longer blocks: 559559

Updated

7 years ago
Blocks: 559559
You need to log in before you can comment on or make changes to this bug.