342425 - nsCaseConversionImp2 leaking 2 nsCompressedMap objects

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Adam Guthrie]

2528	MOZ_CO_DATE=2006:06:21:08:10:00
2560	MOZ_CO_DATE=2006:06:21:09:40:00

Checkins:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006%3A06%3A21%3A08%3A10%3A00&maxdate=2006%3A06%3A21%3A09%3A40%3A00&cvsroot=%2Fcvsroot

Build log shows that we're leaking nsCompressedMap (objects?).

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This is bug 204114... I'll take a look.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

-> me

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 4

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

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.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v1.0

Here's a first stab at fixing this leak.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Doesn't that still have the "static with constructor" issue?

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yes, but is the constructor something we'd want to rerun if XPCOM is shut down and reinitailized?

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v1.1

Ok, this version moves the zeroing of mCache to an Initialize function instead of relying on the constructor.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

bz, dbaron, does this patch seem correct?

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v1.2

Ah, right. This patch completely removes the constructor.

Comment 15

[Boris Zbarsky [: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....

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v1.3

Okay, updated to reflect review comments.

Comment 17

[Boris Zbarsky [: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.  :(

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v1.4

This is what I'm going to check in.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed on trunk.

Comment 21

[Adam Guthrie]

No longer seeing nsCompressedMap objects leaked. ->VERIFIED