402208 - XPathResult holding attribute node causes cycle collector fault

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Peter Van der Beken [:peterv]]

Seeing this with the ForecastFox extension. The XPathResult's cycle collector participant traverses the nodes it holds, but we didn't addref all of them. In particular, for attribute nodes we addref their ownerElement. This causes faults in the cycle collector which makes us leak a lot.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

This seems to fix it, but sucks a bit if people reuse XPathResults as variables/parameters with XSLT. The other solution would be to teach the tree walker about nsIAttribute, that would also allow use of XPath on disconnected attribute nodes.

Comment 2

[Damon Sicore (:damons)]

Peterv, don't hesitate to land for M9.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 287113 [details] [diff] [review]
v1

>Index: content/xslt/src/xpath/nsXPathResult.cpp
>@@ -222,12 +221,12 @@ nsXPathResult::SnapshotItem(PRUint32 aIn
>         return NS_ERROR_DOM_TYPE_ERR;
>     }
> 
>-    txNodeSet *nodeSet = static_cast<txNodeSet*>(mResult.get());
>-    if (aIndex < (PRUint32)nodeSet->size()) {
>-        return txXPathNativeNode::getNode(nodeSet->get(aIndex), aResult);
>+    if (mCurrentPos < (PRUint32)mResultNodes.Count()) {
>+        NS_ADDREF(*aResult = mResultNodes[mCurrentPos++]);
>+    }
>+    else {
>+        *aResult = nsnull;
>     }

this looks wrong, you should use aIndex here, not mCurrentPos. You can also simply do:

NS_IF_ADDREF(*aResult = mResultNodes->SafeObjectAt(aIndex));

>@@ -378,12 +390,31 @@ nsXPathResult::GetExprResult(txAExprResu
>         return NS_ERROR_DOM_INVALID_STATE_ERR;
>     }
> 
>-    *aExprResult = mResult.get();
>-    if (!*aExprResult) {
>+    if (mResult) {
>+        NS_ADDREF(*aExprResult = mResult);
>+
>+        return NS_OK;
>+    }
>+
>+    if (mResultNodes.Count() == 0) {
>         return NS_ERROR_DOM_INVALID_STATE_ERR;
>     }

Why fail for this? Couldn't the resultset simply be empty?

> 
>-    NS_ADDREF(*aExprResult);
>+    nsRefPtr<txNodeSet> nodeSet = new txNodeSet(nsnull);
>+    if (!nodeSet) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+
>+    PRUint32 i, count = mResultNodes.Count();
>+    for (i = 0; i < count; ++i) {
>+        nsAutoPtr<txXPathNode> node;
>+        node = txXPathNativeNode::createXPathNode(mResultNodes[i]);

Isn't this slightly more efficient as 

nsAutoPtr<txXPathNode> node(txXPathNativeNode::createXPathNode(mResultNodes[i]));

>+        if (node) {
>+            nodeSet->append(*node);
>+        }

Should you rather 'throw' if node is null since that only happens on OOM and when we can't properly build a result-set due to an orphaned attribute.

r/sr=me with that fixed.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This'll also fix bug 398410

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: Alternate approach v1

Hrmpf, I had actually started working on this instead. Let me know if you prefer the other one.

Comment 6

[Johnny Stenback (:jst)]

Attachment: Sickings recommended changes.

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 287172 [details] [diff] [review]
Sickings recommended changes.

>diff --git a/content/xslt/src/xpath/nsXPathResult.cpp b/content/xslt/src/xpath/nsXPathResult.cpp

>@@ -396,10 +391,6 @@ nsXPathResult::GetExprResult(txAExprResult** aExprResult)
>         return NS_OK;
>     }
> 
>-    if (mResultNodes.Count() == 0) {
>-        return NS_ERROR_DOM_INVALID_STATE_ERR;
>-    }
>-

This should stay actually, it does mean the XPathResult wasn't initialized. If it was initialized to an empty nodeset we'll have an empty txNodeSet in mResult and we'd have returned early. Other than that looks good.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I prefer the first patch, so lets that get that landed. I'll do it tomorrow if no-one beats me to it.

Comment 9

[Peter Van der Beken [:peterv]]

Jst will land it tonight (thanks!). Note that I do think we eventually want the alternate approach, so I guess I'll file a different bug for that.

Comment 10

[Johnny Stenback (:jst)]

Fix checked in. Marking bug FIXED.

Comment 11

[Johnny Stenback (:jst)]

Attachment: Patch that was checked in.