Closed
Bug 402633
Opened 17 years ago
Closed 17 years ago
[FIX]View source leaks many CTokens and nsStringBuffers
Categories
(Core :: DOM: HTML Parser, defect, P3)
Core
DOM: HTML Parser
Tracking
()
RESOLVED
FIXED
mozilla1.9beta2
People
(Reporter: jruderman, Assigned: bzbarsky)
Details
(Keywords: memory-leak)
Attachments
(1 file)
12.68 KB,
patch
|
mrbkap
:
review+
mrbkap
:
superreview+
|
Details | Diff | Splinter Review |
Doing View Source on any page leaks nsStringBuffer objects. The larger the page, the more string buffers leak. For example, View Source on slashdot.org leaks 5000 of them.
Split from bug 395845.
Flags: blocking1.9?
Comment 1•17 years ago
|
||
The nsStringBuffers aren't the only things leaking; they're leaked via leaked CTokens, specifically, I believe, the end-element </foo> tokens. These tokens don't show up as leaks because they don't use the macros to tracked in leak reports. I have a patch in bug 402612 to reenable those macros, and theoretically that plus XPCOM_MEM_ALLOC_LOG and XPCOM_MEM_LOG_CLASSES=CToken should be enough to figure out what's up (for some reason it wasn't quite working for me when I tried it, oddly).
Reporter | ||
Updated•17 years ago
|
Summary: View source leaks many nsStringBuffers → View source leaks many CTokens and nsStringBuffers
Assignee | ||
Comment 2•17 years ago
|
||
What a mess. So CTokens are refcounted. I figured I'd switch on refcount logging for them and see what happens. That's the first part of the patch. With this patch, there are no more random calls to Destroy() except from the Release() method.
This ran immediately into the issue that the CToken constructor sets the refcount to 1 and we stack-allocate them in some places. That's why I had to add the hack in ~Ctoken.
The rest of it is much simpler. The CTokenDeallocator is used to clean up the tokenizer's deque, and it really shouldn't be removing more than the single ref that got added before the token went into the deque. nsParserNode was addrefing tokens even though it had no plans to release them (because they're stack tokens). Removed some members of nsViewSourceHTML that haven't been used in years (ever since we don't need a parser node to close a tag).
And finally the leak itself. Since tokens start life with a refcount of 1, CreateTokenOfType() effectively returns already_AddRefed<>. Which means that any caller that creates tokens that way has to either pass off ownership to someone else or IF_FREE the token. Since parser nodes don't take ownership (except when adding attributes to them), we needed some IF_FREEs in the view-source code.
The only part of this that could be a bit risky is the CTokenDeallocator change. And at worst it introduces leaks in normal parsing... I haven't run into any so far, though.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #287505 -
Flags: superreview?(mrbkap)
Attachment #287505 -
Flags: review?(mrbkap)
Assignee | ||
Updated•17 years ago
|
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Summary: View source leaks many CTokens and nsStringBuffers → [FIX]View source leaks many CTokens and nsStringBuffers
Target Milestone: --- → mozilla1.9 M10
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: P1 → P3
Comment 3•17 years ago
|
||
Comment on attachment 287505 [details] [diff] [review]
Possible fix
Thanks for doing this. Changing the deallocator to release references instead of destroy tokens is conservative, but people should be on the lookout for leaking tokens. It's very possible that we're missing some IF_FREEs in CNavDTD and those will start showing up as leaks now.
Attachment #287505 -
Flags: superreview?(mrbkap)
Attachment #287505 -
Flags: superreview+
Attachment #287505 -
Flags: review?(mrbkap)
Attachment #287505 -
Flags: review+
Assignee | ||
Comment 4•17 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•