Closed Bug 457452 Opened 14 years ago Closed 14 years ago

Leaking nsAutoSyncManager on shutdown

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b1

People

(Reporter: standard8, Assigned: bugmil.ebirol)

References

Details

(Keywords: memory-leak, regression, Whiteboard: [Standard8 to land patch once approved (wants to monitor new leak boxes)])

Attachments

(2 files, 3 obsolete files)

From bug 426963 comment 5 - we're leaking a nsAutoSyncManager on shutdown.

Just having a quick scan of the file, I noticed that nsAutoSyncManager in its destructor removes itself from the idle observer service. This is too late and should actually be happening as part of xpcom-shutdown or a bit earlier, see the link for more info:

http://developer.mozilla.org/En/Observer_Notifications

This should be a simple fix, and I think we should fix it up before b2.
Flags: blocking-thunderbird3+
Assignee: nobody → bugmil.ebirol
Attached patch Proposed fix 1 (obsolete) — Splinter Review
First cut.
(In reply to comment #1)
> Created an attachment (id=341137) [details]
> Proposed fix 1
> 
> First cut.

with this patch i also get this assertion at shutdown (with an existing trunk profile), i don't see it without the patch:

###!!! ASSERTION: nsDefaultURIFixup not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/skywalker/comm-central/mozilla/docshell/base/nsDefaultURIFixup.cpp, line 56

(gdb) bt
#0  0x9582e4ee in semaphore_wait_signal_trap ()
#1  0x95835fc5 in pthread_mutex_lock ()
#2  0x00600ecb in PR_Lock (lock=0x70bbd0) at /Users/skywalker/comm-central/mozilla/nsprpub/pr/src/pthreads/ptsynch.c:206
#3  0x0046bb7f in NS_LogAddRef_P (aPtr=0x739c10, aRefcnt=6, aClazz=0x9cc987b "nsBaseAppShell", classSize=64) at /Users/skywalker/comm-central/mozilla/xpcom/base/nsTraceRefcntImpl.cpp:920
#4  0x09ca1a5b in nsBaseAppShell::AddRef (this=0x739c10) at /Users/skywalker/comm-central/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:49
#5  0x00458213 in nsCOMPtr<nsIThreadObserver>::nsCOMPtr (this=0xbfffed90, aSmartPtr=@0x716654) at nsCOMPtr.h:560
#6  0x00456ea2 in nsThread::ProcessNextEvent (this=0x716640, mayWait=1, result=0xbfffedec) at /Users/skywalker/comm-central/mozilla/xpcom/threads/nsThread.cpp:495
#7  0x003e082e in NS_ProcessNextEvent_P (thread=0x716640, mayWait=1) at nsThreadUtils.cpp:227
#8  0x004576b3 in nsThread::Shutdown (this=0x724ff0) at /Users/skywalker/comm-central/mozilla/xpcom/threads/nsThread.cpp:465
#9  0x098c2322 in nsSocketTransportService::Shutdown (this=0x813000) at /Users/skywalker/comm-central/mozilla/netwerk/base/src/nsSocketTransportService2.cpp:445
#10 0x0989701d in nsIOService::SetOffline (this=0x724b50, offline=1) at /Users/skywalker/comm-central/mozilla/netwerk/base/src/nsIOService.cpp:619
#11 0x098993c1 in nsIOService::Observe (this=0x724b50, subject=0x14b5ae80, topic=0x12e592 "profile-change-net-teardown", data=0x130f40) at /Users/skywalker/comm-central/mozilla/netwerk/base/src/nsIOService.cpp:776
#12 0x003f6e41 in nsObserverList::NotifyObservers (this=0x8d6c4c, aSubject=0x14b5ae80, aTopic=0x12e592 "profile-change-net-teardown", someData=0x130f40) at /Users/skywalker/comm-central/mozilla/xpcom/ds/nsObserverList.cpp:128
#13 0x003f8392 in nsObserverService::NotifyObservers (this=0x724d70, aSubject=0x14b5ae80, aTopic=0x12e592 "profile-change-net-teardown", someData=0x130f40) at /Users/skywalker/comm-central/mozilla/xpcom/ds/nsObserverService.cpp:181
#14 0x000f0f24 in nsXREDirProvider::DoShutdown (this=0xbffff32c) at /Users/skywalker/comm-central/mozilla/toolkit/xre/nsXREDirProvider.cpp:852
#15 0x000dafcb in ScopedXPCOMStartup::~ScopedXPCOMStartup (this=0xbffff3cc) at /Users/skywalker/comm-central/mozilla/toolkit/xre/nsAppRunner.cpp:946
#16 0x000e29ec in XRE_main (argc=1, argv=0xbffff6bc, aAppData=0x70e2c0) at /Users/skywalker/comm-central/mozilla/toolkit/xre/nsAppRunner.cpp:3306
#17 0x0000285f in main (argc=1, argv=0xbffff6bc) at /Users/skywalker/comm-central/mail/app/nsMailApp.cpp:103
good news is that the patch does seem to fix the leak on shutdown for a fresh profile on Mac 10.5.5 though.
Gary, I don't see this assertion. Can you reproduce it consistently?

You can debug it with XPCOM_DEBUG_BREAK set. Simply do "$export XPCOM_DEBUG_BREAK trap" on your console, and run tb. This makes gdb stop at every assertion. When you think that you reach to this assertion, then you can get a stack trace. Thanks in advance.
(In reply to comment #4)
> Gary, I don't see this assertion. Can you reproduce it consistently?

Unfortunately, no, I can't. It seems to be one-off.

> You can debug it with XPCOM_DEBUG_BREAK set. Simply do "$export
> XPCOM_DEBUG_BREAK trap" on your console, and run tb. This makes gdb stop at
> every assertion. When you think that you reach to this assertion, then you can
> get a stack trace. Thanks in advance.

That's what I did to get the stack trace above.
Why are you getting nsIObserverService to hold a weak reference? The service (once started) won't be shutdown until xpcom-shutdown, so it doesn't matter if the observer service holds a strong reference.
As I understand it nsIObserverService requires the listener to have weak reference support, mainly that's why I have included weak reference support to nsAutoSyncManager. I think this way nsIObserverService doesn't force the listener to stay alive.. Does the patch solve the leakage?
nsIObeserverService can work with strong or weak references depending on what the third argument to AddObserver is (PR_TRUE is to use a weak ref, PR_FALSE a strong ref).

nsAutoSyncManager is a service (you only create/call it with do_GetService). That means once you've created it, even if you drop all references to it, it won't get deleted until shutdown (i.e. just after the xpcom-shutdown notification).

So after the patch you're now registering with the observer service and idle service. You are also removing the registrations when you get the xpcom-shutdown notification (RemoveObserver) - that will drop the observer service reference to nsAutoSyncManager, and then when you get to the closing services stage, there won't be any references hanging around and nsAutoSyncManager will still shutdown cleanly.

Finally, the current patch does solve the leakage.
Attached patch Proposed fix 2 (obsolete) — Splinter Review
Still don't get the following assertion:

###!!! ASSERTION: nsDefaultURIFixup not thread-safe:
'_mOwningThread.GetThread() == PR_GetCurrentThread()', file
/Users/skywalker/comm-central/mozilla/docshell/base/nsDefaultURIFixup.cpp, line
56
Attachment #341137 - Attachment is obsolete: true
Attachment #344243 - Flags: review?(bugzilla)
Attachment #344243 - Flags: review?(bugzilla) → review+
Comment on attachment 344243 [details] [diff] [review]
Proposed fix 2

That looks better. The only thing I can think of with the assertion is that part of auto sync or something may have been active at shutdown (I'm not seeing it either). Although I'm a bit hesitant about it, shutting down nsAutoSyncManager correctly is better than what we're doing at the moment. So I think if Garry is still seeing the assertion after this goes in, then we should cover that in a separate bug, certainly I'd expect it could be an issue that is exposed by fixing this bug, rather than this bug causing it.

One nit:

nsAutoSyncManager::~nsAutoSyncManager()
-{
-  if (mTimer)
-    mTimer->Cancel();
-  
-  if (mIdleService)
-    mIdleService->RemoveIdleObserver(this, kIdleTimeInSec);
-}
+{}

Please put the brackets on separate lines:

{
}
I want to land this, as I want to watch the new leak boxes during & after the landing.
Whiteboard: [Standard8 to land patch once approved (wants to monitor new leak boxes)]
Comment on attachment 344243 [details] [diff] [review]
Proposed fix 2

Given my comments that this is ok, moving reviews on so we can hopefully get this landed tomorrow and we can get the bloat boxes on the tree when they are green.
Attachment #344243 - Flags: superreview?(bienvenu)
Comment on attachment 344243 [details] [diff] [review]
Proposed fix 2

I'm not sure we need the assertions; we'll get warnings from xpcom if these calls fail, I believe, which should be sufficient.
Attachment #344243 - Flags: superreview?(bienvenu) → superreview+
Emre, can you update the patch and I'll land it in later today or tomorrow?
Attached patch Patch rev 1 (obsolete) — Splinter Review
Nit fixed, assertions are removed.
Attachment #344243 - Attachment is obsolete: true
Attached patch Patch rev1.1Splinter Review
Previous is the wrong patch.
Attachment #344322 - Attachment is obsolete: true
Checked in: http://hg.mozilla.org/comm-central/rev/aab166693221

This made the Linux bloat box drop to ref count leaks of 0 bytes and turn green (its currently in test on MozillaTest tree).
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.