394514 - need to better express ownership of nsXULTemplateQueryProcessorRDF to cycle collector

Status: NEW

(Core :: XUL, defect, P4)

Description

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

This is pretty closely related to bug 387491, but I didn't want to add it as a comment there since I'm still too confused to be sure they're the same.  Neil, I'd appreciate if you could take both bugs, though.

Steps to reproduce:

  1. start firefox (DEBUG).  Keep an eye on the count of DOMWINDOWs printed to the console.

  2. Repeat the following a bunch of times:
    a. Go to Tools -> Addons
    b. Close the addons window

Actual results:  The DOMWINDOW number generally doesn't decrease when closing the addons window.  But opening and closing new tabs multiple times can get the count to go back down again.

Expected results: The cycle collection that occurs a few seconds after closing the addons window restores the DOMWINDOW count to what it was before opening the addons window.

When I build with DEBUG_CC, I see things like the following:
nsCycleCollector: nsWindowDataSource 0x24e4fad0 was not collected due to 4
  external references (9 total - 5 known)
  An object expected to be garbage could be reached from it by the path:
    nsWindowDataSource 0x24e4fad0
    InMemoryDataSource 0x24e4fdfc
    CompositeDataSourceImpl 0x2d31f9d0
    nsXULTemplateQueryProcessorRDF 0x2b5d9bd0
    nsGenericElement 0x2cbbe420
    nsGenericElement 0x2cbbe390
    nsGenericElement 0x2cbbe300
    nsEventListenerManager 0x2cbbe240
    nsJSEventListener 0x2cbbe180
    nsJSContext 0x2d322ef0
(and presumably the "locked object" keeping the nsGlobalWindow alive is the js_LockGCThing from such nsJSEventListener objects)

Then, at the next cycle collection, I see the following two releases on that nsXULTemplateQueryProcessorRDF:

#0  nsXULTemplateQueryProcessorRDF::Release (this=0x2b5d9bd0) at ../../../../dist/include/xpcom/nsISupportsImpl.h:188
#1  0x0200cd44 in nsCOMArray_base::RemoveObject (this=0x2b5d9bd0, aObject=0x2b5d9bd4) at nsCOMArray.cpp:129
#2  0x2193be2c in CompositeDataSourceImpl::RemoveObserver (this=0x2b5d9bd0, aObserver=0x0) at ../../../dist/include/xpcom/nsCOMArray.h:258
#3  0x06245660 in nsXULTemplateQueryProcessorRDF::Done (this=0x2b5d9bd0) at /builds/dbaron/trunk-cc/mozilla/content/xul/templates/src/nsXULTemplateQueryProcessorRDF.cpp:381
#4  0x062393e4 in nsXULTemplateBuilder::NodeWillBeDestroyed (this=0x2cb6d710, aNode=0x0) at /builds/dbaron/trunk-cc/mozilla/content/xul/templates/src/nsXULTemplateBuilder.cpp:1108
#5  0x060a3ba0 in nsBindingManager::NodeWillBeDestroyed (this=0x2b5d9bd0, aNode=0x35b1000) at /builds/dbaron/trunk-cc/mozilla/content/xbl/src/nsBindingManager.cpp:1373
#6  0x05edbb48 in nsNodeUtils::LastRelease (aNode=0x35b1000) at /builds/dbaron/trunk-cc/mozilla/content/base/src/nsNodeUtils.cpp:183
#7  0x05e83598 in nsDocument::Release (this=0x35b1000) at /builds/dbaron/trunk-cc/mozilla/content/base/src/nsDocument.cpp:929
#8  0x0600768c in nsXMLDocument::Release (this=0x35b1000) at /builds/dbaron/trunk-cc/mozilla/content/xml/document/src/nsXMLDocument.cpp:210
#9  0x060af068 in nsXULDocument::Release (this=0x35b1000) at /builds/dbaron/trunk-cc/mozilla/content/xul/document/src/nsXULDocument.cpp:362
#10 0x02015664 in nsXPCOMCycleCollectionParticipant::Unroot (this=0x2b5d9bd0, p=0x0) at nsCycleCollectionParticipant.cpp:59
#11 0x0208fff8 in nsCycleCollector::CollectWhite (this=0xd5000, graph=@0x28119944) at /builds/dbaron/trunk-cc/mozilla/xpcom/base/nsCycleCollector.cpp:1494
#12 0x02091450 in nsCycleCollector::Collect (this=0xd5000, aTryCollections=1) at /builds/dbaron/trunk-cc/mozilla/xpcom/base/nsCycleCollector.cpp:2155


#0  nsXULTemplateQueryProcessorRDF::Release (this=0x2b5d9bd0) at ../../../../dist/include/xpcom/nsISupportsImpl.h:188
#1  0x0623e404 in nsXULTemplateBuilder::~nsXULTemplateBuilder (this=0x2cb6d710) at ../../../../dist/include/xpcom/nsCOMPtr.h:583
#2  0x064e2e50 in nsXULContentBuilder::~nsXULContentBuilder (this=0x2cb6d710) at /builds/dbaron/trunk-cc/mozilla/content/xul/templates/src/nsXULContentBuilder.cpp:128
#3  0x06236c60 in nsXULTemplateBuilder::Release (this=0x2cb6d710) at /builds/dbaron/trunk-cc/mozilla/content/xul/templates/src/nsXULTemplateBuilder.cpp:282
#4  0x02015664 in nsXPCOMCycleCollectionParticipant::Unroot (this=0x2b5d9bd0, p=0x2b5d9bd0) at nsCycleCollectionParticipant.cpp:59
#5  0x0208fff8 in nsCycleCollector::CollectWhite (this=0xd5000, graph=@0x28132f94) at /builds/dbaron/trunk-cc/mozilla/xpcom/base/nsCycleCollector.cpp:1494

The second of these is something that the cycle collector would have understood, but the first of them is not.  The fact that the cycle collector doesn't understand the ownership model in the first release is preventing it from collecting these objects sooner.  I think it could also potentially cause permanent leaks if JS were doing different things.

(Since I've been recording my leak-debugging sessions since people have expressed interest in those recordings as a form of documentation, you can see screencast + audio of this debugging session in sessions 4 and 5 at http://people.mozilla.com/~dbaron/leak-screencasts/ )


Some possible solutions to fix this:

  * nsXULTemplateQueryProcessorRDF could own fewer objects or be destroyed more agressively.  (It seems like the things it owns are just caching.)

  * The observers on the composite data source could be weak pointers.  (That's the pointer that the cycle collector doesn't understand; it thinks the in memory data source owns the observer, whereas other objects going away causes that reference to be released.)

  * We could somehow change the cycle collection algorithm so it accounts for these ownership models where multiple objects must be kept alive to keep a reference alive.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Make them weak observers

This is the second option that dbaron proposed.Seems to work just fine for me.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 302632 [details] [diff] [review]
Make them weak observers

Neil, any thoughts on this?

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, and so this bug is keeping DOM windows alive much longer than necessary. If we don't want to go with this approach then jst and I found a super-hacky solution to get the DOM windows freed another way.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 302632 [details] [diff] [review]
Make them weak observers

Nevermind, talked with jst and sicking about this and we're just going to go for a super hack rather than risk breaking somebody with this type of change. See bug 416939.

Comment 5

[Neil Deakin]

Ben, so is this bug fixed, or is there plans to fix it a different way?

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

No, this isn't really fixed. We manually break the cycle right now (bug 416939).

Comment 7

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

I guess this isn't blocking since we did a super hack.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

-> default asignee