102229 - DNS Service and DNS Lookups leak at shutdown

Status: RESOLVED WORKSFORME

(Core :: Networking, defect, P3)

Description

[David :Bienvenu]

I apologize in advance if you've already got a bug about this, but I couldn't
find one. This is not a real leak per se, but it shows up in the leak logs
because the DNS Service doesn't shut down properly, at least on windows. I have
a fix for this on windows, but I have no idea if it works on Linux or the Mac.
Basically, I registered a shutdown observer and on shutdown, I post a message to
the dns window which causes the thread to exit and everything unwinds gracefully
(on Windows). I also fixed a problem where the dns server was registering itself
as an nsIObserver, but not implementing nsIObserver in its QI, so it's doubtful
it was getting notifications on pref changes. I'll attach a patch shortly. Since
I'm not knowledgeable about this code, and it's delicate, I'm hoping one of you
can take this patch and run with it.

Comment 1

[David :Bienvenu]

Attachment: rough fix

Comment 2

[Darin Fisher]

i don't believe there is another bug on this... gordon?

Comment 3

[gordon]

Not that I'm aware of. Targeting for 0.9.6.

Comment 4

[gordon]

I need to clean the patch up a bit. Targeting for necko stability milestone 0.9.8.

Comment 5

[gordon]

Adding nsIObserver to QI has already been landed.

Darin, it seems odd that the DNS service should have to register for shutdown
notification separately from Necko.  If the DNS service isn't getting shutdown
properly, then my guess is that nsIOService isn't getting shutdown either, or
socket transport for that matter.

Comment 6

[gordon]

*** Bug 126455 has been marked as a duplicate of this bug. ***

Comment 7

[[e-mail] (gone - use [e-mail] instead)]

*** Bug 126364 has been marked as a duplicate of this bug. ***

Comment 8

[Cathleen]

please note this is the root of leak regression on tinderbox.  bug 126455 is marked
as a dup of this bug.  Will need to get this addressed.  I'd nominate for a +

Comment 9

[Gagan]

per adt, critical for nsbeta1. hence plus.

Comment 10

[Alec Flett]

Comment on attachment 51279 [details] [diff] [review]
rough fix

dont call RemoveObservers() during shutdown observer notification - if you're
in shutdown, you can't get to the observer service anyway (do_GetService will
fail) so you'll just cause a warning on shutdown.

instead, just let the observer be freed appropriately.

Comment 11

[Alec Flett]

also see bug 129641 - you have to make sure to NS_RELEASE entries from the DNS
service's mHashTable

Comment 12

[gordon]

The DNS Service doesn't use an xpcom shutdown observer.  The problem of the DNS
Service leaking needs to be fixed in a different way.  We need to break the
reference cycle between the thread and the runnable.  I need to make the DNS
thread a separate object from the DNS Service (so the service can "orphan"
threads and create new ones when necessary).  This will fix other bugs besided
just this one.

The patch in bug 129641 is probably a good idea, but it is not/was not resulting
in any leaks.  The nsDNSLookups were all released via eviction at shutdown via
AbortLookups().  The real cause of the leaks in the DNS Service reference cycle.

Comment 13

[Alec Flett]

*** Bug 129641 has been marked as a duplicate of this bug. ***

Comment 14

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 15

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 16

[Tony Tovar]

Shouldn't this be a blocker to one of the meta bugs, e.g. bug 102231 or bug 102472 ?

Comment 17

[benc]

gordon: I set the target to "--" because I think this needs to be targeted or
futured.

Comment 18

[Jesse Ruderman]

This leak entrains many types of objects, making it annoying when trying to look for other leaks.  I see it on almost every shutdown on Mac when doing automated testing.

Comment 19

[Jesse Ruderman]

Now I only see this leak on about 0.5% of shutdowns during automated testing.  I can ignore logs where the DNS service leaked, so if something else causes a mozStorageConnection or an nsStringBuffer to leak in a different instance of Firefox, I'll still notice.

Comment 20

[Mike Schroepfer]

Is this an easy shutdown leak we should just get fixed up?  Jim can you help?

Comment 21

[Jesse Ruderman]

Tomcat says he can reliably reproduce this leak (or at least a bug that leaks the same objects) by selecting "Work Offline" from the File menu.  See bug 403694.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm gonna mark this a blocker, just because it adds so much noise to regular leak hunting.

Comment 24

[Patrick McManus [:mcmanus]]

we addressed this a while ago.. completely killing the shutdown leak is nigh impossible because some threadpool threads are stuck in a blocking OS call that can take ~forever to return.. so we shutdown fast in non leak checking builds and in leak checking builds give it a healthy timeout