455966 - IMAP threadsafety assertions post-autosync landings

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

I think the first two assertions are probably separate from the rest (and they're already filed in another bug).  But just in case they're somehow related, I'm including them here too:

###!!! ASSERTION: most recent message has 0 or negative size: 'fDownloadingHeaders || fSizeOfMostRecentMessage > 0', file /Users/dmose/s/comm-central-trunk/src/mailnews/imap/src/nsImapServerResponseParser.cpp, line 3242
###!!! ASSERTION: syntax error in generic parser: '!error', file /Users/dmose/s/comm-central-trunk/src/mailnews/imap/src/nsIMAPGenericParser.cpp, line 92
###!!! ASSERTION: nsWeakReference not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsWeakReference.cpp, line 146
###!!! ASSERTION: nsWeakReference not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsWeakReference.cpp, line 146
###!!! ASSERTION: nsWeakReference not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsWeakReference.cpp, line 146
###!!! ASSERTION: nsWeakReference not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsWeakReference.cpp, line 146
###!!! ASSERTION: nsWeakReference not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsWeakReference.cpp, line 146
###!!! ASSERTION: nsAutoSyncState not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/dmose/s/comm-central-trunk/src/mailnews/imap/src/nsAutoSyncState.cpp, line 625
###!!! ASSERTION: nsStandardURL not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/dmose/s/comm-central-trunk/src/mozilla/netwerk/base/src/nsStandardURL.cpp, line 867

Comment 1

[Emre Birol]

I occasionally get 

>###!!! ASSERTION: nsWeakReference not thread-safe: '_mOwningThread.GetThread()
>== PR_GetCurrentThread()', file nsWeakReference.cpp, line 146

mostly when an imap connection goes bad. This is a know issue.

>###!!! ASSERTION: nsAutoSyncState not thread-safe: '_mOwningThread.GetThread()
>== PR_GetCurrentThread()', file
>/Users/dmose/s/comm-central-trunk/src/mailnews/imap/src/nsAutoSyncState.cpp,
>line 625

This one is scary. Unless I interpret this assertion wrong, it means that nsAutoSyncState is called from another thread that is not its creator. nsAutoSyncState is a satellite object of nsImapMailFolder to manage the background downloads. Its creator thread is the same one which creates its owner folder: the main thread. 

nsAutoSyncState is mostly called during idle processing, by main thread, and in nsImapMailFolder::HeaderFetchCompleted by imap protocol thread via proxy, it means by the main thread again. So, not sure what might cause this assertion. Since it follows bunch of nsWeakReference assertions, I suspect imap connection issues might be the source of this problem.

During my tests (see bug 455963, comment #2), I haven't seen these assertions.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Some debug spew with auto-sync debugging turned on:

*** Skipping folder imap://[e-mail]/INBOX [Offline: no]
*** Skipping folder imap://[e-mail]/Sent Messages [Offline: no]
*** Skipping folder imap://[e-mail]/Trash [Offline: no]
*** Folder imap://[e-mail]/Junk is syncd
Downloading 2 messages for folder imap://[e-mail]/Archive/AddrBook
*** Folder imap://[e-mail]/Archive/AddrBook is syncd
*** Skipping folder imap://[e-mail]/Archive [Offline: no]
*** Initiating Auto-Sync on folder imap://[e-mail]/Archive/Personal
*** Initiating Auto-Sync on folder imap://[e-mail]/Archive/Sports
*** Initiating Auto-Sync on folder imap://[e-mail]/Archive/Stuff
*** Initiating Auto-Sync on folder imap://[e-mail]/Archive/Tech
*** Initiating Auto-Sync on folder imap://[e-mail]/Archive/Unsorted
*** Initiating Auto-Sync on folder imap://[e-mail]/bulk
*** Initiating Auto-Sync on folder imap://[e-mail]/Deleted Items
*** Initiating Auto-Sync on folder imap://[e-mail]/Deleted Messages
*** Initiating Auto-Sync on folder imap://[e-mail]/Drafts
*** Initiating Auto-Sync on folder imap://[e-mail]/INBOX
*** Initiating Auto-Sync on folder imap://[e-mail]/Junk
*** Initiating Auto-Sync on folder imap://[e-mail]/Pending
*** Initiating Auto-Sync on folder imap://[e-mail]/Prof
*** Skipping folder imap://[e-mail]/Records [Offline: no]
*** Initiating Auto-Sync on folder imap://[e-mail]/Records/Brokers
*** Initiating Auto-Sync on folder imap://[e-mail]/Records/Misc
*** Initiating Auto-Sync on folder imap://[e-mail]/Records/Receipts
*** Initiating Auto-Sync on folder imap://[e-mail]/Records/Speakeasy
*** Initiating Auto-Sync on folder imap://[e-mail]/Sent Items
*** Initiating Auto-Sync on folder imap://[e-mail]/Sent Messages
*** Initiating Auto-Sync on folder imap://[e-mail]/Sent
*** Initiating Auto-Sync on folder imap://[e-mail]/Stuff
*** Initiating Auto-Sync on folder imap://[e-mail]/Trash
*** Initiating Auto-Sync on folder imap://[e-mail]/@ACTION
*** Initiating Auto-Sync on folder imap://[e-mail]/Apple Mail To Do
*** Initiating Auto-Sync on folder imap://[e-mail]/Archive
*** Initiating Auto-Sync on folder imap://[e-mail]/Bugs
*** Initiating Auto-Sync on folder imap://[e-mail]/Bugs/@ACTION
*** Initiating Auto-Sync on folder imap://[e-mail]/Chats
*** Initiating Auto-Sync on folder imap://[e-mail]/Contacts
*** Initiating Auto-Sync on folder imap://[e-mail]/Deleted Messages
*** Initiating Auto-Sync on folder imap://[e-mail]/Drafts
*** Initiating Auto-Sync on folder imap://[e-mail]/Emailed Contacts
*** Initiating Auto-Sync on folder imap://[e-mail]/INBOX
*** Initiating Auto-Sync on folder imap://[e-mail]/Junk
*** Initiating Auto-Sync on folder imap://[e-mail]/Lists
*** Initiating Auto-Sync on folder imap://[e-mail]/Lists/whatwg
*** Initiating Auto-Sync on folder imap://[e-mail]/Pending
*** Initiating Auto-Sync on folder imap://[e-mail]/Sent Messages
*** Initiating Auto-Sync on folder imap://[e-mail]/Sent
*** Initiating Auto-Sync on folder imap://[e-mail]/Templates
*** Initiating Auto-Sync on folder imap://[e-mail]/Trash
*** Skipping folder imap://[e-mail]/INBOX [Offline: no]
*** Skipping folder imap://[e-mail]/Sent Messages [Offline: no]
*** Skipping folder imap://[e-mail]/Trash [Offline: no]
Downloading 11 messages for folder imap://[e-mail]/Archive/Personal
###!!! ASSERTION: most recent message has 0 or negative size: 'fDownloadingHeaders || fSizeOfMostRecentMessage > 0', file /Users/dmose/s/comm-central-trunk/src/mailnews/imap/src/nsImapServerResponseParser.cpp, line 3242
###!!! ASSERTION: syntax error in generic parser: '!error', file /Users/dmose/s/comm-central-trunk/src/mailnews/imap/src/nsIMAPGenericParser.cpp, line 92
Downloading 11 messages for folder imap://[e-mail]/Archive/Personal
###!!! ASSERTION: nsWeakReference not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsWeakReference.cpp, line 146
###!!! ASSERTION: nsWeakReference not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsWeakReference.cpp, line 146
###!!! ASSERTION: nsWeakReference not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsWeakReference.cpp, line 146
###!!! ASSERTION: nsWeakReference not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsWeakReference.cpp, line 146
###!!! ASSERTION: nsWeakReference not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsWeakReference.cpp, line 146
###!!! ASSERTION: nsAutoSyncState not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/dmose/s/comm-central-trunk/src/mailnews/imap/src/nsAutoSyncState.cpp, line 625
###!!! ASSERTION: nsStandardURL not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/dmose/s/comm-central-trunk/src/mozilla/netwerk/base/src/nsStandardURL.cpp, line 867
###!!! ASSERTION: OnDataAvailable implementation consumed no data: 'Error', file /Users/dmose/s/comm-central-trunk/src/mozilla/netwerk/base/src/nsInputStreamPump.cpp, line 529
###!!! ASSERTION: unknown error, but don't alert user.: 'errorID != UNKNOWN_ERROR', file /Users/dmose/s/comm-central-trunk/src/mailnews/base/util/nsMsgProtocol.cpp, line 431

Comment 3

[David :Bienvenu]

dmose, a more complete stack of the weak ref assertions would be helpful

Comment 4

[David :Bienvenu]

Attachment: possible fix for some of the assertions

Dan, you might try running with this patch and see if some of the assertions go away. This proxies the mock channel over to the ui thread.

Comment 5

[Emre Birol]

It makes the connection related thread-safety assertions go away, thanks David.

rv is not defined in nsImapProtocol::RetryUrl anymore, it gives build error. Should be removed at
 ...do_QueryReferent(m_server, &rv);

Comment 6

[David :Bienvenu]

oops, good catch; I was doing some last minute cleanup of the patch and I "over-cleansed" :-) I should either check that rv or m_server...

Comment 7

[David :Bienvenu]

Attachment: fix undeclared var

It would be really nice to land this at some point soon so the assertions will go away...

Comment 8

[David :Bienvenu]

Comment on attachment 342072 [details] [diff] [review]
fix undeclared var

switch review requests since Dan is overloaded...

Comment 9

[David :Bienvenu]

Neil asked why the patch had to proxy the channel over to the ui thread, instead of just getting rid of the save and restoring of the mock channel, because SetUrlState won't clear the mock channel if re-running is true.

It's true that will fix the assertion. But there are other things going on - the imap url only has a weak reference to the channel, so if we don't hold a strong reference, it could get cleared out from under us as a result of other things that happen in SetUrlState. It's the weak reference accesses in nsImapUrl::Get/SetMockChannel that are causing the assertions.

I've recently seen that if you click on a message and we end up retrying the url, we load the message from the imap server, but don't display it in the ui, which means that something in retryUrl is broken, I believe.

Comment 10

[[e-mail]]

(In reply to comment #9)
> It's true that will fix the assertion. But there are other things going on -
> the imap url only has a weak reference to the channel, so if we don't hold a
> strong reference, it could get cleared out from under us as a result of other
> things that happen in SetUrlState.
Do you mean ReleaseUrlState? Also, for how long do you need that strong ref?

> It's the weak reference accesses in
> nsImapUrl::Get/SetMockChannel that are causing the assertions.
Only SetMockChannel... you've left in the call to GetMockChannel, although possibly that isn't really threadsafe either?

Comment 11

[David :Bienvenu]

> > things that happen in SetUrlState.
> Do you mean ReleaseUrlState? Also, for how long do you need that strong ref?
I do mean ReleaseUrlState. I need the strong ref until the url starts running again and someone else grabs hold of the channel. I believe that happens in the call to nsImapIncomingServer::RetryUrl, in particular, nsImapProtocol::LoadImapUrl calls SetupWithUrl, which adds the channel/request to the load group, which holds a strong ref. I have seen rare instances of problems with the channel however, with and w/o this patch.
> 
> > It's the weak reference accesses in
> > nsImapUrl::Get/SetMockChannel that are causing the assertions.
> Only SetMockChannel... you've left in the call to GetMockChannel, although
> possibly that isn't really threadsafe either?

Could be; I'd have to go back and look at the assertions again...

Comment 12

[[e-mail]]

Comment on attachment 342072 [details] [diff] [review]
fix undeclared var

>+  nsAutoCMonitor mon(this);
>   nsCOMPtr <nsIImapUrl> kungFuGripImapUrl = m_runningUrl;
>   nsCOMPtr <nsIImapMockChannel> saveMockChannel;
>   m_runningUrl->GetMockChannel(getter_AddRefs(saveMockChannel));
>   ReleaseUrlState(PR_TRUE);
One more question: ReleaseUrlState also creates nsAutoCMonitor, is that safe?

Comment 13

[David :Bienvenu]

yes, it's safe, since it's the same thread requesting the monitor. I hit this code all the time (gmail is an excellent stress test :-) )

Comment 14

[[e-mail]]

Comment on attachment 342072 [details] [diff] [review]
fix undeclared var

OK I think that covers everything now.

Comment 15

[David :Bienvenu]

Thx, Neil. Pinging Standard8 for review, to keep on his radar...

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 342072 [details] [diff] [review]
fix undeclared var

-  void retryUrl(in nsIImapUrl aImapUrl);
+  void retryUrl(in nsIImapUrl aImapUrl, in nsIImapMockChannel aChannel);

nit: As you're touching this, please could you add a brief bit of documentation in the usual style.

Comment 17

[David :Bienvenu]

fix checked in, which doxygen comment added