nsCaseConversionImp2 leaking 2 nsCompressedMap objects

VERIFIED FIXED

Status

()

Core
Internationalization
--
minor
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Adam Guthrie, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

({memory-leak, regression})

Trunk
memory-leak, regression
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

This is bug 204114... I'll take a look.
Assignee: nobody → bent.mozilla
Component: General → General
OS: Linux → All
Product: Firefox → Core
Hardware: PC → All
Summary: Increase Rlk on balsa on 2006-06-21 → CaseConversionImp2 leaking 2 nsCompressedMap objects
-> me
Component: General → Internationalization
The deal is that the two nsCompressedMap objects are now global statics... So I guess the leak log doesn't take statics into account? Is it bad to have these as static variables? It was actally alecf's one wish to have these become statics instead of heap-based objects, but I guess we could revert that change. CC'ing dbaron for his thoughts.
Status: NEW → ASSIGNED
Keywords: mlk
Summary: CaseConversionImp2 leaking 2 nsCompressedMap objects → nsCaseConversionImp2 leaking 2 nsCompressedMap objects
Using statics for objects with constructors and destructors runs into both portability problems and ordering ambiguity problems; we generally try to avoid it.  It also may be an obstacle to things like in-process restartability of XPCOM.
(In reply to comment #4)
> Using statics for objects with constructors and destructors runs into both
> portability problems and ordering ambiguity problems; we generally try to avoid
> it.  It also may be an obstacle to things like in-process restartability of
> XPCOM.
> 

I chatted with bz about this and he recommended three changes:

1. removing the Ctor/Dtor logging
2. changing mCache to be an actual array instead of dynamically allocated
3. implementing a private operator new

That would fix the leak, but it wouldn't address the other issues. Should we jst go back to heap-allocated nsCompressedMaps?
Actually, what about keeping them as statics and moving all the code in the ctor/dtor into init/shutdown methods? That way we could still use statics and make sure to free all memory without running into portability problems, right?
QA Contact: general → amyy
Created attachment 226941 [details] [diff] [review]
Patch v1.0

Here's a first stab at fixing this leak.
Attachment #226941 - Flags: review?(dbaron)
Attachment #226941 - Flags: review?(bzbarsky)
Doesn't that still have the "static with constructor" issue?
(In reply to comment #8)
> Doesn't that still have the "static with constructor" issue?
> 

Yes... I guess I was hoping that the real issue is the destructor. I mean, there's no way that any compiler would let us access these objects without running that constructor, right?
Yes, but is the constructor something we'd want to rerun if XPCOM is shut down and reinitailized?
Created attachment 226992 [details] [diff] [review]
Patch v1.1

Ok, this version moves the zeroing of mCache to an Initialize function instead of relying on the constructor.
Attachment #226941 - Attachment is obsolete: true
Attachment #226992 - Flags: review?(bzbarsky)
Attachment #226941 - Flags: review?(dbaron)
Attachment #226941 - Flags: review?(bzbarsky)
Attachment #226992 - Flags: review?(dbaron)
bz, dbaron, does this patch seem correct?
I'm not sure.  Frankly, trying to figure out whether something with ctors and dtors is safe to use statically is just hard.  For example, does mLastBase need to be reset at any point?

The safest fix here is to just stop using static objects with ctors, per comment 4.
Created attachment 230970 [details] [diff] [review]
Patch v1.2

Ah, right. This patch completely removes the constructor.
Attachment #226992 - Attachment is obsolete: true
Attachment #230970 - Flags: review?(bzbarsky)
Attachment #226992 - Flags: review?(dbaron)
Attachment #226992 - Flags: review?(bzbarsky)
Comment on attachment 230970 [details] [diff] [review]
Patch v1.2

>Index: intl/unicharutil/src/nsCaseConversionImp2.cpp
>+   nsCompressedMap() { };
>+   ~nsCompressedMap() { };

Why bother even mentioning those?

>+   // Not meant to be implemented
>+   static void* operator new(size_t /*size*/) CPP_THROW_NEW {
>+      return nsnull;

Then please, don't implement it... Just declare it.

>@@ -219,26 +214,27 @@ static PRUnichar FastToLower(
>-  return NS_OK;
>+  return nsnull;

I'm not sure what the point of this change is; the code is wrong both before and after.

If you're trying to fix a compiler warning or something, why not just remove the "else-after-return" pattern in this code, and leave it as:

  if (...)  { return something; }
  if (...)  { return something else; }
  return fallback;

Looks good other than that....
Attachment #230970 - Flags: review?(bzbarsky) → review+
Created attachment 232191 [details] [diff] [review]
Patch v1.3

Okay, updated to reflect review comments.
Attachment #230970 - Attachment is obsolete: true
Attachment #232191 - Flags: review?(bzbarsky)
Comment on attachment 232191 [details] [diff] [review]
Patch v1.3

>Index: intl/unicharutil/src/nsCaseConversionImp2.cpp
>   else if( IS_NOCASE_CHAR(aChar)) // optimize for block which have no case
>-  {
>-    return aChar;
>-  } 
>+     return aChar;

Please leave the curly braces.

r+sr=bzbarsky with that change.  Sorry about the lag.  :(
Attachment #232191 - Flags: superreview+
Attachment #232191 - Flags: review?(bzbarsky)
Attachment #232191 - Flags: review+
(In reply to comment #17)

I'm in the process of moving and won't be able to check this in for a few days.

Flags: blocking1.9?
Keywords: regression
Created attachment 234952 [details] [diff] [review]
Patch v1.4

This is what I'm going to check in.
Attachment #232191 - Attachment is obsolete: true
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 21

12 years ago
No longer seeing nsCompressedMap objects leaked. ->VERIFIED
Status: RESOLVED → VERIFIED
Flags: blocking1.9? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.