[FIX]View source leaks many CTokens and nsStringBuffers

RESOLVED FIXED in mozilla1.9beta2

Status

()

P3
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: jruderman, Assigned: bzbarsky)

Tracking

({memory-leak})

Trunk
mozilla1.9beta2
memory-leak
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

11 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

11 years ago
Summary: View source leaks many nsStringBuffers → View source leaks many CTokens and nsStringBuffers
(Assignee)

Comment 2

11 years ago
Created attachment 287505 [details] [diff] [review]
Possible fix

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

11 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

11 years ago
Flags: blocking1.9? → blocking1.9+
Priority: P1 → P3
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

11 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.