Closed Bug 52352 Opened 24 years ago Closed 6 years ago

Fix uses of 'static nsAutoString / nsAutoCString'

Categories

(Core :: XPCOM, task, P5)

task

Tracking

()

RESOLVED INACTIVE

People

(Reporter: jband_mozilla, Assigned: sgautherie)

References

()

Details

(Keywords: helpwanted, memory-footprint, Whiteboard: [See comment 15])

Attachments

(1 file, 8 obsolete files)

(deleted), patch
Bienvenu
: review-
jcranmer
: feedback-
Details | Diff | Splinter Review
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.
Keywords: footprint
I meant: 'static nsString' or 'nsAutoString'
*** This bug has been marked as a duplicate of 50295 ***
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → DUPLICATE
confirm as dup of 50295
Status: RESOLVED → VERIFIED
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 → ---
Status: REOPENED → ASSIGNED
Target Milestone: --- → M20
Summary: Fix uses of 'static nsAutoString' → [x]Fix uses of 'static nsAutoString'
Target Milestone: M20 → ---
Have patch. Same patch fixes three bugs -- this one, bug 50295 and bug 56644. Patch is attached to bug 56644.
Summary: [x]Fix uses of 'static nsAutoString' → [w]Fix uses of 'static nsAutoString'
Summary: [w]Fix uses of 'static nsAutoString' → Fix uses of 'static nsAutoString'
Whiteboard: [w]
Patch in bug 56644 checked in. That takes care of this bug as well.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → FIXED
Oops, there's more to this bug than just wallet. Reopening and reassigning.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reassigning to ftang for the i18n part.
Assignee: morse → ftang
Status: REOPENED → NEW
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.
Status: NEW → ASSIGNED
Whiteboard: [w]
Updating the target milestone.
Target Milestone: --- → mozilla0.8
Target Milestone: mozilla0.8 → mozilla0.9
Target Milestone: mozilla0.9 → mozilla1.0
Target Milestone: mozilla1.0 → Future
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
Product: Browser → Seamonkey
Assignee: tetsuroy → nobody
Status: ASSIGNED → NEW
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 → ---
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
Status: NEW → ASSIGNED
Depends on: 130393
Flags: wanted1.9.0.x?
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?
Attached patch (Bv1) <nsMsgI18N.cpp> (obsolete) (deleted) — Splinter Review
[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)
Summary: Fix uses of 'static ns*String' → Fix uses of 'new/static nsAutoString/nsCAutoString'
[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)
[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)
Attached patch (Ev1) <os2/nsFilePicker.cpp> (obsolete) (deleted) — Splinter Review
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.
[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-
(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.
Attachment #334231 - Flags: review?(mozilla) → review-
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-
Attachment #334233 - Attachment description: (Fv1) <nsWindowsHooksUtil.h> → (Fv1) <nsWindowsHooksUtil.h> [See comment 24]
Attachment #334233 - Attachment is obsolete: true
(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.
Attachment #334231 - Attachment is obsolete: true
Attachment #334284 - Flags: review?(mozilla)
Attached patch (Bv2) <nsMsgI18N.*> (obsolete) (deleted) — Splinter Review
[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 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+
Attachment #334225 - Flags: superreview?(jst)
Attachment #334225 - Flags: superreview+
Attachment #334225 - Flags: review?(jst)
Attachment #334225 - Flags: review+
(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: sgautherie.bz → nobody
Status: ASSIGNED → NEW
Component: General → String
QA Contact: general → string
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Blocks: 307807
(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.
Attachment #334284 - Flags: review?(mozilla) → review+
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]
Attachment #334225 - Attachment description: (Cv1) <nsDOMOfflineResourceList.*> → (Cv1) <nsDOMOfflineResourceList.*> [Checkin: Comment 36]
Attachment #334284 - Attachment description: (Ev2) <os2/nsFilePicker.cpp> → (Ev2) <os2/nsFilePicker.cpp> [Checkin: Comment 36]
Attachment #334227 - Flags: review?(aaronleventhal) → review+
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.*>
Blocks: 450174
Keywords: checkin-needed
Whiteboard: [c-n: Bv2, Dv1 // Leave opened] [See comment 15] → [c-n: ... // Leave opened] [See comment 15]
No longer blocks: 450174
Depends on: 450174
Attached patch (Bv3) <nsMsgI18N.*> (deleted) — Splinter Review
[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)
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
Comment on attachment 339616 [details] [diff] [review] (Bv3) <nsMsgI18N.*> bienvenu, ping for r+sr.
David, ping for review.
Attachment #339616 - Flags: superreview?(bienvenu)
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...
Attachment #339616 - Flags: review?(bienvenu) → review-
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)
Attachment #334225 - Attachment description: (Cv1) <nsDOMOfflineResourceList.*> [See comment 39] → (Cv1) <nsDOMOfflineResourceList.*> [Superseded by bug 450174]
Attachment #334225 - Attachment description: (Cv1) <nsDOMOfflineResourceList.*> [Superseded by bug 450174] → (Cv1) <nsDOMOfflineResourceList.*> [Superseded by bug 442806 and bug 450174]
Attachment #334284 - Attachment description: (Ev2) <os2/nsFilePicker.cpp> → (Ev2) <os2/nsFilePicker.cpp> [Backed out: Comment 37]
Attachment #334233 - Attachment description: (Fv1) <nsWindowsHooksUtil.h> [See comment 24] → (Fv1) <nsWindowsHooksUtil.h> [Superseded by bug 364168]
Depends on: 364168, 442806
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]
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
Keywords: helpwanted
Whiteboard: [c-n: ... // Leave opened] [See comment 15] → [See comment 15 and 37]
Target Milestone: mozilla1.9.1a2 → ---
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)
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
Depends on: 969757
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
Depends on: 437607
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'
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)
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-
(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)
(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)

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

Status: ASSIGNED → RESOLVED
Type: defect → task
Closed: 24 years ago6 years ago
Resolution: --- → INACTIVE
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: