Leaks went up by about 200k

VERIFIED FIXED in mozilla0.9.1

Status

()

Core
DOM: Core & HTML
P1
critical
VERIFIED FIXED
17 years ago
7 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

({mlk, topmlk})

Trunk
mozilla0.9.1
mlk, topmlk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments)

(Assignee)

Description

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.
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.1
(Assignee)

Comment 1

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

Updated

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.

/be
(Assignee)

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 :(.
(Assignee)

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?
(Assignee)

Comment 8

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

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).
r/sr=jst

Updated

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
(Assignee)

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
(Assignee)

Comment 14

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

Updated

16 years ago
QA Contact: lchiang → stummala

Comment 15

16 years ago
verified patch check in.marking as verified
Status: RESOLVED → VERIFIED

Updated

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.