63627 - LEAKS in style context fast cache

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Marc Attinasi]

Enabling the fast cache has caused new leaks. This needs to be fixed.

Comment 1

[Marc Attinasi]

Attachment: leak log from checkin

Comment 2

[Marc Attinasi]

Leaking nsVoidArrays - probably the cache's destructor needs to do more to clean 
up than just calling Reset(), like enumerating each entry and removing the list?

I'll fix this after the holiday, unless it is deemed critical, in which case, 
please note it in this bug and email [e-mail]

Comment 3

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

It would probably be fixed by either doing enumeration as you suggest (and
deleting in one other place, I think), or by converting the nsHashtable in
StyleContextCache to an nsObjectHashtable.   (BTW, why is StyleContextCache in
nsIStyleSet.h?)

Comment 4

[Marc Attinasi]

Attachment: Patch to fix leak (and move StyleContextCache to cpp file)

Comment 5

[Marc Attinasi]

I have attached a patch that changes the StyleContextCache to use an 
nsObjectHashTable, as suggested by David. Added the requisite callback to 
destroy the allocated nsVoidArrays in the hash table, and cleaned up the 
allocation / destruction encapsulation a bit. Also moved the StyleContextCache 
declaration out of the interface header file, David's suggestion again.

Can somebody please review this when you get a chance? Thanks.

Comment 6

[Marc Attinasi]

Attachment: Second part of change

Comment 7

[Marc Attinasi]

My apologies: the patch is missing the friend declaration required to compile: 
in the StyleContextCache class decl, insert

  friend PRBool PR_CALLBACK HashTableEnumDestroy(nsHashKey *aKey, void *aData, 
void* closure);

so the DestroyList static fcn. can be called in the callback. Sorry I messed up 
the patch.

Comment 8

[Marc Attinasi]

Attachment: Corrected Patch

Comment 9

[buster]

sr=buster

Comment 10

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

What's the advantage of separate |DestroyList| and |AllocateList| when you can
just use |new nsVoidArray()| and |delete|?  They don't seem to offer any
additional abstraction since they take/return nsVoidArray* anyway.  It seems to
just introduce more code (and the, admittedly minor, performance/bloat cost with
it).

Comment 11

[Marc Attinasi]

The reason for the DestroyList / AllocateList methods is to hide the way that
the lists are allocated and destroyed from the clients of the cache. It is
minor, but when I implemented the destroy callback I really didn't like that it
had to know to use 'delete' because internally it was allocated using 'new' -
hence the trivial static methods to encapsulate the creation and destruction
implementation. Without those abstraction methods there is shared knowledge
between the nsStyleContextCache and PR_CALLBACK HashTableEnumDestroy function,
namely that new and delete are used.

I'm not convinced that it is critical to have those methods, they are more
stylistic I guess, but I also think that the extra fcn call to allocate or
delete the list is trivial compared to the actual heap access. Also, if we
change to using a pool or arena or another heap, then the change is encapsulated
in the class.

Comment 12

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

OK, r=dbaron

Comment 13

[Marc Attinasi]

Fix is in, leaks way down on Tinderbox. Happy New Year!

Comment 14

[Hixie (not reading bugmail)]

Netscape's standard compliance QA team reorganised itself once again, so taking 
remaining non-tables style bugs. Sorry about the spam. I tried to get this done 
directly at the database level, but apparently that is "not easy because of the 
shadow db", "plus it screws up the audit trail", so no can do...