Leaks went up by about 200k

VERIFIED FIXED in mozilla0.9.1



DOM: Core & HTML
17 years ago
7 years ago


(Reporter: peterv, Assigned: peterv)


({mlk, topmlk})

mlk, topmlk

Firefox Tracking Flags

(Not tracked)


(Whiteboard: want for mozilla 0.9.1 - have r=heikki,sr=jst,a=chofmann, URL)


(3 attachments)



17 years ago
After my check-in for bug 81989 leaks went up by about 200k. I have found what
the problem is, the nsStyleLinkingElement isn't releasing the parser if it
doesn't load a stylesheet. I'm attaching a fix.


17 years ago
Priority: -- → P1
Target Milestone: --- → mozilla0.9.1

Comment 1

17 years ago
Created attachment 36345 [details] [diff] [review]
Release the parser if we're not blocking it


17 years ago
Keywords: mlk

Comment 2

17 years ago
peterv, lets get someone to review your patch and test it out.

Comment 3

17 years ago
While you're at it, remove the unused local variable you introduced in bug
81989, too. ;-)
@@ -71,8 +72,9 @@
   nsCOMPtr<nsIParser> mParser;
   PRBool mDontLoadStyle;
   PRBool mUpdatesEnabled;
   PRBool mAppend;
+  PRBool mBlockingParser;

Perfect example of where to use PRPackedBool, four times in the row.


Comment 5

17 years ago
I think I have a simpler fix: the CSS Loader keeps a strong ref to the parser
(in its SheetLoadData), so it seems the strong ref in nsStyleLinkelement is
unnecessary. I haven't run into any problems with it yet. The content sink,
which used to launch the style link loads didn't hold a strong ref either. I
would appreciate it if someone else could verify that it does fix the memory
leak. It does for me, but I don't trust my own eyes anymore :(.

Comment 6

17 years ago
Created attachment 36379 [details] [diff] [review]
Don't hold a strong ref to the parser
Couldn't the logic for releasing the parser be isolated to nsStyleLinkElement,
do we need the API change, and do we need to make the sink call ReleaseParser(),
why not let the nsStyleLinkElement deal with releasing the parser?

Couldn't you make the nsStyleLinkElement release the parser if we're not
blocking the parser and use the fact that there is a parser to tell that we must
unblock the parser when we're done, then you wouldn't need to add the new PRBool
member. Then you could continue holding a strong ref (which is a tad safer) to
the parser in the nsStyleLinkElement.cpp.

I agree with Brendan, use PRPackedBool, but be careful. Don't pass
PRPackedBool's by address or reference to anything that takes PRBool* or PRBool
& since that might write in memory that doesn't belong to the PRPackedBool,
right, Brendan?

Also, I don't see mAppend being used anywhere, could that be removed?

Comment 8

17 years ago
Created attachment 36406 [details] [diff] [review]
Keep strong ref as long as necessary

Comment 9

17 years ago
Third try. We keep the strong ref, but only as long as necessary: until we pass
it to the CSS loader (which keeps it own strong ref) or before that if we fail
to call the CSS loader (and do an early return).


17 years ago
Keywords: topmlk
Whiteboard: want for mozilla 0.9.1

Comment 11

17 years ago
is this ready for checkin?   lets get it in quickly if it is...
Whiteboard: want for mozilla 0.9.1 → want for mozilla 0.9.1 - have r=sr=
r=heikki, although I would suggest using something else as "kungFuDeathGrip" as
the temp variable name holding the parser. Typically that name is used only as a
ref holder, and nothing is done with it beyond initialization. Simply "parser"
would do in my opinion.

You are free to check in, I think.
Whiteboard: want for mozilla 0.9.1 - have r=sr= → want for mozilla 0.9.1 - have r=heikki,sr=jst

Comment 13

17 years ago
I'll check this in tonight with parser instead of kungFuDeathGrip.
Whiteboard: want for mozilla 0.9.1 - have r=heikki,sr=jst → want for mozilla 0.9.1 - have r=heikki,sr=jst,a=chofmann

Comment 14

17 years ago
Leaks are back to where they were.
Last Resolved: 17 years ago
Resolution: --- → FIXED


16 years ago
QA Contact: lchiang → stummala

Comment 15

16 years ago
verified patch check in.marking as verified


7 years ago
Component: DOM: Abstract Schemas → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.