Shutting down Thunderbird, I see the following warnings (which are only currently emitted during the shutdown collection, because it's the only time we do multiple collections in succession): nsCycleCollector: a later shutdown collection collected the additional suspect 0xdc7960 nsGenericElement (which could be fixed by improving traversal) [repeated for 13 nsGenericElement and 1 nsJSContext] This means that the cycle collector collected some garbage, and the releases propagating from that garbage allowed more garbage to be freed. This is a problem because: * if the traversal methods indicated that this garbage was reachable, we would have freed it the collection before and used less memory during the interval between collections * if there were an owning reference from one of these elements back to the first set of garbage that was freed, we would have leaked Note that this particular warning may go away without this bug being fixed, if Thunderbird leaks are cleaned up such that this stuff is freed earlier (i.e., before the shutdown collections). I took XPCOM_MEM_REFCNT_LOGs for all of the nsGenericElements above, and for all 13 of them, there were 2 Releases that occurred within ::Unroot -- the last was the last release, and the other was presumably the collection before, and always involved the following stack (with varying numbers of elements at the top): NS_LogRelease_P (/home/dbaron/builds/trunk-cc/mozilla/xpcom/base/nsTraceRefcntImpl.cpp:1070) nsGenericElement::Release() (/home/dbaron/builds/trunk-cc/mozilla/content/base/src/nsGenericElement.cpp:3374) nsXULElement::Release() (/home/dbaron/builds/trunk-cc/mozilla/content/xul/content/src/nsXULElement.cpp:364) nsAttrAndChildArray::Clear() (/home/dbaron/builds/trunk-cc/mozilla/content/base/src/nsAttrAndChildArray.cpp:651) ~nsAttrAndChildArray (/home/dbaron/builds/trunk-cc/mozilla/content/base/src/nsAttrAndChildArray.cpp:135) ~nsGenericElement (/home/dbaron/builds/trunk-cc/mozilla/content/base/src/nsGenericElement.cpp:1096) ~nsXULElement (/home/dbaron/builds/trunk-cc/mozilla/content/xul/content/src/nsXULElement.h:438) nsNodeUtils::LastRelease(nsINode*) (/home/dbaron/builds/trunk-cc/mozilla/content/base/src/nsNodeUtils.cpp:228) nsGenericElement::Release() (/home/dbaron/builds/trunk-cc/mozilla/content/base/src/nsGenericElement.cpp:3374) nsXULElement::Release() (/home/dbaron/builds/trunk-cc/mozilla/content/xul/content/src/nsXULElement.cpp:364) nsAttrAndChildArray::Clear() (/home/dbaron/builds/trunk-cc/mozilla/content/base/src/nsAttrAndChildArray.cpp:651) ~nsAttrAndChildArray (/home/dbaron/builds/trunk-cc/mozilla/content/base/src/nsAttrAndChildArray.cpp:135) ~nsGenericElement (/home/dbaron/builds/trunk-cc/mozilla/content/base/src/nsGenericElement.cpp:1096) ~nsXULElement (/home/dbaron/builds/trunk-cc/mozilla/content/xul/content/src/nsXULElement.h:438) nsNodeUtils::LastRelease(nsINode*) (/home/dbaron/builds/trunk-cc/mozilla/content/base/src/nsNodeUtils.cpp:228) nsGenericElement::Release() (/home/dbaron/builds/trunk-cc/mozilla/content/base/src/nsGenericElement.cpp:3374) nsXULElement::Release() (/home/dbaron/builds/trunk-cc/mozilla/content/xul/content/src/nsXULElement.cpp:364) ~nsCOMPtr (/home/dbaron/builds/trunk-cc/obj/thunderbird-debug/content/xul/content/src/../../../../dist/include/xpcom/nsCOMPtr.h:583) ~nsTemplateQuerySet (/home/dbaron/builds/trunk-cc/mozilla/content/xul/templates/src/nsTemplateRule.h:312) nsXULTemplateBuilder::Uninit(int) (/home/dbaron/builds/trunk-cc/mozilla/content/xul/templates/src/nsXULTemplateBuilder.cpp:215) nsXULContentBuilder::Uninit(int) (/home/dbaron/builds/trunk-cc/mozilla/content/xul/templates/src/nsXULContentBuilder.cpp:495) nsXULTemplateBuilder::NodeWillBeDestroyed(nsINode const*) (/home/dbaron/builds/trunk-cc/mozilla/content/xul/templates/src/nsXULTemplateBuilder.cpp:1111) nsXULContentBuilder::NodeWillBeDestroyed(nsINode const*) (/home/dbaron/builds/trunk-cc/mozilla/content/xul/templates/src/nsXULContentBuilder.cpp:1808) nsBindingManager::NodeWillBeDestroyed(nsINode const*) (/home/dbaron/builds/trunk-cc/mozilla/content/xbl/src/nsBindingManager.cpp:1365) nsNodeUtils::LastRelease(nsINode*) (/home/dbaron/builds/trunk-cc/mozilla/content/base/src/nsNodeUtils.cpp:182) nsDocument::Release() (/home/dbaron/builds/trunk-cc/mozilla/content/base/src/nsDocument.cpp:927) nsXMLDocument::Release() (/home/dbaron/builds/trunk-cc/mozilla/content/xml/document/src/nsXMLDocument.cpp:210) nsXULDocument::Release() (/home/dbaron/builds/trunk-cc/mozilla/content/xul/document/src/nsXULDocument.cpp:362) nsXPCOMCycleCollectionParticipant::Unroot(void*) (/home/dbaron/builds/trunk-cc/mozilla/xpcom/base/nsCycleCollectionParticipant.cpp:59) nsCycleCollector::CollectWhite(GCGraph&) (/home/dbaron/builds/trunk-cc/mozilla/xpcom/base/nsCycleCollector.cpp:1501) nsCycleCollector::Collect(unsigned int) (/home/dbaron/builds/trunk-cc/mozilla/xpcom/base/nsCycleCollector.cpp:2162) nsCycleCollector::Shutdown() (/home/dbaron/builds/trunk-cc/mozilla/xpcom/base/nsCycleCollector.cpp:2217) nsCycleCollector_shutdown() (/home/dbaron/builds/trunk-cc/mozilla/xpcom/base/nsCycleCollector.cpp:2605) NS_ShutdownXPCOM_P (/home/dbaron/builds/trunk-cc/mozilla/xpcom/build/nsXPComInit.cpp:780) ~ScopedXPCOMStartup (/home/dbaron/builds/trunk-cc/mozilla/toolkit/xre/nsAppRunner.cpp:809) XRE_main (/home/dbaron/builds/trunk-cc/mozilla/toolkit/xre/nsAppRunner.cpp:2930) main (/home/dbaron/builds/trunk-cc/mozilla/mail/app/nsMailApp.cpp:87) I think what this is showing is that we're not telling the cycle collector about either of: * the ownership relationship expressed by what the nsXULTemplateBuilder::NodeWillBeDestroyed function does (which may be rather hard to express), or * the ownership relationship of the binding manager owning its document observers I'm not sure exactly what we need to do here, but I think it would be good to traverse at least one of these relationships, if not both.

This may also be causing the sequence from closing a firefox window to collecting the chrome window for it to require two cycle collections rather than one, some of the time, but I couldn't reproduce the problem reliably enough to debug it.

I'm thinking this may not be a real problem. It looks like members of mObservers are actually weak pointers (which surprises me). So really it's just that there are relationships that are broken more often than the cycle collector knows about (and more often than necessary), which I think shouldn't be a problem.

So should we mark this INVALID or what?