387491 - need to traverse to nsXULContentBuilder better

Status: NEW

(Core :: XUL, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So should we mark this INVALID or what?