25875 - Leaking CAttributeTokens

Status: VERIFIED INVALID

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

Description

[Patrick C. Beard]

Tokens aren't always being recycled properly, some of them leak. I haven't 
figured out how precisely, but they always seem to be CAttributeTokens if that's 
any help. They are root leaks, meaning that whatever object was referencing them, 
has gone away.

Comment 1

[Patrick C. Beard]

Attachment: Example leak report

Comment 2

[Patrick C. Beard]

Looking further at the code, I see the only way CAttributeTokens will get 
recycled, is if nsCParserNode's destructor has a recycler to use. And yet, there 
are many places in CNavDTD.cpp where the nsCParserNode::Init() is getting called 
with a 0 token recycler. Here're some patches that both pass a recycler, and give 
nsCParserNode's destructor a fallback of eagerly deleting attribute nodes. With 
these patches, my build doesn't leak CAttributeTokens.

Comment 3

[Patrick C. Beard]

Attachment: patches to CNavDTD.cpp & nsXIFDTD.cpp for token recycling

Comment 4

[Patrick C. Beard]

Not sure if this is necessary, but if you don't pass a token recycler, here's a 
way to free attribute tokens:

Index: mozilla/htmlparser/src/nsParserNode.cpp
===================================================================
RCS file: /cvsroot/mozilla/htmlparser/src/nsParserNode.cpp,v
retrieving revision 3.36
diff -c -2 -r3.36 nsParserNode.cpp
*** nsParserNode.cpp	1999/12/07 00:22:14	3.36
--- nsParserNode.cpp	2000/01/31 16:17:04
***************
*** 86,89 ****
--- 86,94 ----
      if(mRecycler) {
        RecycleTokens(mRecycler,*mAttributes);
+     } else {
+       CToken* theToken;
+       while((theToken=(CToken*)mAttributes->Pop()) != nsnull){
+         delete theToken;
+       }
      }
      delete mAttributes;

Comment 5

[harishd]

working on parser leaks. Moving to my list.

Comment 6

[Patrick C. Beard]

Here's another reason for CAttributeToken leaks. In nsXIFDTD::PopAndDelete(), you 
are deleting a variable of type nsIParserNode*, which prevents the proper 
nsCParserNode destructor from getting called. The fix is either to cast, or to 
use proper reference counting. You decide. Here's the full patch to nsXIFDTD.cpp 
that seems to fix remaining CAttributeToken leaks:

Index: mozilla/htmlparser/src/nsXIFDTD.cpp
===================================================================
RCS file: /cvsroot/mozilla/htmlparser/src/nsXIFDTD.cpp,v
retrieving revision 1.58
diff -r1.58 nsXIFDTD.cpp
592c592
<   nsCParserNode   node = ((CHTMLToken*)aToken);
---
>   nsCParserNode   node((CHTMLToken*)aToken, 1, mTokenizer->GetTokenRecycler());
993c993
<   nsCParserNode*  node = new nsCParserNode(token);
---
>   nsCParserNode*  node = new nsCParserNode(token, 1, mTokenizer->GetTokenRecycler());
1047c1047
<     delete node;
---
>     delete (nsCParserNode*)node;	// beard:  perhaps a virtual destructor, or 
actual reference counting...

Comment 7

[harishd]

Thanx for the patches Patrick. 

Comment 8

[Bruce Mitchener]

Any chance that these patches could get reviewed and go in today?

Comment 9

[harishd]

The patch provided ( for CNavDTD.cpp) does not actually cover the entire 
problem, infact I'll be surprised if it actually fixes the leak ( I have to 
verify this). I'm looking for the real solution to address this problem ( also 
taking the patch into consideration).  

The XIF dtd is going to change completely..so I'll not be touching it anytime 
soon.
 

Comment 10

[rickg]

I've already fixed the leak in XIFDTD -- and I'll land this one over the 
weekend.

Comment 11

[harishd]

Patrick, could you give me the URL/testcase that exposed this leak?

Comment 12

[harishd]

Patrick, could you please point me to the URL that could reproduce this leak.
Thanx.

Comment 13

[harishd]

Not a beta blocker...moving M17.

Comment 14

[harishd]

I'm not sure how to reproduce this leak..without a URL or testcase.  Patrick, 
could you please provide me with sufficient information?  
Thanx.

Comment 15

[rickg]

Harish, here's something I forget to mention: I added a global tokenrecycler to 
the bodycontext so that the StrictDTD wouldn't have to create it's own. Perhaps 
you can have the NavDTD do the same? Risk is low, reuse is high.

Comment 16

[harishd]

I'm going to mark this bug invalid. Let's reopen when there's a solid test case.

Comment 17

[Jan Carpenter]

verified

Comment 18

[Patrick C. Beard]

I'll see if I can reproduce this leak again. I don't quite agree that this is 
invalid, more that you don't know how to reproduce it. If you look at the stack 
crawl in the leak report, you'll see that an XML_Parse is involved. Have you 
examined XML parsing for leaks?