43835 - Leakage noise fix for class nsHTMLTags

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P3)

Description

[Inaky Perez Gonzalez]

Class nsHTMLTags allocates in one of it´s static methods [GetStringValue()] an
static nsString which is never released, inducing leakage noise.

	The normal procedure woul dhave been to add a class-wide instances count, but
this class is only used statically, so the constructor and destructors ain´t
used.

	The sollution I propose is using atexit() to register a call which will free
that dynamically allocated nsString before program exit. It won´t show up
correcly under bloatview, but will do in the allocation log.

	FInd a patch attached.

Comment 1

[Inaky Perez Gonzalez]

Attachment: Fix using atexit(). Pls read comments in bug log

Comment 2

[Inaky Perez Gonzalez]

I don´t feel really confortable using atexit(). I´ve been trying to check if we
could safely use AddRefTable() and ReleaseTable(), but didn´t reach a
conclusion. Would need some help on that one.

Comment 3

[carl.wong]

Since nsStr is not an XPCOM objects, why can't we solve this simple problem 
with a simple fix by defining it as a static object(i.e. static nsCString 
kNullStr("")) and let the compiler generate the destructor code?????

Comment 4

[Chris Waterson]

This is why:

http://www.mozilla.org/hacking/portable-cpp.html#dont_use_static_constructors

Comment 5

[Inaky Perez Gonzalez]

[Speaking for myself, not for my employer, Intel Corporation, though my
intention would be to benefit everybody with this comments]

I know I may be the 1142423th person who mentions this, but haven´t you guys
just considered to drop support for compilers which aren´t implementing such
basic and old features of the C++ standards as static ctors (and many more I´ve
seen on the portability page)?

My point is the bloat this introduces into the code. I understand some fancy
features such as RTTI and perhaps exceptions can be trickier, though I consider
the almost three years gone since the standard was accepted to be more than
enough.

Being able to fully use the C++ standard would simplify mozilla code such a lot
that just thinking about it drives me nuts (simply using exceptions for error
handling could cut source size by 1/4).

Ok, we are in bug fixing phase for stable release, but I think this is something
that should be considered for the next one. 

Comment 6

[karnaze (gone)]

Harish, I'm reassigning rickg parser bugs to you.

Comment 7

[harishd]

Nominating for beta3.

Comment 8

[Nisheeth Ranjan]

Marking nsbeta3+...

Comment 9

[harishd]

Attachment: Proposed patch [ Same as Inaky's but without using atexit() ]

Comment 10

[harishd]

I think this bug must have got fixed by John's recent checkin to nsHTMLTags. 
Adding John to the CC list for input.  

Comment 11

[John Bandhauer]

In my changed the code in question became:

  } else {
    static nsCString kNullStr;
    return kNullStr;
  }

I think this just fixes the problem being debated. As I understand things, this 
static local is created when the block is entered and not at module startup 
time. The language guarantees that it will de destucted on shutdown.

Tell me why this does not fix the problem or why it would really be worthwhile 
to add aditional code to handle what is supposed to be automatic.

Comment 12

[harishd]

I agree with John. The good thing is that we are not heap allocating  anymore ( 
though it's not a big deal here ).  His changes would definitely fix the 
problem.  Marking bug FIXED ( fixed by jband ).

Comment 13

[Jan Carpenter]

This seems to be a code fix,
developer please verify

Comment 14

[bsharma]

updated qa contact.

Comment 15

[bsharma]

Marking verified as per above developer comments.