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.

I meant: 'static nsString' or 'nsAutoString'

*** This bug has been marked as a duplicate of 50295 ***

confirm as dup of 50295

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.

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

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

Patch in bug 56644 checked in. That takes care of this bug as well.

Oops, there's more to this bug than just wallet. Reopening and reassigning.

Reassigning to ftang for the i18n part.

Reassigning to Roy.

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.

Updating the target milestone.

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);

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

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.

Won't take this in 1.9.0.x, but feel free to try for 1.9.1.

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

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

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

OS/2: uncompiled.

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.

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

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.

(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 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.

Comment on attachment 334221 [details] [diff] [review] (Bv1) <nsMsgI18N.cpp> as dbaron said, acceptLang will be freed and returned

(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 ?

yes, it looks like that function can be removed.

[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).

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.

(In reply to comment #4) > Fixing the "new nsAutoString" seems like a good idea too. David filled bug 78388 :-)

(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 on attachment 334284 [details] [diff] [review] (Ev2) <os2/nsFilePicker.cpp> [Superseded by bug 969757] Looks good from the OS/2 POV, thanks.

Cv1 pushed as 17133:83f4ff468cca. Ev2 pushed as 17134:f656ee0d93e7.

I've backed out Cv1 and Ev2 due to leaks on the unit testers

[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".

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;|.

Comment on attachment 339616 [details] [diff] [review] (Bv3) <nsMsgI18N.*> bienvenu, ping for r+sr.

David, ping for review.

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

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...

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.

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

Comment on attachment 339616 [details] [diff] [review] (Bv3) <nsMsgI18N.*> Serge, Bienvenu isn't active anymore. Perhaps jcranmer could provide the feedback you seek

Comment on attachment 334284 [details] [diff] [review] (Ev2) <os2/nsFilePicker.cpp> [Superseded by bug 969757] Ftr, bug 969757 removed the whole file.

Comment on attachment 334227 [details] [diff] [review] (Dv1) <nsDocAccessible.cpp> [Superseded by bug 437607] Ftr, bug 437607 removed the whole method.

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); }}

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.)

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.

(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 ;-)

(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?