289970 - The sanitizing serializer leaks parsers

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

When I made the patch for bug 254989, I didn't fully realize that there was a
cycle between the parser and the content sinks. Therefore, my patch introduced a
leak of the parser and the content sink (and the various classes used by both of
them). I have a patch that fixes the leak, which I will attach shortly.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: fix the cycle

This fixes the cycle by having the content sink let go of the parser when the
parser is done with it (it calls DidBuildModel() when it's done).

Comment 2

[Ben Bucksch (:BenB)]

nsCOMPtr<nsIParser> temp(mParser); mParser = nsnull;

This looks iffy to me. Why is mParser = nsnull; insufficient? Doesn't that let
go of the reference? Alternatively a raw pointer and ADDREF/RELEASE?
I guess best would be a weak reference, if the parser is garanteed to be around
as long as you need it?

Anyways, all is fine with me. r=BenB.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 180420 [details] [diff] [review]
fix the cycle

Marking this r+ based on comment 2 and requesting approval for 1.8b2 for this
leak fix.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Ben, the problem with just setting |mParser = nsnull| or using a weak ref, is
that the parser may go away before the content sink does, which could
potentially cause problems. In the case of not using the temp nsCOMPtr, the sink
could get deleted before the assignment to mParser takes place, which would crash.

Comment 5

[Asa Dotzler [:asa]]

Comment on attachment 180420 [details] [diff] [review]
fix the cycle

a=asa

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.