378173 - Template removes content incorrectly when multiple queries are used

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

Attachment: testcase

In this test, two <rule>s are used which both generate a list of animals. However, both the 'Tarantula' and 'Chameleon' animals are generated from both rules, so only the first rule should apply.

When the 'Change Rule 2' button is pressed, the RDF data is changed such that 'Chameleon' now only applies to rule 1. (The RDF data is changed by removing the chameleon from the list). Currently, this will cause the content for 'Chameleon' to disappear.

However, the first rule still has a 'Chameleon' so it should still apply. What's happening is that the template updating code isn't checking to make sure that the removed result actually created some content and is removing it regardless of which <rule> is actually active.

The datasource is at http://www.xulplanet.com/ndeakin/tests/xts/templates/animals.rdf

Comment 1

[Neil Deakin]

Attachment: make sure that the item is removed properly

This patch was originally added to bug 330776.

In addition:
 - added lots more comments to UpdateResultInContainer
 - also make sure that HasBeenRemoved is called before an item is removed

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 262250 [details] [diff] [review]
make sure that the item is removed properly

>         }
>         else if (!mMatchMap.Put(resultid, newmatch)) {
>+            newmatch->mResult->HasBeenRemoved();
>             nsTemplateMatch::Destroy(mPool, newmatch);

Could nsTemplateMatch::Destroy always call aMatch->mResult->HasBeenRemoved()?
Or if not always, then ::Destroy should perhaps have a parameter to indicate
whether or not to call mResult->HasBeenRemoved().
And could ::Destroy automatically also set the aMatch to point null, so that after
calling ::Destroy above, newmatch would be null. I don't like keeping
pointers to deleted objects around...


>+    // This method takes a result that no longer applies (aOldResult) and
>+    // replaces it with a new result (aNewResult). Either may be null
>+    // indicating to just remove a result or add a new one without replacing.
>+    //
>+    // Matches are stored in the hashtable mMatchMap, keyed by result id. If
>+    // there is more than one query, or the same id is found in different
>+    // containers, the values in the hashtable will be a linked list of all
>+    // the matches for that id. The matches are sorted according to the
>+    // queries they are associated with. Matches for earlier queries in the
>+    // template have an earlier priority. 

"earlier priority"?  Does that mean higher priority? "earlier priority" is used also
elsewhere.

            }
> 
>+                    // acceptedmatch may have been set in the block handling
>+                    // aOldResult earlier. If so, we would only get here when
>+                    // that match has a lower priority than this new match.
>+                    // As only one match can have content generated for it, it
>+                    // is OK to set acceptedmatch here to the new match,
>+                    // ignoring the other one.

But here you use words "lower priority". I think using higher/lower everywhere would be
better. Or am I missing something, does higher/lower priority mean something else than
earlier/later priority?
 
>+                    // clear the matched state of the later results for the
>+                    // same container

Nit, sentences should (usually) start with a capital letter and end with a full stop.
Same with few other comments too.

Btw, great comments.

Comment 3

[Neil Deakin]

Attachment: update for comments

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 262523 [details] [diff] [review]
update for comments


>-    Destroy(nsFixedSizeAllocator& aPool, nsTemplateMatch* aMatch) {
>+    Destroy(nsFixedSizeAllocator& aPool, nsTemplateMatch*& aMatch, PRBool aRemoveResult) {
>+        if (aRemoveResult && aMatch->mResult)
>+          aMatch->mResult->HasBeenRemoved();
>         aMatch->~nsTemplateMatch();
>-        aPool.Free(aMatch, sizeof(*aMatch)); }
>+        aPool.Free(aMatch, sizeof(*aMatch));
>+        aMatch = nsnull;
>+    }

You might save some space if de-inlining Destroy().

>+    // The first query has a priority 0, and higher numbers are for later
>+    // queries with successively higher priorities. Thus, a match takes
>+    // precedence if it has a lower priority than another. If there is only
>+    // one query or container, then the match doesn't have any linked items.

Sort of reversed meaning of lower/higher priority, but the comment makes it now
clear what is what. So ok to me.


r=me with de-inlining.

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 262523 [details] [diff] [review]
update for comments

>Index: content/xul/templates/src/nsTemplateMatch.h
>===================================================================

>+    Destroy(nsFixedSizeAllocator& aPool, nsTemplateMatch*& aMatch, PRBool aRemoveResult) {

Long line

>Index: content/xul/templates/src/nsXULTemplateBuilder.cpp
>===================================================================

>@@ -578,43 +575,132 @@ nsXULTemplateBuilder::UpdateResult(nsIXU

>+    // Naturally, if a match with an lower priority is active, it overrides

s/an/a/

>+    // Indicates that the old match was active and must have its content removed

Long line.

>+                    // Indiciate that the old match was active so its content

Indicate

>+                    // If a match with an lower priority is active, the new

s/an/a/

>+                // indiciate that it needs its content removed.

indicate

>+                            // Replacedmatch should be null here. If not, it means
>+                            // that two matches were active which isn't a valid state
>+                            NS_ASSERTION(!replacedmatch, "replaced match already set");

Long lines.

>+                                nsTemplateMatch::Destroy(mPool, newmatch, PR_FALSE);

Long line.

>+                                    nsTemplateMatch::Destroy(mPool, newmatch, PR_FALSE);

Long line.

>+        rv = ReplaceMatch(replacedmatch->mResult, nsnull, nsnull, aInsertionPoint);

Long line.

Comment 6

[Neil Deakin]

I think there's an automated test for this in bug 378893, but I'll need to check