Closed Bug 102229 Opened 23 years ago Closed 9 years ago

DNS Service and DNS Lookups leak at shutdown

Categories

(Core :: Networking, defect, P3)

x86
Windows 2000
defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: Bienvenu, Assigned: sicking)

References

Details

(Keywords: memory-leak, perf, regression, Whiteboard: [adt3])

Attachments

(1 obsolete file)

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.
Attached patch rough fix (obsolete) (deleted) — Splinter Review
i don't believe there is another bug on this... gordon?
Not that I'm aware of. Targeting for 0.9.6.
Target Milestone: --- → mozilla0.9.6
I need to clean the patch up a bit. Targeting for necko stability milestone 0.9.8.
Target Milestone: mozilla0.9.6 → mozilla0.9.8
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Priority: -- → P1
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.
*** Bug 126455 has been marked as a duplicate of this bug. ***
Keywords: mlk
*** Bug 126364 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla0.9.9 → mozilla1.0
Keywords: nsbeta1
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 +
Keywords: perf, regression
per adt, critical for nsbeta1. hence plus.
Keywords: nsbeta1nsbeta1+
Whiteboard: [adt3]
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.
also see bug 129641 - you have to make sure to NS_RELEASE entries from the DNS service's mHashTable
Attachment #51279 - Attachment is obsolete: true
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.
*** Bug 129641 has been marked as a duplicate of this bug. ***
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt. If you have any questions about this, please email adt@netscape.com. You can search for "changing adt3 bugs" to quickly find and delete these bug mails.
Keywords: nsbeta1-
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt. If you have any questions about this, please email adt@netscape.com. You can search for "changing adt3 bugs" to quickly find and delete these bug mails.
Keywords: nsbeta1+
Shouldn't this be a blocker to one of the meta bugs, e.g. bug 102231 or bug 102472 ?
gordon: I set the target to "--" because I think this needs to be targeted or futured.
Target Milestone: mozilla1.0 → ---
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.
Blocks: 398292
Assignee: gordon → nobody
QA Contact: benc → networking
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.
Blocks: 402335
Is this an easy shutdown leak we should just get fixed up? Jim can you help?
Flags: blocking1.9?
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
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.
Assignee: nobody → jonas
Priority: P1 → P2
I'm gonna mark this a blocker, just because it adds so much noise to regular leak hunting.
Flags: wanted1.9+
Flags: blocking1.9-
Flags: blocking1.9+
Blocks: 403694
No longer depends on: 403694
Flags: tracking1.9+ → wanted-next+
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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: