307160 - Leak in XUL template builder caused by bug 285076

Status: NEW

(Core :: XUL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The checkin for bug 285076 caused a leak increase on Balsa.  See graph at
http://build-graphs.mozilla.org/graph/query.cgi?testname=refcnt_leaks&units=bytes&tbox=balsa&autoscale=1&days=7&avg=1
and checkin range seems to be
http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-09-02+13%3A30&maxdate=2005-09-02+17%3A00&cvsroot=%2Fcvsroot

The new classes that are leaking are:

--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
nsNodeInfo                                      192          -
nsGenericDOMNodeList                             48          -
nsGenericElement                                112          -
nsNodeInfoManager                                40          -
AtomImpl                                        128     166.67%

and I've verified via XPCOM refcount logging that the nsGenericElement instances
(which are probably the leak roots here) are leaked through the XUL template
builder.

I suspect the problem is the following code in
nsXULContentBuilder::CreateTemplateAndContainerContents:

1129     if (tmpl)
1130         CreateTemplateContents(aElement, tmpl, aContainer,
aNewIndexInContainer);
1131 
1132     nsCOMPtr<nsIRDFResource> resource;
1133     nsXULContentUtils::GetElementRefResource(aElement,
getter_AddRefs(resource));
1134     if (resource) {
1135         // The element has a resource; that means that it corresponds
1136         // to something in the graph, so we need to go to the graph to
1137         // create its contents.
1138         CreateContainerContents(aElement, resource, PR_FALSE, aContainer,
aNewIndexInContainer);
1139     }

Now if both |tmpl| and |resource| are non-null and aContainer is non-null, we'll
write to *aContainer in both CreateTemplateContents and CreateContainerContents
and end up never releasing the ref that CreateTemplateContents adds.  At least
that's my best guess for what's going on given the leak logs.

Comment 1

[Scott MacGregor]

thanks Boris. I was tracking the performance numbers but didn't think to look at
Balsa. 

Comment 2

[Scott MacGregor]

Attachment: fix for the leak

Sorry for the delay in fixing this. Too much bug triage, not enough bug fixing
these days.

I verified that all the callers to CreateContainerContents fall into one of
three categories for the aContainer argument: 
1) aContainer is null
2) *aContainer is initialized to null 
3) or *aContainer points to a valid element (the case of this leak)

We can safely remove the nsnull assignment in CreateContainerContents,
replacing it with an NS_IF_RELEASE which properly handles the case where
*aContainer points to a valid element.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 197211 [details] [diff] [review]
fix for the leak

Hmm...	I'm a little unhappy with us making assumptions like that.  More
importantly, I'm not happy with the arbitrary way we're deciding that it's the
return value of CreateContainerContents we want, not that of
CreateTemplateContents.  Both can generate content, from what I see of the
code, and if they both do it looks like we'll notify on some of that content,
but not all....  The only way I can see this sort of approach being safe is if
we get both of the return containers (in different COMPtrs), assert that
they're the same, and take the min of the indices.... and even then I'd want
some very thorough testing of template-heavy stuff (of which there is a lot
more outside Firefox, from what I've seen) to make sure the assert is not
hit...

Comment 4

[Boris Zbarsky [:bzbarsky]]

This is assuming that that assert would have any bearing on reality.  I'm not
really sure it would.

Comment 5

[Neil Deakin]

There are cases where CreateTemplateContents and CreateContainerContents will
not return the same value for aContainer.

CreateTemplateContents should always set aContainer to either aElement or null.

CreateContainerContents may set aContainer to:
 - aElement
 - an element added by the call to CreateTemplateContents
 - a descendant of some element created outside of the template builder, which
is a bad thing to do anyway

In any case, aContainer is always a descendant of aElement (or null)

Comment 6

[Boris Zbarsky [:bzbarsky]]

So it sounds like if CreateTemplateContents returns anything we should use that,
and ignore what CreateContainerContents returns, then?  For purposes of
notifying on new content we've added to the tree?

Comment 7

[Neil Deakin]

That sounds about right under normal circumstances, if you meant to add that we
should use the CreateContainerContents value if the CreateTemplateContents value
is null.

There's supposed to be a way for CreateContainerContents to insert content
before the CreateTemplateContents content but it doesn't work, (bug 48774),
probably because of the notification thing assuming that content is always going
to be appended.

Also when I said "aContainer is always a descendant of aElement" and the end of
my previous comment, I meant to say "aContainer is always aElement or a
descendant of aElement"

Comment 8

[Boris Zbarsky [:bzbarsky]]

The "notification thing" is completely inside the template builder; if it should
be using ContentInserted() instead of ContentAppended(), we should do that...

Comment 9

[Scott MacGregor]

Attachment: a possible fix

Let's see if I understood Neil's comments correctly.

This always uses aContainer returned by CreateTemplateContents, if that returns
null for the container then we end up using the value returned by
CreateContainerContents.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 197451 [details] [diff] [review]
a possible fix

That looks right per Neil's description.  Neil, want to just confirm that?

Comment 11

[Scott MacGregor]

thanks for the reviews guys.

Comment 12

[Scott MacGregor]

Comment on attachment 197451 [details] [diff] [review]
a possible fix

This fix should bake on the trunk for a day or so just to be safe before
approving. 

This leak fixed is a requirement if we are going to take Bug 285076    .

Comment 13

[Scott MacGregor]

fixed on the trunk.

Comment 14

[Scott MacGregor]

Yeah! This looks fixed to me.

See the new graph:

http://build-graphs.mozilla.org/graph/query.cgi?testname=refcnt_leaks&units=bytes&tbox=balsa&autoscale=1&days=7&avg=1

Comment 15

[Mike Schroepfer]

Comment on attachment 197451 [details] [diff] [review]
a possible fix

Approved for 1.8b5.

Comment 16

[Scott MacGregor]

fixed on the branch now too.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Scott, I don't think that the leaks are fixed at all. Especially not when a
menulist is used to display constructed content from a XUL template builder.

To see it you can use following example:
http://www.xulplanet.com/ndeakin/tests/xul/template-guide-ex17.xul

Note that the content for menulist is not contructed for the first time you load
that page. So just reloading it. After it's finished select a country to view
only the assigned images. What you will see are at least four images. The first
ones are still there. To see a working example use a build earlier than
2005-09-03 - before you checked in the patch on bug 285076. The regression
starts definetely between the 0902 and 0903. That's the timeframe of the above
checkin.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #17)
> Scott, I don't think that the leaks are fixed at all. Especially not when a
> menulist is used to display constructed content from a XUL template builder.
> 
> To see it you can use following example:
> http://www.xulplanet.com/ndeakin/tests/xul/template-guide-ex17.xul

But it is working fine if the datasource was already loaded before opening this
page. Also the use of static content prevents the leak:

http://www.xulplanet.com/ndeakin/tests/xul/template-guide-ex18.xul

Comment 19

[Boris Zbarsky [:bzbarsky]]

That's a separate bug, please file it separately, mark it as blocking bug
285076, and cc me and scott.  This bug was on a particular issue (a leak on a
particular testcase) and that issue is fixed.

Given that your problem description never mentions leaks, I have no idea what
made you think you're seeing this bug.

Comment 20

[Scott MacGregor]

this has been backed out due to Bug #310833.

no longer a blocker as I've worked around it for 1.5