fyi, this was found as part of the testing described at http://www.wykiwyk.com/mozilla/leakTesting/testlog_20070921_summary.html.

This is a somewhat cleaned up version of http://s1.grono.net/js/login.8.js. It looks like this script leaks by declaring event listeners that close over XPath results, but it's hard to tell because it looks like the script calls XHR recursively and the site happens to be down for maintenance at the moment. If the Polish-English translation I got is correct, it should be back in an hour or so.

This fixes the leak for me, but maybe there are other XPath classes that should participate.

Comment on attachment 282210 [details] [diff] [review] have nsXPathResult participate in cycle collection >Index: content/xslt/src/xpath/nsXPathResult.cpp >=================================================================== >+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsXPathResult) >+ NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mDocument) This should call RemoveMutationObserver if mDocument is non-null. I think it should also call releaseNodeSet. >+NS_IMPL_CYCLE_COLLECTION_UNLINK_END >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsXPathResult) >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mDocument) >+ txNodeSet *nodeSet = static_cast<txNodeSet*>(tmp->mResult.get()); >+ if (nodeSet && nodeSet->getResultType() == txAExprResult::NODESET && nodeSet->size() > 0) { I'd prefer if you checked for getResultType() == txAExprResult::NODESET before casting to txNodeSet. And put the code in a block to avoid any conflicts with variables declared in the BEGIN macro. { txAResultResult *result = tmp->mResult.get(); if (result && result->getResultType() == txAExprResult::NODESET) txNodeSet *nodeSet = static_cast<txNodeSet*>(tmp->mResult.get()); ... } Looks good otherwise. >+ nsCOMPtr<nsIDOMNode> node; >+ PRInt32 count = nodeSet->size(); >+ PRInt32 i; >+ for (i = 0; i < count; ++i) { >+ txXPathNativeNode::getNode(nodeSet->get(i), getter_AddRefs(node)); >+ cb.NoteXPCOMChild(node); Hmm, maybe we should add an API to get the refcounted node without addrefing.

Add more XPath classes to cycle collection. I think this catches the remaining ones.

I thought that refcounting inside Traverse was generally a no-no. Am I mistaken?

I think it should mostly be fine (if not done on the object that's being traced).

Comment on attachment 282266 [details] [diff] [review] nsXPathExpression/nsXPathNSResolver v1 Fix leaks by making objects participate in cycle collection.

> I think it should also call releaseNodeSet. This causes a crash or abort.

Comment on attachment 282287 [details] [diff] [review] address peter's comments Not calling releaseNodeSet is fine, I'll try to figure out why it crashes.

This also improved our collection at shutdown. Previously, we were holding onto 4 DOM windows until the JS-implemented XPCOM services were released. The microsummary service uses XPath, so perhaps there was a cycle there, too.