285076 - XUL Template Builder isn't working with Dynamic overlays

Status: RESOLVED WORKSFORME

(Core :: XUL, defect)

Description

[Scott MacGregor]

With the new preferences design, elements that use the XUL template builder in
Thunderbird (such as the privacy / general / sender's white list menu list) fail
to get their RDF datasources reflected into the drop down menu lists.

Comment 1

[Scott MacGregor]

just to avoid dupes, this also effects the character encoding language menus.

Comment 2

[Scott MacGregor]

*** Bug 285302 has been marked as a duplicate of this bug. ***

Comment 3

[Heiko Adams]

I can ACK this bug on Windows 2K too. So changing OS to "All" would usefull IMHO

Comment 4

[Scott MacGregor]

Attachment: the fix

if we have already finished the initial reflow, then we should notify the
document and any observers of any content changes by the xul template builder.
This is the matching set of changes that were made to the XUL Document to
support dynamic overlays.

Comment 5

[Scott MacGregor]

FYI, i changed the comment in this patch to say:

  // If we've already done initial reflow, then set notify to true
  // to force notifications when content elements are added to the document.  

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 176783 [details] [diff] [review]
the fix

Changing review request to jst, since this isn't technically in my code.

Comment 7

[Scott MacGregor]

*** Bug 285427 has been marked as a duplicate of this bug. ***

Comment 8

[Scott MacGregor]

Hey Johnny, can you review this patch when you have some time? 

Comment 9

[Scott MacGregor]

*** Bug 286568 has been marked as a duplicate of this bug. ***

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 176783 [details] [diff] [review]
the fix

r=jst

Comment 11

[tabmix.onemen]

Regressions by the fix ????
with BEAST Tinderbox builds (hourly)
on new and old profile i get all the above

   1. double entry in Bookmarks Toolbar Folder
   2. double entry in Download Manager
   3. double entry in Extensions Manager
   4. triple entry in the toolbar for bottons created by costomizable toolbar
bottons extension
in dom inspector i don't see the double entry for the Bookmarks Toolbar Folder.

Comment 12

[Frederic Bezies]

Re comment 11 

This is bug 286709.

Comment 13

[tabmix.onemen]

(In reply to comment #12)
> Re comment 11 
> 
> This is bug 286709.

286709 is new bug. but what caused it???. maybe the fix of this bug???

Comment 14

[Frederic Bezies]

Indeed? I meant that while writing my comment. A PITA bug, to say so :(

Comment 15

[Scott MacGregor]

I had to back this out because of regressions it caused with the Firefox
bookmarks manager. I guess we'll have to come up with some other way of
determining when to set the notifiation flag to true besides checking for
initial reflow.

Comment 16

[Scott MacGregor]

Attachment: another approach

this idea builds upon the previous fix but instead of testing to see if we've
done the initial reflow before passing in TRUE for notifications, we instead
look at the xul document and check to see if it thinks we are processing a
dynamic overlay.

This more explicit approach avoids the duplicate content issues the previous
patch had. The downside is that it now exposes dynamic overlay loading state as
a property on a XUL document. And currently, in the case of errors, its
possible that this flag would not get cleared so it still needs some work.

Comment 17

[Benjamin Smedberg]

Rob/Doron, this seems related to the issues with richlistbox and templates, is
there a more generic solution that could happen here?

Comment 18

[Doron Rosenberg (IBM)]

(In reply to comment #17)
> Rob/Doron, this seems related to the issues with richlistbox and templates, is
> there a more generic solution that could happen here?

The problem in the managers is that the widget never knows when its template has
finished creating the data, not sure if is is related

Comment 19

[Scott MacGregor]

Does anyone else have any good ideas on how to know when it is appropriate to
issue notifications when building new content other than the approach I started
to work on in comment 16? Unfortunately checking to see if we've already done an
initial reflow and only sending notifications if we have works for dynamic
overlays but causes problems for non dynamic overlays. Hence my attempt to try
to add a dynamic overlay check on the xul document.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Does anyone else have any good ideas on how to know when it is appropriate to
> issue notifications

Notifications must ALWAYS be issued, if we want to avoid problems.  If aNotify
is passed as false to AppendChildTo or RemoveChildAt or whatever, then the
caller takes the responsibility of later sending the notification onto himself
(as the content sink does, for example).  Trying to actually not send
notifications based on reflow state or anything else is a kludge which may lead
to "correct" rendering (in that the frame model will be right) but can also lead
to incorrect functioning (for example content lists missing data they should
have in them).

Comment 21

[Scott MacGregor]

Boris, currently notifications never get generated here as PR_FALSE is always
passed in as the notification parameter. This means that in the case of dynamic
overlays where the template content builder is building content after the
document has loaded, that the UI never shows the new content because it never
gets notified. I tried to fix this once before by always passing in TRUE for
aNotify if we've already done the initial document load, but that change caused
content to show up twice in several areas of the Firefox UI. Hence my attempt to
try to be more precise by knowing if we are loading a dynamic overlay or not
when deciding the value of aNotify. 

http://lxr.mozilla.org/mozilla/source/content/xul/templates/src/nsXULContentBuilder.cpp#1136

Comment 22

[Boris Zbarsky [:bzbarsky]]

Right.  My point is that the current overlay code is just generally buggy if it
always passes PR_FALSE and never notifies after that...

Did someone figure out why we're ending up with doubled content?  That usually
indiecates either doubled notification or content getting a frame constructed
before it's notified...

Comment 23

[Scott MacGregor]

*** Bug 303357 has been marked as a duplicate of this bug. ***

Comment 24

[Mark Banner (:standard8)]

*** Bug 301934 has been marked as a duplicate of this bug. ***

Comment 25

[Scott MacGregor]

(In reply to comment #22) 
> Did someone figure out why we're ending up with doubled content?  That usually
> indiecates either doubled notification or content getting a frame constructed
> before it's notified...

Hey Boris, here's what I'm seeing in the debugger when opening up the themes
dialog  with one theme. In the onload handler for the dialog, we set a data
source href attribute on the widget to trigger the content building code
http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/content/extensions.js#177

In reponse to getting the href attribute set, I see two calls to
nsXULDocument::ContentAppended for the element. The first call is triggered by
nsXULElement::InsertChildAt from the XULSortService. Here's the relevant stack
trace.

nsXULDocument::ContentAppended
nsXULElement::InsertChildAt
XULSortServiceImpl::InsertContainerNode
nsXULContentBuilder::BuildContentFromTemplate
nsXULContentBuilder::CreateContainerContents
nsXULContentBuilder::CreateTemplateAndContainerContents
nsXULContentBuilder::RebuildAll
nsXULTemplateBuilder::Rebuild
nsXULTemplateBuilder::AttributeChanged
nsXULContentBuilder::AttributeChanged
nsXULDocument::AttributeChanged
nsXULElement::SetAttrAndNotify
nsXULElement::SetAttr
nsGenericElement::SetAttr
nsGenericElement::SetAttribute
nsXULElement::SetAttribute

Now, the stack starts to unwind all the way down to:
nsXULContentBuilder::RebuildAll. After calling 
CreateTemplateAndContainerElements, it then calls ContentAppended. This is the
second call for the same element:

http://lxr.mozilla.org/mozilla/source/content/xul/templates/src/nsXULContentBuilder.cpp#1909

And that is how we end up showing the same element twice if we always pass in
true for aNotify in nsXULContentBuilder::CreateTemplateAndContainerContents

Comment 26

[Scott MacGregor]

Attachment: another fix

Boris, what do you think of something like this?

1) CreateTemplateAndContainerContents now always passes PR_TRUE for aNotify
instead of PR_FALSE into CreateContainerContents. This means that
CreateTemplateAndContainerContents will always result in ContentAdded
notifications at the document level.

2) Get rid of the code in nsXULContentBuilder::RebuildAll which manually called
ContentAppended after CreateTemplateAndContainerContents since
CreateTemplateAndContainerContents will now generate the notification for us.

Comment 27

[Scott MacGregor]

Boris, do you have any initial thoughts on my proposed fix? If it seems
reasonable, I'd like to start getting trunk coverage on it as soon as I can.

Comment 28

[Boris Zbarsky [:bzbarsky]]

I don't know this code well enough to judge offhand, and I won't be able to
review this for at least a week or two, if only because I have to learn the
code.  If I understand what's going on at all, though, I suspect this will
severely degrade the performance of XUL templates by making the number of
notifications for an insertion O(number of nodes inserted) instead of 1.

Comment 29

[Scott MacGregor]

(In reply to comment #28)
> I don't know this code well enough to judge offhand, and I won't be able to
> review this for at least a week or two, if only because I have to learn the
> code.  If I understand what's going on at all, though, I suspect this will
> severely degrade the performance of XUL templates by making the number of
> notifications for an insertion O(number of nodes inserted) instead of 1.

hmm I must have misunderstood you in comment 20 where I thought you were
suggesting we should always pass in PR_TRUE for the aNotify parameter whenever
an element is added or removed on the document.

Comment 30

[Boris Zbarsky [:bzbarsky]]

What I said is that notifications must always be issued, either by passing
aNotify == PR_TRUE or by doing the notifying yourself later in batches like the
content sink.  The latter is faster, which is why the content sink does it.

Comment 31

[Scott MacGregor]

Attachment: another fix

Ok, here's another approach that hopefully won't break batching of content
notification calls. 

Make aNotify an argument to CreateTemplateAndContainerContents. This allows
callers that wish to block notifications (such as ::RebuildAll), to pass in
PR_FALSE for aNotify.

Comment 32

[Scott MacGregor]

Comment on attachment 192260 [details] [diff] [review]
another fix

clearing an obsolete request since this patch breaks batching notifications.

Comment 33

[Boris Zbarsky [:bzbarsky]]

So what's the net effect of this patch?  Is it to notify when we didn't use to
before, basically?  It'd really help to have more context and to use the -p
option to diff, by the way....

Comment 34

[Scott MacGregor]

Attachment: cvs diff -p for boris

same patch, with cvs diff -p for Boris.

Boris, this patch implements your idea in comment 20 about always generating
notifications in order to avoid problems, without degrading performance in
RebuildAll where we want to batch the construction notifications. Hope that
helps, thanks for looking at the patch.

It might be interesting to check this patch into the trunk for a tinderbox
cycle to see if we get any data from the automated xul performance tests.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 193515 [details] [diff] [review]
another fix

I meant something more like diff -up8.... ;)

>Index: nsXULContentBuilder.cpp

>     nsresult
>     CreateTemplateAndContainerContents(nsIContent* aElement,
>+                                       PRBool aNotify,
>                                        nsIContent** aContainer,
>                                        PRInt32* aNewIndexInContainer);

Please add the following at the beginning of that method:

NS_PRECONDITION(!aContainer == !aNewIndexInContainer,
		"Must ask for either both or neither");
NS_PRECONDITION(aNotify || aContainer, "Notifications will get lost");

>@@ -1534,7 +1536,7 @@
>+    return CreateTemplateAndContainerContents(aElement, PR_TRUE /* aNotify */, nsnull /* don't care */, nsnull /* don't care */);

Why not manually notify like RebuildAll does?  It seems like that should be
faster, I think, if it gets the right notification This code is called for any
XUL element which might have generated kids, after all.

That said r=bzbarsky for this patch if tinderbox tests are not affected, I
guess.

Comment 36

[Scott MacGregor]

Attachment: yet another possible solution

This approach makes nsXULContentBuilder::CreateContents look a lot like
RebuildAll in that it manually fires the content appended notification. I'm not
sure which approach is better. I'm leaning towards this one though. 

I wonder if I should be doing 

nsCOMPtr<nsIDocument> doc = mRoot->GetDocument();

instead of aElement->GetDocument

although it shouldn't matter. I'll do some more testing.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Ideally you want to use container->GetCurrentDoc() for the document.  That'll
guarantee that it's the right document to notify about content being added to
|container|.

Do templates always only append content to a container?  They can't insert it
somewhere in the middle of it?

Comment 38

[Scott MacGregor]

The comment for BuildContentFromTemplate 

http://lxr.mozilla.org/seamonkey/source/content/xul/templates/src/nsXULContentBuilder.cpp#401

implies that content is only appended by the template builder:

"|aContainer| is an out parameter that will be set to the first
container element in the "real" content tree to which content
was appended." A brief code inspection of that method seems to support that comment.

Hmm, the other callers of ContentAppended in this class all seem to get the
document from mRoot. My gut tells me we should follow that pattern, but I don't
have a technical opinion one way or the other on using mRoot vs.
container->GetContainerDoc()

Comment 39

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  In that case this last solution is best, imo.

Comment 40

[Boris Zbarsky [:bzbarsky]]

One other thing I just realized.  It's worth calling BeginUpdate and EndUpdate
around the notification.  Not doing that can easily lead to bugs.  We have a
mozAutoDocUpdate class to handle this automatically, though in this case that's
not really necessary since there are no early returns involved.

Comment 41

[Scott MacGregor]

Attachment: updated fix using mRoot

Ok, this patch uses mRoot to get the document to be consistent with the rest of
the callers in this file.

None of the other 3 callers of ContentAppended are calling
BeginUpdate/EndUpdate in nsXULContentBuilder. Should they be? If so, I'd break
this into two parts, just the basic change for the 1.8 branch and then fixing
all callers to use BeginUpdate/EndUpdate in the XULContentBuilder for the
trunk.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 194259 [details] [diff] [review]
updated fix using mRoot

>Index: nsXULContentBuilder.cpp

> nsXULContentBuilder::CreateTemplateAndContainerContents(nsIContent*

>+    NS_PRECONDITION(!aContainer == !aNewIndexInContainer,
>+		                "Must ask for either both or neither");

Fix the indent?

>@@ -1124,17 +1126,17 @@ nsXULContentBuilder::CreateTemplateAndCo
>-    if (resource) {
>+    if (resource) {      

Please don't add those trailing spaces.

> +        nsCOMPtr<nsIDocument> doc = mRoot->GetDocument();

This can be a weak nsIDocument* ref, and please use GetCurrentDoc(), not
GetDocument().

sr=bzbarsky, with those changes though in the future we probably want to not
use mRoot in a lot of places in this file...

> None of the other 3 callers of ContentAppended are calling
> BeginUpdate/EndUpdate in nsXULContentBuilder. Should they be?

Yes, if we ever hope to get incremental layout working for XUL... I agree that
this should probably be a separate bug and not something to worry about on the
1.8 branch.

Comment 43

[Scott MacGregor]

Attachment: updated patch based on review comments

uses a weak reference to the document, calls GetCurrentDoc and fixes some white
space issues.

carrying forward bz's sr.

Comment 44

[Scott MacGregor]

I've checked this into the trunk so we can get some baking time on the trunk.
I'll also be watching the performance numbers on tinderbox just in case.

Comment 45

[Boris Zbarsky [:bzbarsky]]

This checkin caused memory leaks -- see bug 307160

Comment 46

[Asa Dotzler [:asa]]

What needs to happen here to get this onto the branch? Time is running short for
1.8b5.

Comment 47

[Scott MacGregor]

What needs to happen: We're waiting to verify that the leak this introduced is
really fixed on the trunk before we land this on the branch. The leak is BUg
#307160.

Comment 48

[Scott MacGregor]

Comment on attachment 194378 [details] [diff] [review]
updated patch based on review comments

I've verified that the leak fix (Bug 307160) is fixed on the trunk. So both of
these bugs are ready to go for the branch.

Comment 49

[Mike Schroepfer]

Comment on attachment 194378 [details] [diff] [review]
updated patch based on review comments

Approved for 1.8b5.

Comment 50

[Mike Schroepfer]

Comment on attachment 194378 [details] [diff] [review]
updated patch based on review comments

Approved for 1.8b5.

Comment 51

[Scott MacGregor]

this is now fixed on the branch.

Comment 52

[Boris Zbarsky [:bzbarsky]]

This caused bug 310833 -- basically, a wide variety of templates (including
especially templates that are interacting with XBL-bound elements) are broken.

Comment 53

[Scott MacGregor]

this has been backed out due to Bug #310833.

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

Comment 54

[Boris Zbarsky [:bzbarsky]]

So for trunk, we should think about how to fix this...  The basic problem the
approach we tried here ran into is that in some cases
nsXULContentBuilder::CreateContents needs to NOT notify.  In particular, if it's
being called by the GetChildCount() method of a XUL element while we're
constructing frames...  otherwise we end up notifying (which constructs frames)
and then just constructing frames.  Which gives us two frames per node, which is
bad.

The problem is that GetChildCount() has no idea who's calling it and why, and
I'd really rather not change every single caller of it.  So what are our options
here?

Comment 55

[Cédric Corazza]

This seems related to this bug.
When launching Thunderbird 1.5 (20051218) and going to Edition > Preferences > Display > Fonts both "Outgoing Mail" and "Incoming mail" dropdown lists work correctly. If closing the Preferences window and then returning to Display > Fonts item, the dropdown lists don't work anymore --you cannot choose a different charset-- and the text of the charset is superposed over the dropdown list arrows.
If exiting Thunderbird then returning to this preference, it works again the first time this preference is displayed, then don't work the second time (at least for the en-US version, for the French one (build 20051201), I had to remove XUL.mfasl to make this preference work again).
Hum, I hope I was clear in my explanation...
My conf : GNU/Linux 2.6.13-15 (OpenSuse)

Is this a blocker for 1.5 release?

Comment 56

[Cédric Corazza]

Update :
Sorry for the bugspam. For the French version, same behavior as for the en-US one: you only need to restart Thunderbird for the preference to work fine again. Don't know why I had to remove XUL.mfasl before.

Comment 57

[Mike Cowperthwaite]

(In reply to comment #55)
> This seems related to this bug.
> both "Outgoing Mail" and "Incoming mail" dropdown lists work correctly.
> If closing the Preferences window and then returning to Display > Fonts item,
> the dropdown lists don't work anymore --you cannot choose a different
> charset-- and the text of the charset is superposed over the dropdown
> list arrows.

This problem is bug 315057.  I agree, it appears related.  It was denied 1.5-blocker status; maybe if we're lucky someone will bother to fix it, and then
we can get it included in a subsequent 1.5.x. release.

Comment 58

[Worcester12345]

What is the story on this bug? Is it fixed, WFM, or what? I was just getting rid of some old bugs in my inbox. Sorry for the bother. Thanks.

Comment 59

[Ian Neal]

No, not fixed AFAIA

Comment 60

[Magnus Melin [:mkmelin]]

Overlays are gone since long.