Bug 118863. Fix several incorrect uses of IsElementContainedBy when IsElementInBuilder is required. Fixes problems with nested templates. r=rjc, sr=hyatt.

content/xul/templates/src/nsContentTestNode.cpp

@@ -50,45 +50,20 @@
 extern PRLogModuleInfo* gXULTemplateLog;
 #endif
 
-PRBool
+extern PRBool
-IsElementContainedBy(nsIContent* aElement, nsIContent* aContainer)
+IsElementInBuilder(nsIContent *aContent, nsIXULTemplateBuilder *aBuilder);
-{
-    // Make sure that we're actually creating content for the tree
-    // content model that we've been assigned to deal with.
-
-    // Walk up the parent chain from us to the root and
-    // see what we find.
-    if (aElement == aContainer)
-        return PR_TRUE;
-
-    // walk up the tree until you find rootAtom
-    nsCOMPtr<nsIContent> element(do_QueryInterface(aElement));
-    nsCOMPtr<nsIContent> parent;
-    element->GetParent(*getter_AddRefs(parent));
-    element = parent;
-    
-    while (element) {
-        if (element.get() == aContainer)
-            return PR_TRUE;
-
-        element->GetParent(*getter_AddRefs(parent));
-        element = parent;
-    }
-    
-    return PR_FALSE;
-}
 
 nsContentTestNode::nsContentTestNode(InnerNode* aParent,
                                      nsConflictSet& aConflictSet,
                                      nsIXULDocument* aDocument,
-                                     nsIContent* aRoot,
+                                     nsIXULTemplateBuilder* aBuilder,
                                      PRInt32 aContentVariable,
                                      PRInt32 aIdVariable,
                                      nsIAtom* aTag)
     : TestNode(aParent),
       mConflictSet(aConflictSet),
       mDocument(aDocument),
-      mRoot(aRoot),
+      mBuilder(aBuilder),
       mContentVariable(aContentVariable),
       mIdVariable(aIdVariable),
       mTag(aTag)
@@ -107,6 +82,21 @@ nsContentTestNode::nsContentTestNode(InnerNode* aParent,
 #endif
 }
 
+#ifdef PR_LOGGING
+static void
+ElementToString(nsIContent *aContent, nsString &aResult)
+{
+    aResult.Truncate();
+
+    nsCOMPtr<nsIAtom> tag;
+    aContent->GetTag(*getter_AddRefs(tag));
+    tag->ToString(aResult);
+
+    aResult.Append(PRUnichar('@'));
+    aResult.AppendInt(PRInt32(aContent), 16);
+}
+#endif
+
 nsresult
 nsContentTestNode::FilterInstantiations(InstantiationSet& aInstantiations, void* aClosure) const
 {
@@ -124,6 +114,22 @@ nsContentTestNode::FilterInstantiations(InstantiationSet& aInstantiations, void*
         Value idValue;
         PRBool hasIdBinding = inst->mAssignments.GetAssignmentFor(mIdVariable, &idValue);
 
+#ifdef PR_LOGGING
+        if (PR_LOG_TEST(gXULTemplateLog, PR_LOG_DEBUG)) {
+            nsAutoString content(NS_LITERAL_STRING("(unbound)"));
+            if (hasContentBinding)
+                ElementToString(VALUE_TO_ICONTENT(contentValue), content);
+
+            const char *id = "(unbound)";
+            if (hasIdBinding)
+                VALUE_TO_IRDFRESOURCE(idValue)->GetValueConst(&id);
+
+            PR_LOG(gXULTemplateLog, PR_LOG_DEBUG,
+                   ("nsContentTestNode[%p]: FilterInstantiations() content=%s id=%s",
+                    this, NS_LossyConvertUCS2toASCII(content).get(), id));
+        }
+#endif
+
         if (hasContentBinding && hasIdBinding) {
             // both are bound, consistency check
             PRBool consistent = PR_TRUE;
@@ -147,6 +153,9 @@ nsContentTestNode::FilterInstantiations(InstantiationSet& aInstantiations, void*
                     consistent = PR_FALSE;
             }
 
+            PR_LOG(gXULTemplateLog, PR_LOG_DEBUG,
+                   ("    consistency check => %s", consistent ? "passed" : "failed"));
+
             if (consistent) {
                 Element* element =
                     nsContentTestNode::Element::Create(mConflictSet.GetPool(),
@@ -172,8 +181,18 @@ nsContentTestNode::FilterInstantiations(InstantiationSet& aInstantiations, void*
                 nsCOMPtr<nsIAtom> tag;
                 content->GetTag(*getter_AddRefs(tag));
 
-                if (tag != mTag)
+                if (tag != mTag) {
                     consistent = PR_FALSE;
+
+                    const PRUnichar *expected, *actual;
+                    mTag->GetUnicode(&expected);
+                    tag->GetUnicode(&actual);
+
+                    PR_LOG(gXULTemplateLog, PR_LOG_DEBUG,
+                           ("    => tag mismatch; expected %s, actual %s",
+                            NS_LossyConvertUCS2toASCII(expected).get(),
+                            NS_LossyConvertUCS2toASCII(actual).get()));
+                }
             }
 
             if (consistent) {
@@ -181,6 +200,15 @@ nsContentTestNode::FilterInstantiations(InstantiationSet& aInstantiations, void*
                 nsXULContentUtils::GetElementRefResource(content, getter_AddRefs(resource));
 
                 if (resource) {
+#ifdef PR_LOGGING
+                    if (PR_LOG_TEST(gXULTemplateLog, PR_LOG_DEBUG)) {
+                        const char *str;
+                        resource->GetValueConst(&str);
+                        PR_LOG(gXULTemplateLog, PR_LOG_DEBUG,
+                               ("    => [%s]", str));
+                    }
+#endif
+
                     Instantiation newinst = *inst;
                     newinst.AddAssignment(mIdVariable, Value(resource.get()));
 
@@ -194,6 +222,10 @@ nsContentTestNode::FilterInstantiations(InstantiationSet& aInstantiations, void*
 
                     aInstantiations.Insert(inst, newinst);
                 }
+                else {
+                    PR_LOG(gXULTemplateLog, PR_LOG_DEBUG,
+                           ("    => element has no resource"));
+                }
             }
 
             aInstantiations.Erase(inst--);
@@ -201,21 +233,19 @@ nsContentTestNode::FilterInstantiations(InstantiationSet& aInstantiations, void*
         else if (hasIdBinding) {
             // the 'id' is bound, find elements in the content tree that match
             const char* uri;
-            rv = VALUE_TO_IRDFRESOURCE(idValue)->GetValueConst(&uri);
+            VALUE_TO_IRDFRESOURCE(idValue)->GetValueConst(&uri);
-            if (NS_FAILED(rv)) return rv;
+
+            mDocument->GetElementsForID(NS_ConvertUTF8toUCS2(uri), elements);
 
-            rv = mDocument->GetElementsForID(NS_ConvertUTF8toUCS2(uri), elements);
-            if (NS_FAILED(rv)) return rv;
             PRUint32 count;
-            rv = elements->Count(&count);
+            elements->Count(&count);
-            if (NS_FAILED(rv)) return rv;
 
             for (PRInt32 j = PRInt32(count) - 1; j >= 0; --j) {
                 nsISupports* isupports = elements->ElementAt(j);
                 nsCOMPtr<nsIContent> content = do_QueryInterface(isupports);
                 NS_IF_RELEASE(isupports);
 
-                if (IsElementContainedBy(content, mRoot)) {
+                if (IsElementInBuilder(content, mBuilder)) {
                     if (mTag) {
                         // If we've got a tag, check it to ensure
                         // we're consistent.
@@ -226,6 +256,16 @@ nsContentTestNode::FilterInstantiations(InstantiationSet& aInstantiations, void*
                             continue;
                     }
 
+#ifdef PR_LOGGING
+                    if (PR_LOG_TEST(gXULTemplateLog, PR_LOG_DEBUG)) {
+                        nsAutoString str;
+                        ElementToString(content, str);
+
+                        PR_LOG(gXULTemplateLog, PR_LOG_DEBUG,
+                               ("    => %s", NS_LossyConvertUCS2toASCII(str).get()));
+                    }
+#endif                    
+
                     Instantiation newinst = *inst;
                     newinst.AddAssignment(mContentVariable, Value(content.get()));
 

content/xul/templates/src/nsContentTestNode.h

@@ -45,6 +45,7 @@
 #include "nsFixedSizeAllocator.h"
 #include "nsIAtom.h"
 
+class nsIXULTemplateBuilder;
 class nsIXULDocument;
 class nsConflictSet;
 
@@ -54,7 +55,7 @@ public:
     nsContentTestNode(InnerNode* aParent,
                       nsConflictSet& aConflictSet,
                       nsIXULDocument* aDocument,
-                      nsIContent* aRoot,
+                      nsIXULTemplateBuilder* aBuilder,
                       PRInt32 aContentVariable,
                       PRInt32 aIdVariable,
                       nsIAtom* aTag);
@@ -112,14 +113,11 @@ public:
 protected:
     nsConflictSet& mConflictSet;
     nsIXULDocument* mDocument; // [WEAK] because we know the document will outlive us
-    nsCOMPtr<nsIContent> mRoot;
+    nsIXULTemplateBuilder *mBuilder;
     PRInt32 mContentVariable;
     PRInt32 mIdVariable;
     nsCOMPtr<nsIAtom> mTag;
 };
 
-extern PRBool
-IsElementContainedBy(nsIContent* aElement, nsIContent* aContainer);
-
 #endif // nsContentTestNode_h__
 

content/xul/templates/src/nsXULContentBuilder.cpp

@@ -70,6 +70,7 @@
 #include "nsXULContentUtils.h"
 #include "nsXULElement.h"
 #include "nsXULTemplateBuilder.h"
+#include "nsSupportsArray.h"
 
 #include "jsapi.h"
 #include "pldhash.h"
@@ -82,6 +83,38 @@ static NS_DEFINE_CID(kTextNodeCID,               NS_TEXTNODE_CID);
 static NS_DEFINE_CID(kXMLElementFactoryCID,      NS_XML_ELEMENT_FACTORY_CID);
 static NS_DEFINE_CID(kXULSortServiceCID,         NS_XULSORTSERVICE_CID);
 
+PRBool
+IsElementInBuilder(nsIContent *aContent, nsIXULTemplateBuilder *aBuilder)
+{
+    // Make sure that the element is contained within the heirarchy
+    // that we're supposed to be processing.
+    nsCOMPtr<nsIDocument> doc;
+    aContent->GetDocument(*getter_AddRefs(doc));
+
+    nsCOMPtr<nsIXULDocument> xuldoc = do_QueryInterface(doc);
+    if (! xuldoc)
+        return PR_FALSE;
+
+    nsCOMPtr<nsIContent> content = dont_QueryInterface(aContent);
+    do {
+        nsCOMPtr<nsIXULTemplateBuilder> builder;
+        xuldoc->GetTemplateBuilderFor(content, getter_AddRefs(builder));
+        if (builder) {
+            if (builder == aBuilder)
+                return PR_TRUE; // aBuilder is the builder for this element.
+
+            // We found a builder, but it's not aBuilder.
+            break;
+        }
+
+        nsCOMPtr<nsIContent> parent;
+        content->GetParent(*getter_AddRefs(parent));
+        content = parent;
+    } while (content);
+
+    return PR_FALSE;
+}
+
 //----------------------------------------------------------------------
 //
 // Return values for EnsureElementHasGenericChild()
@@ -126,9 +159,6 @@ protected:
     nsresult Init();
 
     // Implementation methods
-    PRBool
-    IsElementInBuilder(nsIContent *aElement);
-
     nsresult
     OpenContainer(nsIContent* aElement);
 
@@ -930,6 +960,9 @@ nsXULContentBuilder::SynchronizeUsingTemplate(nsIContent* aTemplateNode,
     rv = aTemplateNode->GetAttrCount(numAttribs);
     if (NS_FAILED(rv)) return rv;
 
+    // XXXwaterson. Ugh, we just checked the failure code, above. Why
+    // do it again? This method needs a scrub-down, and could stand
+    // to have some of this bogo-error checking removed.
     if (rv == NS_CONTENT_ATTR_HAS_VALUE) {
         for (PRInt32 aLoop=0; aLoop<numAttribs; aLoop++) {
             PRInt32    attribNameSpaceID;
@@ -1011,7 +1044,6 @@ nsXULContentBuilder::SynchronizeUsingTemplate(nsIContent* aTemplateNode,
     return NS_OK;
 }
 
-
 PRBool
 nsXULContentBuilder::IsDirectlyContainedBy(nsIContent* aChild, nsIContent* aParent)
 {
@@ -1751,7 +1783,7 @@ nsXULContentBuilder::CreateContents(nsIContent* aElement)
     if (! aElement)
         return NS_ERROR_NULL_POINTER;
 
-    NS_ASSERTION(IsElementContainedBy(aElement, mRoot), "element not managed by this template builder");
+    NS_ASSERTION(IsElementInBuilder(aElement, this), "element not managed by this template builder");
 
     return CreateTemplateAndContainerContents(aElement, nsnull /* don't care */, nsnull /* don't care */);
 }
@@ -1907,26 +1939,13 @@ nsXULContentBuilder::SynchronizeMatch(nsTemplateMatch* match, const VariableSet&
     }
 #endif
 
-    Value parentValue;
-    match->mAssignments.GetAssignmentFor(mContentVar, &parentValue);
-
-    nsIContent* parent = VALUE_TO_ICONTENT(parentValue);
-
     // Now that we've got the resource of the member variable, we
     // should be able to update its kids appropriately
-    nsresult rv;
+    nsSupportsArray elements;
-    nsCOMPtr<nsISupportsArray> elements;
+    GetElementsForResource(resource, &elements);
-    rv = NS_NewISupportsArray(getter_AddRefs(elements));
-    NS_ASSERTION(NS_SUCCEEDED(rv), "unable to create new ISupportsArray");
-    if (NS_FAILED(rv)) return rv;
 
-    rv = GetElementsForResource(resource, elements);
+    PRUint32 cnt = 0;
-    NS_ASSERTION(NS_SUCCEEDED(rv), "unable to retrieve elements from resource");
+    elements.Count(&cnt);
-    if (NS_FAILED(rv)) return rv;
-
-    PRUint32 cnt;
-    rv = elements->Count(&cnt);
-    if (NS_FAILED(rv)) return rv;
 
 #ifdef PR_LOGGING
     if (PR_LOG_TEST(gXULTemplateLog, PR_LOG_DEBUG) && cnt == 0) {
@@ -1939,24 +1958,19 @@ nsXULContentBuilder::SynchronizeMatch(nsTemplateMatch* match, const VariableSet&
 #endif
 
     for (PRInt32 i = PRInt32(cnt) - 1; i >= 0; --i) {
-        nsISupports* isupports = elements->ElementAt(i);
+        nsCOMPtr<nsIContent> element = do_QueryElementAt(&elements, i);
-        nsCOMPtr<nsIContent> element( do_QueryInterface(isupports) );
+        if (! IsElementInBuilder(element, this))
-        NS_IF_RELEASE(isupports);
-
-        // If the element is contained by the parent of the rule
-        // that we've just matched, then it's the wrong element.
-        if (! IsElementContainedBy(element, parent))
             continue;
 
         nsCOMPtr<nsIContent> templateNode;
         mTemplateMap.GetTemplateFor(element, getter_AddRefs(templateNode));
 
+        NS_ASSERTION(templateNode, "couldn't find template node for element");
         if (! templateNode)
-            return NS_ERROR_UNEXPECTED;
+            continue;
 
         // this node was created by a XUL template, so update it accordingly
-        rv = SynchronizeUsingTemplate(templateNode, element, *match, modified);
+        SynchronizeUsingTemplate(templateNode, element, *match, modified);
-        if (NS_FAILED(rv)) return rv;
     }
         
 #ifdef PR_LOGGING
@@ -1977,43 +1991,11 @@ nsXULContentBuilder::SynchronizeMatch(nsTemplateMatch* match, const VariableSet&
 // Implementation methods
 //
 
-PRBool
-nsXULContentBuilder::IsElementInBuilder(nsIContent *aContent)
-{
-    // Make sure that the element is contained within the heirarchy
-    // that we're supposed to be processing.
-    nsCOMPtr<nsIDocument> doc;
-    aContent->GetDocument(*getter_AddRefs(doc));
-
-    nsCOMPtr<nsIXULDocument> xuldoc = do_QueryInterface(doc);
-    if (! xuldoc)
-        return PR_FALSE;
-
-    nsCOMPtr<nsIContent> content = dont_QueryInterface(aContent);
-    do {
-        nsCOMPtr<nsIXULTemplateBuilder> builder;
-        xuldoc->GetTemplateBuilderFor(content, getter_AddRefs(builder));
-        if (builder) {
-            if (builder == NS_STATIC_CAST(nsIXULTemplateBuilder *, this))
-                return PR_TRUE; // We're the builder for this element.
-
-            // We found a builder, but it's not us.
-            break;
-        }
-
-        nsCOMPtr<nsIContent> parent;
-        content->GetParent(*getter_AddRefs(parent));
-        content = parent;
-    } while (content);
-
-    return PR_FALSE;
-}
-
 nsresult
 nsXULContentBuilder::OpenContainer(nsIContent* aElement)
 {
     // See if we're responsible for this element
-    if (! IsElementInBuilder(aElement))
+    if (! IsElementInBuilder(aElement, this))
         return NS_OK;
 
     nsCOMPtr<nsIRDFResource> resource;
@@ -2055,7 +2037,7 @@ nsresult
 nsXULContentBuilder::CloseContainer(nsIContent* aElement)
 {
     // See if we're responsible for this element
-    if (! IsElementInBuilder(aElement))
+    if (! IsElementInBuilder(aElement, this))
         return NS_OK;
 
     nsCOMPtr<nsIAtom> tag;
@@ -2143,7 +2125,7 @@ nsXULContentBuilder::InitializeRuleNetworkForSimpleRules(InnerNode** aChildNode)
         new nsContentTestNode(mRules.GetRoot(),
                               mConflictSet,
                               xuldoc,
-                              mRoot,
+                              this,
                               mContentVar,
                               mContainerVar,
                               nsnull);
@@ -2263,7 +2245,7 @@ nsXULContentBuilder::CompileContentCondition(nsTemplateRule* aRule,
         new nsContentTestNode(aParentNode,
                               mConflictSet,
                               xuldoc,
-                              mRoot,
+                              this,
                               mContentVar, // XXX see above
                               urivar,
                               tag);

content/xul/templates/src/nsXULTemplateBuilder.cpp

@@ -469,9 +469,8 @@ nsXULTemplateBuilder::Propagate(nsIRDFResource* aSource,
             nsRDFTestNode* rdftestnode = NS_STATIC_CAST(nsRDFTestNode*, *i);
 
             Instantiation seed;
-            if (rdftestnode->CanPropagate(aSource, aProperty, aTarget, seed)) {
+            if (rdftestnode->CanPropagate(aSource, aProperty, aTarget, seed))
                 livenodes.Add(rdftestnode);
-            }
         }
     }
 
@@ -573,23 +572,15 @@ nsXULTemplateBuilder::OnAssert(nsIRDFDataSource* aDataSource,
     if (IsActivated(aSource))
         return NS_OK;
 
-    nsresult rv;
-
 	if (mCache)
         mCache->Assert(aSource, aProperty, aTarget, PR_TRUE /* XXX should be value passed in */);
 
     LOG("onassert", aSource, aProperty, aTarget);
 
     nsClusterKeySet newkeys;
-    rv = Propagate(aSource, aProperty, aTarget, newkeys);
+    Propagate(aSource, aProperty, aTarget, newkeys);
-    if (NS_FAILED(rv)) return rv;
+    FireNewlyMatchedRules(newkeys);
-
+    SynchronizeAll(aSource, aProperty, nsnull, aTarget);
-    rv = FireNewlyMatchedRules(newkeys);
-    if (NS_FAILED(rv)) return rv;
-
-    rv = SynchronizeAll(aSource, aProperty, nsnull, aTarget);
-    if (NS_FAILED(rv)) return rv;
-
     return NS_OK;
 }
 
@@ -646,19 +637,13 @@ nsXULTemplateBuilder::OnUnassert(nsIRDFDataSource* aDataSource,
     if (IsActivated(aSource))
         return NS_OK;
 
-    nsresult rv;
-
 	if (mCache)
 		mCache->Unassert(aSource, aProperty, aTarget);
 
     LOG("onunassert", aSource, aProperty, aTarget);
 
-    rv = Retract(aSource, aProperty, aTarget);
+    Retract(aSource, aProperty, aTarget);
-    if (NS_FAILED(rv)) return rv;
+    SynchronizeAll(aSource, aProperty, aTarget, nsnull);
-
-    rv = SynchronizeAll(aSource, aProperty, aTarget, nsnull);
-    if (NS_FAILED(rv)) return rv;
-    
     return NS_OK;
 }
 
@@ -688,30 +673,22 @@ nsXULTemplateBuilder::OnChange(nsIRDFDataSource* aDataSource,
 			mCache->Assert(aSource, aProperty, aNewTarget, PR_TRUE);
 	}
 
-    nsresult rv;
-
     LOG("onchange", aSource, aProperty, aNewTarget);
 
     if (aOldTarget) {
         // Pull any old rules that were relying on aOldTarget
-        rv = Retract(aSource, aProperty, aOldTarget);
+        Retract(aSource, aProperty, aOldTarget);
-        if (NS_FAILED(rv)) return rv;
     }
 
     if (aNewTarget) {
         // Fire any new rules that are activated by aNewTarget
         nsClusterKeySet newkeys;
-        rv = Propagate(aSource, aProperty, aNewTarget, newkeys);
+        Propagate(aSource, aProperty, aNewTarget, newkeys);
-        if (NS_FAILED(rv)) return rv;
+        FireNewlyMatchedRules(newkeys);
-
-        rv = FireNewlyMatchedRules(newkeys);
-        if (NS_FAILED(rv)) return rv;
     }
 
     // Synchronize any of the content model that may have changed.
-    rv = SynchronizeAll(aSource, aProperty, aOldTarget, aNewTarget);
+    SynchronizeAll(aSource, aProperty, aOldTarget, aNewTarget);
-    if (NS_FAILED(rv)) return rv;
-
     return NS_OK;
 }