52352 - Fix uses of 'static nsAutoString / nsAutoCString'

Status: RESOLVED INACTIVE

(Core :: XPCOM, task, P5)

Description

[John Bandhauer]

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.

Comment 1

[John Bandhauer]

I meant: 'static nsString' or 'nsAutoString'

Comment 2

[Stephen P. Morse]

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

Comment 3

[jorge o. castro]

confirm as dup of 50295

Comment 4

[John Bandhauer]

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.

Comment 5

[John Bandhauer]

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

Comment 6

[Stephen P. Morse]

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

Comment 7

[Stephen P. Morse]

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

Comment 8

[Stephen P. Morse]

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

Comment 9

[Stephen P. Morse]

Reassigning to ftang for the i18n part.

Comment 10

[Erik van der Poel]

Reassigning to Roy.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 12

[Roy Yokoyama]

Attachment: Removing static nsAutoString and defining it as a nsString member data [Superseded by bug 130393]

Comment 13

[Roy Yokoyama]

Updating the target milestone.

Comment 14

[mmc]

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

[timeless]

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 16

[Serge Gautherie (:sgautherie)]

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.

Comment 17

[Samuel Sidler (old account; do not CC)]

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

Comment 18

[Serge Gautherie (:sgautherie)]

Attachment: (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)

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (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)

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (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)

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) <os2/nsFilePicker.cpp>

OS/2: uncompiled.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on 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.

Comment 23

[Serge Gautherie (:sgautherie)]

Attachment: (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)

Comment 24

[[e-mail]]

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.

Comment 25

[Serge Gautherie (:sgautherie)]

(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

[Peter Weilbacher]

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 27

[David :Bienvenu]

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

as dbaron said, acceptLang will be freed and returned

Comment 28

[Serge Gautherie (:sgautherie)]

(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

[David :Bienvenu]

yes, it looks like that function can be removed.

Comment 30

[Serge Gautherie (:sgautherie)]

Attachment: (Ev2) <os2/nsFilePicker.cpp> [Superseded by bug 969757]

Comment 31

[Serge Gautherie (:sgautherie)]

Attachment: (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).

Comment 32

[David :Bienvenu]

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.

Comment 33

[Serge Gautherie (:sgautherie)]

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

David filled bug 78388 :-)

Comment 34

[Serge Gautherie (:sgautherie)]

(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

[Peter Weilbacher]

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

Looks good from the OS/2 POV, thanks.

Comment 36

[Reed Loden [:reed]]

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

Comment 37

[Dave Townsend [:mossop]]

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

Comment 38

[Serge Gautherie (:sgautherie)]

Attachment: (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".

Comment 39

[Serge Gautherie (:sgautherie)]

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 40

[Serge Gautherie (:sgautherie)]

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


bienvenu, ping for r+sr.

Comment 41

[Serge Gautherie (:sgautherie)]

David, ping for review.

Comment 42

[David :Bienvenu]

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

Comment 43

[David :Bienvenu]

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 44

[Serge Gautherie (:sgautherie)]

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 45

[Serge Gautherie (:sgautherie)]

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 46

[Wayne Mery (:wsmwk)]

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

Serge, Bienvenu isn't active anymore. Perhaps jcranmer could provide the feedback you seek

Comment 47

[Serge Gautherie (:sgautherie)]

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

Ftr, bug 969757 removed the whole file.

Comment 48

[Serge Gautherie (:sgautherie)]

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

Ftr, bug 437607 removed the whole method.

Comment 49

[Serge Gautherie (:sgautherie)]

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 50

[Serge Gautherie (:sgautherie)]

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 51

[Joshua Cranmer [:jcranmer]]

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.

Comment 52

[Serge Gautherie (:sgautherie)]

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

Comment 53

[Serge Gautherie (:sgautherie)]

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

Comment 54

[Joshua Cranmer [:jcranmer]]

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?

Comment 55

[Sylvestre Ledru [:Sylvestre]]

Closing as nothing happened for 5 years on this... Probably not a big deal