Fix uses of 'static nsAutoString / nsAutoCString'

ASSIGNED
Assigned to

Status

()

Core
String
P5
normal
ASSIGNED
17 years ago
3 years ago

People

(Reporter: John Bandhauer, Assigned: sgautherie)

Tracking

({footprint, helpwanted})

Trunk
footprint, helpwanted
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [See comment 15], URL)

Attachments

(1 attachment, 8 obsolete attachments)

2.03 KB, patch
Bienvenu
: review-
jcranmer
: feedback-
Details | Diff | Splinter Review
(Reporter)

Description

17 years ago
It looks to me like all of these cases (except the function declarations) should 
either be 'static' or 'nsAutoString' but not 'static nsAutoString'

This wastes memory.

http://lxr.mozilla.org/seamonkey/search?string=static+nsAutoString

It looks like some are in wallet and some are i18n code.
(Reporter)

Updated

17 years ago
Keywords: footprint
(Reporter)

Comment 1

17 years ago
I meant: 'static nsString' or 'nsAutoString'

Comment 2

17 years ago

*** This bug has been marked as a duplicate of 50295 ***
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → DUPLICATE

Comment 3

17 years ago
confirm as dup of 50295
Status: RESOLVED → VERIFIED
(Reporter)

Comment 4

17 years ago
This is not a duplicate of that other bug. I'm a little ticked off that you 
dismissed it so quickly.

- One, this bug was not just about your use in wallet. If you'd followed the lxr 
query or read the bug text you'd have seen that it also talks about i18n making 
the same sorts of mistakes. When you are done fixing this bug please pass it on 
instead of marking it a dup.

- Two, I don't see why you'd want to put the static nsAutoStrings in an object 
(as that other bug suggests). Your code shows that you really are mising the 
whole point of nsAutoString. It is made to be used on the heap to avoid 
alloactions. It has a big honking internal buffer and pretty much every use of 
an nsAutoString as a static, a class member, or alloc'd on the heap is a memory 
bloating bug. 

[\mozilla\extensions\wallet\src] grep "new nsAutoString" *.h *.cpp
htmldlgs.h:  nsAutoString * nsCookie = new nsAutoString("");
wallet.cpp:          valuePtr = new nsAutoString(value);
wallet.cpp:          schemaPtr = new nsAutoString(schema);
wallet.cpp:            valuePtr = new nsAutoString(value);
wallet.cpp:            schemaPtr = new nsAutoString(schema);
wallet.cpp:              valuePtr = new nsAutoString(value);
wallet.cpp:              schemaPtr = new nsAutoString(schema);

Three, Let's look at the wallet items in the list:
http://lxr.mozilla.org/seamonkey/search?string=static+nsAutoString

They are all inside "for (;;)" loops and are filled and used each pass through 
the loop. There is no evident need for the values to persist into the future. 
There is no evident need for them to be static.

What is the justification for using 152 bytes each of static space? This is your 
chance to trim off over a whopping kilobyte of bloat. Why not just do it?

Fixing the "new nsAutoString" seems like a good idea too.
Status: VERIFIED → REOPENED
Resolution: DUPLICATE → ---
(Reporter)

Comment 5

17 years ago
There is some info on nsAutoString usage at...
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsStr.h#75
http://www.mozilla.org/projects/xpcom/nsString.html
...and in the class header (where we see the nice big buffer)...
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsString2.h#580

Updated

17 years ago
Status: REOPENED → ASSIGNED
Target Milestone: --- → M20

Updated

17 years ago
Summary: Fix uses of 'static nsAutoString' → [x]Fix uses of 'static nsAutoString'
Target Milestone: M20 → ---

Comment 6

17 years ago
Have patch.  Same patch fixes three bugs -- this one, bug 50295 and bug 56644.  
Patch is attached to bug 56644.

Updated

17 years ago
Summary: [x]Fix uses of 'static nsAutoString' → [w]Fix uses of 'static nsAutoString'

Updated

17 years ago
Summary: [w]Fix uses of 'static nsAutoString' → Fix uses of 'static nsAutoString'
Whiteboard: [w]

Comment 7

17 years ago
Patch in bug 56644 checked in.  That takes care of this bug as well.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 8

17 years ago
Oops, there's more to this bug than just wallet.  Reopening and reassigning.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 9

17 years ago
Reassigning to ftang for the i18n part.
Assignee: morse → ftang
Status: REOPENED → NEW

Comment 10

17 years ago
Reassigning to Roy.
Assignee: ftang → yokoyama
The patch on bug 61957 changes the two static nsAutoString in intl/chardet/src/
to nsLiteralString.

Isn't it really better to fix these by making them not static?  It's rule number
two in the C++ portability guidelines (I know it prevents us from running on
OpenBSD), and it prevents us from doing DLL unloading on Linux if the statics
are within function scope.

Updated

17 years ago
Status: NEW → ASSIGNED

Comment 12

17 years ago
Created attachment 21217 [details] [diff] [review]
Removing static nsAutoString and defining it as a nsString member data
[Superseded by bug 130393]

Updated

17 years ago
Whiteboard: [w]

Comment 13

17 years ago
Updating the target milestone.
Target Milestone: --- → mozilla0.8

Updated

17 years ago
Target Milestone: mozilla0.8 → mozilla0.9

Updated

16 years ago
Target Milestone: mozilla0.9 → mozilla1.0

Updated

16 years ago
Target Milestone: mozilla1.0 → Future

Comment 14

13 years ago
The lxr query in the url field returns only:
mozilla/widget/src/xpwidgets/nsBaseWidget.h 

210 #ifdef DEBUG
211 protected:
212   static nsAutoString debug_GuiEventToString(nsGUIEvent * aGuiEvent);

Comment 15

13 years ago
re comment 14, that falls under comment 0's (except the function declarations)
caveat. personally i'd rather we didn't return an nsAutoString either in that
case (an out nsAString or something would be more to my liking).

note that a lot of the hits for the following aren't as clear as the hits used
to be for static nsAutoString, but I think some of them are similar:
http://lxr.mozilla.org/seamonkey/search?string=static+nsCAutoString
http://lxr.mozilla.org/seamonkey/search?string=static+nsCString
http://lxr.mozilla.org/seamonkey/search?string=static+nsString

Product: Browser → Seamonkey
(Assignee)

Updated

9 years ago
Assignee: tetsuroy → nobody
Status: ASSIGNED → NEW
Component: General → General
Priority: P3 → --
Product: Mozilla Application Suite → Core
QA Contact: doronr → general
Summary: Fix uses of 'static nsAutoString' → Fix uses of 'static ns*String'
Whiteboard: [See comment 15]
Target Milestone: Future → ---
(Assignee)

Comment 16

9 years ago
Comment on attachment 21217 [details] [diff] [review]
Removing static nsAutoString and defining it as a nsString member data
[Superseded by bug 130393]

Obsoleted by patch in bug 130393.
Attachment #21217 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
Depends on: 130393
Flags: wanted1.9.0.x?
(Assignee)

Updated

9 years ago
Status: ASSIGNED → NEW
Won't take this in 1.9.0.x, but feel free to try for 1.9.1.
Flags: wanted1.9.0.x?
(Assignee)

Comment 18

9 years ago
Created attachment 334221 [details] [diff] [review]
(Bv1) <nsMsgI18N.cpp>

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080816024259 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #334221 - Flags: superreview?(bienvenu)
Attachment #334221 - Flags: review?(bienvenu)
(Assignee)

Updated

9 years ago
Summary: Fix uses of 'static ns*String' → Fix uses of 'new/static nsAutoString/nsCAutoString'
(Assignee)

Comment 19

9 years ago
Created attachment 334225 [details] [diff] [review]
(Cv1) <nsDOMOfflineResourceList.*>
[Superseded by bug 442806 and bug 450174]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080816024259 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)
Attachment #334225 - Flags: superreview?(jst)
Attachment #334225 - Flags: review?(jst)
(Assignee)

Comment 20

9 years ago
Created attachment 334227 [details] [diff] [review]
(Dv1) <nsDocAccessible.cpp>
[Superseded by bug 437607]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080816024259 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)
Attachment #334227 - Flags: review?(aaronleventhal)
(Assignee)

Comment 21

9 years ago
Created attachment 334231 [details] [diff] [review]
(Ev1) <os2/nsFilePicker.cpp>

OS/2: uncompiled.
Attachment #334231 - Flags: review?(mozilla)
Comment on attachment 334221 [details] [diff] [review]
(Bv1) <nsMsgI18N.cpp>

>-        static nsCAutoString acceptLang;
>+        nsCAutoString acceptLang;
>         LossyCopyUTF16toASCII(ucsval, acceptLang);
>         return acceptLang.get();

This will mean the function returns memory to the caller that has already been freed.
(Assignee)

Comment 23

9 years ago
Created attachment 334233 [details] [diff] [review]
(Fv1) <nsWindowsHooksUtil.h>
[Superseded by bug 364168]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080816024259 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)
Attachment #334233 - Flags: superreview?(neil)
Attachment #334233 - Flags: review?(neil)
Comment on attachment 334233 [details] [diff] [review]
(Fv1) <nsWindowsHooksUtil.h>
[Superseded by bug 364168]

Windows hooks is due to die as soon as we finish shell service, so no point patching it.
Attachment #334233 - Flags: superreview?(neil)
Attachment #334233 - Flags: superreview-
Attachment #334233 - Flags: review?(neil)
Attachment #334233 - Flags: review-
(Assignee)

Comment 25

9 years ago
(In reply to comment #22)
> (From update of attachment 334221 [details] [diff] [review])
> This will mean the function returns memory to the caller that has already been
> freed.

Ah, right :-<

Actually, this function is unused:
http://mxr.mozilla.org/comm-central/search?string=nsMsgI18NGetAcceptLanguage&case=on
Should I rather remove it ?

Comment 26

9 years ago
Comment on attachment 334231 [details] [diff] [review]
(Ev1) <os2/nsFilePicker.cpp>

Well, it does compile and I like it that you used this chance to change four-space-indents to two spaces. But you should not add additional blank lines; while you are at it, please change "aCharset" to just "charset" as it's not an argument.
Attachment #334231 - Flags: review?(mozilla) → review-

Comment 27

9 years ago
Comment on attachment 334221 [details] [diff] [review]
(Bv1) <nsMsgI18N.cpp>

as dbaron said, acceptLang will be freed and returned
Attachment #334221 - Flags: superreview?(bienvenu)
Attachment #334221 - Flags: superreview-
Attachment #334221 - Flags: review?(bienvenu)
Attachment #334221 - Flags: review-
(Assignee)

Updated

9 years ago
Attachment #334233 - Attachment description: (Fv1) <nsWindowsHooksUtil.h> → (Fv1) <nsWindowsHooksUtil.h> [See comment 24]
Attachment #334233 - Attachment is obsolete: true
(Assignee)

Comment 28

9 years ago
(In reply to comment #27)
> (From update of attachment 334221 [details] [diff] [review])
> as dbaron said, acceptLang will be freed and returned

Yes: what about my comment 25 question ?

Comment 29

9 years ago
yes, it looks like that function can be removed.
(Assignee)

Comment 30

9 years ago
Created attachment 334284 [details] [diff] [review]
(Ev2) <os2/nsFilePicker.cpp>
[Superseded by bug 969757]
Attachment #334231 - Attachment is obsolete: true
Attachment #334284 - Flags: review?(mozilla)
(Assignee)

Comment 31

9 years ago
Created attachment 334294 [details] [diff] [review]
(Bv2) <nsMsgI18N.*>

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080816024259 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)

Bv1, with comment 29 suggestion(s).
Attachment #334221 - Attachment is obsolete: true
Attachment #334294 - Flags: superreview?(bienvenu)
Attachment #334294 - Flags: review?(bienvenu)

Comment 32

9 years ago
Comment on attachment 334294 [details] [diff] [review]
(Bv2) <nsMsgI18N.*>

thx, Serge. You might check if there are any other methods in nsMsgI18n.h that aren't used, if you haven't already.
Attachment #334294 - Flags: superreview?(bienvenu)
Attachment #334294 - Flags: superreview+
Attachment #334294 - Flags: review?(bienvenu)
Attachment #334294 - Flags: review+

Updated

9 years ago
Attachment #334225 - Flags: superreview?(jst)
Attachment #334225 - Flags: superreview+
Attachment #334225 - Flags: review?(jst)
Attachment #334225 - Flags: review+
(Assignee)

Comment 33

9 years ago
(In reply to comment #4)
> Fixing the "new nsAutoString" seems like a good idea too.

David filled bug 78388 :-)
Flags: in-testsuite-
Keywords: checkin-needed
Summary: Fix uses of 'new/static nsAutoString/nsCAutoString' → Fix uses of 'static nsAutoString / nsCAutoString'
Whiteboard: [See comment 15] → [c-n: Bv2, Cv1 // Leave opened] [See comment 15]
Target Milestone: --- → mozilla1.9.1a2
(Assignee)

Updated

9 years ago
Assignee: sgautherie.bz → nobody
Status: ASSIGNED → NEW
Component: General → String
QA Contact: general → string
(Assignee)

Updated

9 years ago
Assignee: nobody → sgautherie.bz
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Blocks: 307807
(Assignee)

Comment 34

9 years ago
(In reply to comment #32)
> (From update of attachment 334294 [details] [diff] [review])
> thx, Serge. You might check if there are any other methods in nsMsgI18n.h that
> aren't used, if you haven't already.

I hijacked bug 307807 :-|

Comment 35

9 years ago
Comment on attachment 334284 [details] [diff] [review]
(Ev2) <os2/nsFilePicker.cpp>
[Superseded by bug 969757]

Looks good from the OS/2 POV, thanks.
Attachment #334284 - Flags: review?(mozilla) → review+
(Assignee)

Updated

9 years ago
(Assignee)

Updated

9 years ago
Whiteboard: [c-n: Bv2, Cv1 // Leave opened] [See comment 15] → [c-n: Bv2, Cv1, Ev2 // Leave opened] [See comment 15]
Cv1 pushed as 17133:83f4ff468cca.
Ev2 pushed as 17134:f656ee0d93e7.
Whiteboard: [c-n: Bv2, Cv1, Ev2 // Leave opened] [See comment 15] → [c-n: Bv2 // Leave opened] [See comment 15]
(Assignee)

Updated

9 years ago
Attachment #334225 - Attachment description: (Cv1) <nsDOMOfflineResourceList.*> → (Cv1) <nsDOMOfflineResourceList.*> [Checkin: Comment 36]
(Assignee)

Updated

9 years ago
Attachment #334284 - Attachment description: (Ev2) <os2/nsFilePicker.cpp> → (Ev2) <os2/nsFilePicker.cpp> [Checkin: Comment 36]

Updated

9 years ago
Attachment #334227 - Flags: review?(aaronleventhal) → review+
(Assignee)

Updated

9 years ago
Whiteboard: [c-n: Bv2 // Leave opened] [See comment 15] → [c-n: Bv2, Dv1 // Leave opened] [See comment 15]
I've backed out Cv1 and Ev2 due to leaks on the unit testers
Attachment #334284 - Attachment description: (Ev2) <os2/nsFilePicker.cpp> [Checkin: Comment 36] → (Ev2) <os2/nsFilePicker.cpp>
Attachment #334225 - Attachment description: (Cv1) <nsDOMOfflineResourceList.*> [Checkin: Comment 36] → (Cv1) <nsDOMOfflineResourceList.*>
(Assignee)

Updated

9 years ago
Blocks: 450174
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: [c-n: Bv2, Dv1 // Leave opened] [See comment 15] → [c-n: ... // Leave opened] [See comment 15]
(Assignee)

Updated

9 years ago
No longer blocks: 450174
Depends on: 450174
(Assignee)

Comment 38

9 years ago
Created attachment 339616 [details] [diff] [review]
(Bv3) <nsMsgI18N.*>

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080921023908 SeaMonkey/2.0a1pre] (home, debug default) (W2Ksp4)

Bv2 would leak the new |static nsCString|.

This patch rewrites the function with a |static char[]|.
The other (removal) part was done by bug 307807 in the meantime.
I tested that I get "windows-1252".
Attachment #334294 - Attachment is obsolete: true
Attachment #339616 - Flags: superreview?(bienvenu)
Attachment #339616 - Flags: review?(bienvenu)
(Assignee)

Comment 39

9 years ago
Comment on attachment 334225 [details] [diff] [review]
(Cv1) <nsDOMOfflineResourceList.*>
[Superseded by bug 442806 and bug 450174]

Bug 450174 removed |static nsCAutoString gCachedAsciiHost;|.
Bug 442806 removed |nsCAutoString mAsciiHost;| and |nsCAutoString mDynamicOwnerSpec;|.
Attachment #334225 - Attachment description: (Cv1) <nsDOMOfflineResourceList.*> → (Cv1) <nsDOMOfflineResourceList.*> [See comment 39]
Attachment #334225 - Attachment is obsolete: true
(Assignee)

Comment 40

8 years ago
Comment on attachment 339616 [details] [diff] [review]
(Bv3) <nsMsgI18N.*>


bienvenu, ping for r+sr.
(Assignee)

Comment 41

7 years ago
David, ping for review.
(Assignee)

Updated

7 years ago
Attachment #339616 - Flags: superreview?(bienvenu)

Comment 42

7 years ago
I did spend some time looking at this a while ago...I'll try to finish it soon.

Comment 43

7 years ago
Comment on attachment 339616 [details] [diff] [review]
(Bv3) <nsMsgI18N.*>

a much simpler fix would be to replace nsCAutoString with nsCString since the point of the bug was that static nsCAutoString is silly since Auto is used to store the string on the stack. We don't have to worry about dll unloading in our context...
Attachment #339616 - Flags: review?(bienvenu) → review-
(Assignee)

Comment 44

5 years ago
Comment on attachment 339616 [details] [diff] [review]
(Bv3) <nsMsgI18N.*>

(In reply to Serge Gautherie (:sgautherie) from comment #38)
> Bv2 would leak the new |static nsCString|.

(In reply to David :Bienvenu from comment #43)
> a much simpler fix would be to replace nsCAutoString with nsCString

Per comment 38, that doesn't seem a good solution...

Fwiw, see also
334 nsMsgI18NParseMetaCharset(nsILocalFile* file) 
which uses a char[] too.
Attachment #339616 - Flags: feedback?(dbienvenu)
(Assignee)

Updated

5 years ago
Attachment #334225 - Attachment description: (Cv1) <nsDOMOfflineResourceList.*> [See comment 39] → (Cv1) <nsDOMOfflineResourceList.*> [Superseded by bug 450174]
(Assignee)

Updated

5 years ago
Attachment #334225 - Attachment description: (Cv1) <nsDOMOfflineResourceList.*> [Superseded by bug 450174] → (Cv1) <nsDOMOfflineResourceList.*> [Superseded by bug 442806 and bug 450174]
(Assignee)

Updated

5 years ago
Attachment #334284 - Attachment description: (Ev2) <os2/nsFilePicker.cpp> → (Ev2) <os2/nsFilePicker.cpp> [Backed out: Comment 37]
(Assignee)

Updated

5 years ago
Attachment #334233 - Attachment description: (Fv1) <nsWindowsHooksUtil.h> [See comment 24] → (Fv1) <nsWindowsHooksUtil.h> [Superseded by bug 364168]
(Assignee)

Updated

5 years ago
Depends on: 364168, 442806
(Assignee)

Updated

5 years ago
Attachment #21217 - Attachment description: Removing static nsAutoString and defining it as a nsString member data → Removing static nsAutoString and defining it as a nsString member data [Superseded by bug 130393]
(Assignee)

Comment 45

5 years ago
Comment on attachment 339616 [details] [diff] [review]
(Bv3) <nsMsgI18N.*>

Fwiw, maybe can we copy/reuse other code?
http://mxr.mozilla.org/comm-central/search?string=<nsIPlatformCharset>&case=1
(Assignee)

Updated

5 years ago
Keywords: helpwanted
Whiteboard: [c-n: ... // Leave opened] [See comment 15] → [See comment 15 and 37]
Target Milestone: mozilla1.9.1a2 → ---
(Assignee)

Updated

5 years ago
No longer blocks: 307807
Priority: -- → P5
Comment on attachment 339616 [details] [diff] [review]
(Bv3) <nsMsgI18N.*>

Serge, Bienvenu isn't active anymore. Perhaps jcranmer could provide the feedback you seek
Attachment #339616 - Flags: feedback?(mozilla)
Flags: needinfo?(bugzillamozillaorg_serge_20140323)
(Assignee)

Comment 47

3 years ago
Comment on attachment 334284 [details] [diff] [review]
(Ev2) <os2/nsFilePicker.cpp>
[Superseded by bug 969757]

Ftr, bug 969757 removed the whole file.
Attachment #334284 - Attachment description: (Ev2) <os2/nsFilePicker.cpp> [Backed out: Comment 37] → (Ev2) <os2/nsFilePicker.cpp> [Superseded by bug 969757]
Attachment #334284 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 969757
(Assignee)

Comment 48

3 years ago
Comment on attachment 334227 [details] [diff] [review]
(Dv1) <nsDocAccessible.cpp>
[Superseded by bug 437607]

Ftr, bug 437607 removed the whole method.
Attachment #334227 - Attachment description: (Dv1) <nsDocAccessible.cpp> → (Dv1) <nsDocAccessible.cpp> [Superseded by bug 437607]
Attachment #334227 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 437607
(Assignee)

Comment 49

3 years ago
Ftr, there was "Bug 773151 - Rename nsCAutoString to nsAutoCString" in the meantime.

***

http://mxr.mozilla.org/comm-central/search?string=static+nsAutoC*String&regexp=1&case=1
{{
/mailnews/base/util/nsMsgI18N.cpp
    line 176 -- static nsAutoCString fileSystemCharset;

/mozilla/browser/components/about/AboutRedirector.cpp
    line 129 -- static nsAutoCString
/mozilla/js/xpconnect/src/XPCShellImpl.cpp
    line 120 -- static nsAutoString *gWorkingDirectory = nullptr;
/mozilla/widget/gtk/nsFilePicker.cpp
    line 136 -- static nsAutoCString
/mozilla/widget/xpwidgets/nsBaseWidget.h
    line 447 -- static nsAutoString debug_GuiEventToString(mozilla::WidgetGUIEvent* aGuiEvent);
}}
Summary: Fix uses of 'static nsAutoString / nsCAutoString' → Fix uses of 'static nsAutoString / nsAutoCString'
(Assignee)

Comment 50

3 years ago
Comment on attachment 339616 [details] [diff] [review]
(Bv3) <nsMsgI18N.*>

This 6-year old patch needs to be unbitrotted.
Yet, would this approach pass review?
(Have a look at previous comments about this patch B.)
Attachment #339616 - Flags: feedback?(Pidgeot18)
Flags: needinfo?(bugzillamozillaorg_serge_20140323)
(Assignee)

Updated

3 years ago
Whiteboard: [See comment 15 and 37] → [See comment 15]
Comment on attachment 339616 [details] [diff] [review]
(Bv3) <nsMsgI18N.*>

Review of attachment 339616 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/util/nsMsgI18N.cpp
@@ +194,5 @@
>  // Charset used by the file system.
>  const char * nsMsgI18NFileSystemCharset()
>  {
> +  // Default to "ISO-8859-1".
> +  static char fsc[nsIMimeConverter::MAX_CHARSET_NAME_LENGTH + 1] = "ISO-8859-1";

I really think that the default should be UTF-8 instead of ISO-8859-1. My understanding of charsets these days is that practically all systems other than Windows have defaulted to UTF-8.

Then again, reading more carefully, every caller of this method is misusing it properly:
1. This is supposed to the charset of path names, not file contents.
2. All path name manipulation should just be using our UTF-16 APIs instead, which guarantee to work no matter what the code page of the underlying system is.

I thought I found one correct use of this function, but it turns out, on closer inspection, that one was wrong as well (nsMsgAttachmentData::m_realName is in UTF-8, not platform charset). Which means, literally, every use of this function is wrong and it may be more worthwhile just to fix all the callers of this instead of improving a function that relies on a to-be-killed variant anyways.

@@ +195,5 @@
>  const char * nsMsgI18NFileSystemCharset()
>  {
> +  // Default to "ISO-8859-1".
> +  static char fsc[nsIMimeConverter::MAX_CHARSET_NAME_LENGTH + 1] = "ISO-8859-1";
> +  static PRBool needInit = PR_TRUE;

Don't use PRBool unless you really want me to beat you over the head.

@@ +207,5 @@
> +      rv = platformCharset->GetCharset(kPlatformCharsetSel_FileName, fscString);
> +      if (NS_SUCCEEDED(rv)) {
> +        NS_ASSERTION(fscString.Length() < sizeof(fsc), "Too long charset name");
> +        if (fscString.Length() < sizeof(fsc)) {
> +          PL_strcpy(fsc, fscString.get());

Don't use PL_strcpy.
Attachment #339616 - Flags: feedback?(Pidgeot18) → feedback-
(Assignee)

Comment 52

3 years ago
(In reply to Joshua Cranmer [:jcranmer] from comment #51)

> I really think that the default should be UTF-8 instead of ISO-8859-1. My
> understanding of charsets these days is that practically all systems other
> than Windows have defaulted to UTF-8.

For the same reason, I would think so too, but I didn't want to confuse things in this bug/patch.

> Then again, reading more carefully, every caller of this method is misusing
> it properly:
> [...]
> it may be more
> worthwhile just to fix all the callers of this instead of improving a
> function that relies on a to-be-killed variant anyways.

Then I agree!
Could you give an example of such a fix?

> Don't use PRBool unless you really want me to beat you over the head.
+
> Don't use PL_strcpy.

Yeah, thanks, see my unbitrotting reminder ;-)
Flags: needinfo?(Pidgeot18)
(Assignee)

Comment 53

3 years ago
(In reply to Joshua Cranmer [:jcranmer] from comment #51)
> Then again, reading more carefully, every caller of this method is misusing
> it properly:

http://mxr.mozilla.org/comm-central/search?string=nsMsgI18NFileSystemCharset&case=1&find=%2Fmailnews%2F
"Found 27 matching lines in 17 files"
The nsMsgSend usage should just be a UTF8-to-UTF16 conversion.
nsMsgCompUtils.cpp should just die (RFC 2231 should only be using UTF-8). That I'm eventually doing in my patch queue anyways, and it's annoying because that entire block is futzed up and our tests require that we do stupid things.

Everyone else should be using nsMsgI18NTextFileCharset as a "more correct" variant. Although hopefully we don't actually need to support reading filter files from NS 4.x and that's really dead code?
Flags: needinfo?(Pidgeot18)
You need to log in before you can comment on or make changes to this bug.