498321 - test_nntpPassword.js, test_nntpPassword2.js and test_server.js all leak (including RDF data sources)

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[Mark Banner (:standard8)]

Attachment: Typical leak log

After part 2 of bug 438922 has landed to clean up the leak logs, test_nntpPassword.js, test_nntpPassword2.js and test_server.js all still leak. Typical log attached.

The main highlights are the rdf service and data source. Could be caused by bug 387709 but I'm not convinced at the moment.

Comment 1

[Joshua Cranmer [:jcranmer]]

The common link between the three tests should be news:*--which is apparently related to subscribe.

The subscribe data source shouldn't be coming into play here, but that would be my best bet for the root leak problem. The only other suspect reference to my eyes would be nsSubscribableServer, which most definitely holds up the NNTP server.

Comment 2

[Mark Banner (:standard8)]

Subscribe data source could be an issue - we've got bug 387709 open on hooking it up to cycle collection. I did try hooking it up, but maybe I didn't do it right. One issue is that its addref & release are currently threadsafe and rdf data sources with cycle collection don't support that...

Comment 3

[Joshua Cranmer [:jcranmer]]

I've done some more testing, and I found this cycle in references: nsNntpIncomingServer strong-refs nsSubscribableServer, which strong-refs the server. If I change the latter to a regular nsIMsgIncomingServer*, that drops the reference and should fix the leaks (I still have residual leaks in my environment, so I can't tell for certain).

That's probably the best place to break the link. Unfortunately, we're left with the annoyance that the news server will keep holding the nsSubscribableServer open, but that's life (when I get around to stripping out the RDF from subscribe, that won't be a problem thanks to some dependency inversion).

Comment 4

[Mark Banner (:standard8)]

I've just tried your suggestion here. It would fix the leaks on test_nntpPassword*.js (or at least make them the same as covered by bug 498307), but not the ones on test_server.js.

I guess this could affect IMAP as well (as it uses nSubscribableServer), but it doesn't affect any of the current tests afaict.

Comment 5

[Joshua Cranmer [:jcranmer]]

It's a start, I guess, and probably one that should be fixed anyways. I'll have to look at what else leaks in test_server.js when I find time...

Comment 6

[Joshua Cranmer [:jcranmer]]

(In reply to comment #4)
> I've just tried your suggestion here. It would fix the leaks on
> test_nntpPassword*.js (or at least make them the same as covered by bug
> 498307), but not the ones on test_server.js.

The test_server.js leaks seems to be originating from JS code; the nsCOMPtrs aren't leaking, and the only other non-balanced leak is XPCWrappedNative creating 4 and destroying 3.

Of the leaking classes, most of them can be discounted as being held by nsNntpIncomingServer; that leaves as suspects the nsNntpIncomingServer and the XPC wrappers. The only thing I recall test_server doing differently would be the protocol tests.

The leaking XPCWrappedJS comes from the listener we're using, I'm guessing. The JS closures would probably be holding the url and server open, accounting for two of three leaking XPCWrappedNatives and probably the XPCWrappedNative that is leaking the nsNntpIncomingServer. More to come...

Comment 7

[Joshua Cranmer [:jcranmer]]

The nsNntpMockChannel appears to be ultimately be the cause of the leaks--it appears that the tests actually fail to run the mock channel, although it successfully starts it. Since the channel isn't run, it is held by the server, which is held in a closure by the listener of the channel.

If I make CloseCachedConnections() cancel the channels, that successfully kills the leak. I am currently also debugging the issue as to why the channel isn't being run.

Comment 8

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090530
SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/e44d9c0f4805
 +http://hg.mozilla.org/comm-central/rev/62f2c362a9a4 + bug 493008 patches)

Ftr, this build already had this bug.

Comment 9

[Joshua Cranmer [:jcranmer]]

And this was fixed by patch 2 of bug 226890, specifically <http://hg.mozilla.org/comm-central/diff/78440aa35ff6/mailnews/news/test/unit/tail_news.js>, if memory serves me correctly.