457452 - Leaking nsAutoSyncManager on shutdown

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Emre Birol]

Attachment: Proposed fix 1

First cut.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: leak log with patch applied

good news is that the patch does seem to fix the leak on shutdown for a fresh profile on Mac 10.5.5 though.

Comment 4

[Emre Birol]

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.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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.

Comment 6

[Mark Banner (:standard8)]

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.

Comment 7

[Emre Birol]

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?

Comment 8

[Mark Banner (:standard8)]

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.

Comment 9

[Emre Birol]

Attachment: Proposed fix 2

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

Comment 10

[Mark Banner (:standard8)]

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:

{
}

Comment 11

[Mark Banner (:standard8)]

I want to land this, as I want to watch the new leak boxes during & after the landing.

Comment 12

[Mark Banner (:standard8)]

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.

Comment 13

[David :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.

Comment 14

[Mark Banner (:standard8)]

Emre, can you update the patch and I'll land it in later today or tomorrow?

Comment 15

[Emre Birol]

Attachment: Patch rev 1

Nit fixed, assertions are removed.

Comment 16

[Emre Birol]

Attachment: Patch rev1.1

Previous is the wrong patch.

Comment 17

[Mark Banner (:standard8)]

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).