310833 - Template builder doesn't free constructed content when menulists are in use

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

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

When using the xul template builder to fill a menulist the constructed content
is not freed.

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. 

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

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 1

[Boris Zbarsky [:bzbarsky]]

Could use a minimal testcase here. That said, I don't really know how this ever
worked -- I don't see the template builder or content builder ever calling
ContentRemoved...

Comment 2

[Neil Deakin]

The content builder always calls RemoveChildAt with a notify of true, so
ContentRemoved shouldn't be necessary.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  So any idea why a change to nsXULContentBuilder::CreateContents would
change the behavior here?

Comment 4

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

Tweaking the summary due to it's not the content of a menulist, instead it's the
template of another element which uses the same rdf datasource.

Btw. same happens when you load example 18 before example 17.

Comment 5

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

Generally I get the following assertion when reloading the page, and not only
when the datasource is used twice.

###!!! ASSERTION: unexpected second call to SetInitialChildList: 'Not Reached',
file ../../../../mozilla/layout/generic/nsContainerFrame.cpp, line 108
Break: at file ../../../../mozilla/layout/generic/nsContainerFrame.cpp, line 108
WARNING: Warning add child failed!!
, file ../../../../../../mozilla/layout/xul/base/src/nsBoxFrame.cpp, line 205

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  So the problem here is that XUL lazily generates template content
and it does so within its GetChildCount method.  This is called within frame
construction in a variety of ways, and the net effect is that the frame tree
mutates while we're building it, which frame construction can't deal with (and
really shouldn't have to).

So in brief, the patch in bug 285076 breaks existing templates in a way that I
see no way of fixing without reworking the template system (probably to not be
lazy... or maybe we can think of something more clever and more complicated). 
Given that it's a choice between breaking existing template consumers (in a way
they really cannot work around) and not having templates play nice with a new
and half-baked feature that we're explicitly saying is not stable (dynamic
overlays), I think I'd lean toward backing bug 285076 for 1.8.

Comment 7

[Neil Deakin]

You can't really remove the lazy construction or a bookmarks menu with lots of
stuff in it will slow down startup time. (since the entire bookmark hierarchy
will be built)

Comment 8

[Scott MacGregor]

(In reply to comment #6)
> Given that it's a choice between breaking existing template consumers (in a way
> they really cannot work around) and not having templates play nice with a new
> and half-baked feature that we're explicitly saying is not stable (dynamic
> overlays), I think I'd lean toward backing bug 285076 for 1.8.

Well, we may between a rock and a hard place then. I don't know what you mean by
us explicitly saying dynamic overlays aren't stable, but without 285076, the
thunderbird options dialog is unuseable as you can't change several options
because the content never shows up for many of the menu lists. 

How about other ideas for 285076, such as only calling the new code in the case
of a dynamic overlay load. That at least limits the scope to just the options
dialog for Thunderbird and firefox.

Comment 9

[Asa Dotzler [:asa]]

marking as a blocker because it looks like we have to do _something_ here.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I don't know what you mean by us explicitly saying dynamic overlays aren't
> stable

The API clearly says they shouldn't be relied on because they'll change without
notice.

> because the content never shows up for many of the menu lists. 

As a workaround, does setting the display to none and setting a timeout to unset
that help?  So in pseudocode:

overlayOnload="content.style.display = 'none'; setTimeout(0, function() {
content.style.display = ''; }"

> such as only calling the new code in the case of a dynamic overlay load.

I don't believe we have a reasonable way to detect that, do we?

Comment 11

[Boris Zbarsky [:bzbarsky]]

The other question is whether it's at all possible to wean the relevant options
panels off dynamic overlays if all else fails...

Comment 12

[Scott MacGregor]

(In reply to comment #10)
> As a workaround, does setting the display to none and setting a timeout to unset
> that help?  So in pseudocode:
> 
> overlayOnload="content.style.display = 'none'; setTimeout(0, function() {
> content.style.display = ''; }"

I haven't been able to get this to work yet. Toggling the display attribute on
the menulists/menupopus doesn't seem to have any effect. I'll  keep trying.

Comment 13

[Scott MacGregor]

Update: I'm still working on some work arounds tonight for this bug.

Comment 14

[Scott MacGregor]

Attachment: back out xul template builder changes, re-write thunderbird prefs to not use the template builder

*sigh*

Last minute save for the branch.

Back out the changes to the xul template builder which made it work with
dynamic overlays.

Re-write the Thunderbird widgets to not use the xul template builder in the
prefs dialog, populate everything by hand, brute force.

Comment 15

[Scott MacGregor]

Comment on attachment 198418 [details] [diff] [review]
back out xul template builder changes, re-write thunderbird prefs to not use the template builder

self approving.

folks only care about the changes to nsXULContentBulder. Those changes are a
simple back out for the two dynamic overlay bugs, returning us to where we were
a few days ago. No risk there.

I'll have bienvenu finish reviewing the brute force menu list building changes
for Thunderbird in the morning.

Comment 16

[David :Bienvenu]

Comment on attachment 198418 [details] [diff] [review]
back out xul template builder changes, re-write thunderbird prefs to not use the template builder

+	   if (preference.value == abURI)
+	   {
+.	      aPopup.parentNode.value = abURI;
+	     aPopup.selectedItem = item;

this looks like a typo - the '.' at the beginning of the line...

other than that, it looks ok.