nsStringBundle::FormatString seems to be doing an extra string copy for no reason

RESOLVED FIXED in Firefox 56

Status

()

Core
XPCOM
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: bz, Assigned: bz)

Tracking

53 Branch
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

It's got a comment:

  // nsTextFormatter does not use the shared nsMemory allocator.
  // Instead it is required to free the memory it allocates using
  // nsTextFormatter::smprintf_free.  Let's instead use nsMemory based
  // allocation for the result that we give out and free the string
  // returned by smprintf ourselves!
  *aResult = NS_strdup(text);
  nsTextFormatter::smprintf_free(text);

But I'm fairly sure that at this point all the allocators involved are the same exact allocator, so this copy is pointless...

Should probably wait for bug 1380227 to land before messing with this.
Flags: needinfo?(nfroyd)
Bug 1134923 removed NS_Alloc/NS_Realloc/NS_Free. There are leftovers because back then the standalone glue still mattered, but that's mostly gone as of bug 1332523. So we can actually remove those leftovers. Which then makes the whole point of NS_str*dup moot. I'd go as far as saying we should probably get rid of all the NS_str* functions, in favor of methods of the String classes (although those might actually be using the NS_str* functions under the hood at the moment).
Created attachment 8886641 [details] [diff] [review]
Remove unnecessary strdup in stringbundle code

We just have one allocator around now.
Attachment #8886641 - Flags: review?(mh+mozilla)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(nfroyd)
Looks like smprintf_free() can be removed altogether, and its uses replaced with free()? It already has a comment deprecating it. 

And a similar story for JS_smprintf_free() and js_free().

That would just leave PR_smprintf_free(), which is a harder one to remove thanks to it being NSPR.

Updated

5 months ago
Attachment #8886641 - Flags: review?(mh+mozilla) → review+
Could you file a followup bug for comment 1/3?

Comment 5

5 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59532b5342bc
Remove unnecessary strdup in stringbundle code.  r=glandium
I filed bug 1381725, bug 1381726, and bug 1381727.

Thank you all for the review and suggestions!

Comment 7

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/59532b5342bc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.