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.

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.

Peterv, don't hesitate to land for M9.

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.

This'll also fix bug 398410

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

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.

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

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.

Fix checked in. Marking bug FIXED.