Patch to fix shutdown leak noise in nsStyleSet

VERIFIED FIXED

Status

()

Core
Layout
P3
normal
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: Inaky Perez Gonzalez, Assigned: Inaky Perez Gonzalez)

Tracking

({memory-leak})

Trunk
x86
Linux
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tind-mlk])

Attachments

(4 attachments)

(Assignee)

Description

18 years ago
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.
(Assignee)

Updated

18 years ago
Keywords: mlk, patch
(Assignee)

Comment 1

18 years ago
Created attachment 10591 [details] [diff] [review]
Fix for leakage noise reduction on class nsStyleSet

Comment 2

18 years ago
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

18 years ago
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

18 years ago
I kind of like the xpcom shutdown method better. Less chance for refcounts to 
get screwed up.
(Assignee)

Comment 5

18 years ago
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

18 years ago
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.
(Assignee)

Comment 7

18 years ago
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

18 years ago
*** Bug 43579 has been marked as a duplicate of this bug. ***
Do we even need to keep this as static?  See bug 43579.
(Assignee)

Comment 10

18 years ago
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

18 years ago
TRTTD? Que?
(Assignee)

Comment 12

18 years ago
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

18 years ago
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.
(Assignee)

Comment 14

18 years ago
[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.

(Assignee)

Comment 15

18 years ago
Created attachment 10603 [details] [diff] [review]
Done with reference counts

Comment 16

18 years ago
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.
(Assignee)

Comment 17

18 years ago
Ok, done. I had the check on gQuirkURI not being NULL in my previous patch, I
forgot to re-generate it.
(Assignee)

Comment 18

18 years ago
Created attachment 10604 [details] [diff] [review]
Changed the static var name and check for it to be sane
Whiteboard: [tind-mlk]

Comment 19

18 years ago
did this die?  should it get reviewed for checkin again?
Created attachment 18519 [details] [diff] [review]
removed unneeded threadsafe decrement/increment: just used ++ and --

Comment 21

18 years ago
Thanks! The patch looks good. r=attinasi@netscape.com 
sr=brendan@mozilla.org on that last patch.

/be
Fix checked in 2000-11-07 19:13 PDT.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 24

17 years ago
Marking verified per last comments.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.