43656 - Patch to fix shutdown leak noise in nsStyleSet

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Inaky Perez Gonzalez]

Class nsStyleSet references a nsStr in static variable
nStyleSet::GetStringValueXXX::theStr. Upon shutdown it causes some
noise, as it is not released.  Following the directions I got in
#38283, I implemented a fix which will shut it down on XPCOM shutdown,
by turning nsStyleSet into an nsIObserver.

        As this is only useful for noise reduction during debugging (and 
I don´t thinkit makes world better to have it in production releases), 
the fix is only activated if DEBUG is defined. In case you want this
changed, let me know.

        Please see the attached patch for the fix.

Comment 1

[Inaky Perez Gonzalez]

Attachment: Fix for leakage noise reduction on class nsStyleSet

Comment 2

[Chris Waterson]

inaki: I think the use of XPCOM shutdown is overkill in this case. If this is a 
global object that's allocated once for efficiency's sake, I'd recommend that 
you add a global static counter as a member in nsStyleSet.

- Increment it in each ctor: if the count is one, you'll need to (re-)create the 
object.

- Decrement it in each dtor. When it goes to zero, release the object

This is a more common (and much simpler) pattern than using an XPCOM shutdown 
listener. For an example, see

http://lxr.mozilla.org/mozilla/source/rdf/content/src/nsXULElement.cpp#328
http://lxr.mozilla.org/mozilla/source/rdf/content/src/nsXULElement.cpp#395

thanks,
chris

Comment 3

[Marc Attinasi]

Wait a minute: this looks like a dup of bug 43579, and I have attached a patch 
to that bug to fix this problem. The easiest approach is to just make the URI 
instance a data member on the StyleSet and manage it as a normal data member, 
giving up the static-nature of it but still avoiding a new/delete for each call 
of to EnableQuirkStyleSheet.

Chris' apprach is good too, better in the sense that it does not bind the 
lifetime of the URL instance to each style set, and I'd be all for that 
implementation too.

I think this bug should be Dup'd to 43579, or 43579 should be dup'd to this one.

Comment 4

[Warren Harris]

I kind of like the xpcom shutdown method better. Less chance for refcounts to 
get screwed up.

Comment 5

[Inaky Perez Gonzalez]

Completely agree, Chris ... I am changing all my patches for this kind of
problems to use global in-class counters.

Marc, I prefer Chris´s fix. We still have static, not reallocated each time and
should work almost the same.

Just a damn pity we cannot have those static COMPtrs ...

Comment 6

[Warren Harris]

What's wrong with the callback-based approach? I think it's a lot less error 
prone. With the global counters, you end up with a domino effect if something 
doesn't get freed.

Comment 7

[Inaky Perez Gonzalez]

My english is again hitting my head. WHat do you mean with ´callback based
approach´? If it is the XPCOM sollution, I do too, but I agree with Chris that
it is too overkill for these kind of problems. Take into account that we are
only taking noise away, to help us debugging, but we are in no way enhancing
runtime, we are only affecting shutdown.

Going to re-create this using a instance counter. Please tell me if you guys
agree.

Comment 8

[Marc Attinasi]

*** Bug 43579 has been marked as a duplicate of this bug. ***

Comment 9

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

Do we even need to keep this as static?  See bug 43579.

Comment 10

[Inaky Perez Gonzalez]

Well, if we don´t do, we´ll have around 8 nsStyleSets (as you comment), we are
spending more time in creating and managing them then managing through an static
object. 

And that should be TRTTD (tm) ... :)

Comment 11

[Marc Attinasi]

TRTTD? Que?

Comment 12

[Inaky Perez Gonzalez]

Mande lo cualo?

TRTTD = The Right Thing To Do ... you know, when you want to sound pedantic but
not to make it too evident ... :)

Comment 13

[Warren Harris]

By "callback" I meant using NS_IMPL_NSGETMODULE_WITH_DTOR, supplying a 
destructor to clean up the objects in question.

BTW, I noticed that you've got your stuff ifdef DEBUG -- any reason why we 
shouldn't do it for release builds too just to keep it simple.

Comment 14

[Inaky Perez Gonzalez]

[callback]

huh ... looks like I didn´t go to class the day they explained that. Could you
ellaborate some more on it.

[ifdef DEBUG]
As this is really useful only for debug builds (taking out noise), I just
thought it could be worth comenting it out on release ones, but in the new
patches I´m not doing it; as you say, we make it simpler.

Comment 15

[Inaky Perez Gonzalez]

Attachment: Done with reference counts

Comment 16

[Marc Attinasi]

The patch looks fine. I would name the URI instance gQuirkURI instead of gURI, 
and maybe check for gQuirkURI != nsnull before calling 
cssSheet->ContainsStyleSheet(gUrl ...) because asserting that memory is 
available is dubious at best.  But other than that it looks nice. 

Thanks, Inaky.

Comment 17

[Inaky Perez Gonzalez]

Ok, done. I had the check on gQuirkURI not being NULL in my previous patch, I
forgot to re-generate it.

Comment 18

[Inaky Perez Gonzalez]

Attachment: Changed the static var name and check for it to be sane

Comment 19

[Bruce Mitchener]

did this die?  should it get reviewed for checkin again?

Comment 20

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

Attachment: removed unneeded threadsafe decrement/increment: just used ++ and --

Comment 21

[Marc Attinasi]

Thanks! The patch looks good. r=[e-mail] 

Comment 22

[Brendan Eich [:brendan]]

sr=[e-mail] on that last patch.

/be

Comment 23

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

Fix checked in 2000-11-07 19:13 PDT.

Comment 24

[Chris Petersen]

Marking verified per last comments.