The pref service is not threadsafe (see bug 380315). The IMAP code touches preferences from off-thread in nsImapProtocol::TryToLogon: 7545 nsCOMPtr<nsIPrefBranch> prefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv)); 7546 if (NS_SUCCEEDED(rv) && prefBranch) 7547 prefBranch->GetBoolPref("mail.auth_login", &prefBool);

is this a regression?

I have no idea, to be honest. There've been IMAP threading changes since 1.8, but I don't know whether they affected this.

Per discussion in m.d.a.seamonkey minusing this for 1.9 - if it is a core bug and seamonkey or tbird need this in the 1.9 timeframe please do re-nominate.

Not a regression - the fix would be in the imap code, so it wouldn't affect gecko.

Additionally, is the following code thread-safe? PRBool nsImapProtocol::TryToLogon() { .... .... // get the password and user name for the current incoming server... nsCOMPtr<nsIMsgIncomingServer> server = do_QueryReferent(m_server); if (server) { // we are in the imap thread so *NEVER* try to extract the password with UI // if logon redirection has changed the password, use the cookie as the password if (m_overRideUrlConnectionInfo) password.Assign(m_logonCookie); else rv = server->GetPassword(password); rv = server->GetRealUsername(userName); } What guarantees that "server" is not set to nsnull by another thread just after the 'if(server)' statement and just before the 'server->' call? Does XPCOM weak referencing mechanism prevent such cases? Or this is something we should address in the code?

if you mean the server might be deleted from an other thread, that shouldn't happen because the comptr holds a strong ref to the server object. Code from an other thread couldn't clear out the local variable on the stack.

> Code from an other thread couldn't clear out the local variable on the stack. Yes it cannot but this is not the case here. "if (server)" is not testing a local variable on the stack, it's testing the raw pointer wrapped by the local variable type of nsCOMPtr<> using 'operator nsDerivedSafe<T>*() const' implicit conversion function. Same here for the -> operator. So, according to Weak reference documentation this pointer can become a nsnull anytime.. But also says that Weak refs are not thread-safe! I thought that maybe that's why we are keep getting ###!!! ASSERTION: nsStreamListenerTee not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()' assertion in case of bug 410747. Having said that, I understand now that since m_server is a strong ref and it's a member of imapprotocol class, unless imapprotocol instance goes away, it is safe to use weak ref on m_server. This leads me to the question; is it safe to create a weak ref from a strong ref created by another thread. Because every imapprotocol instance therefore m_server ref is created by UI thread. XPCOM and reference counting give me headache.

Yes, it gives me a headache as well. As I understand it, m_server is a weak ref, but the local comptr is a strong ref to the underlying object that m_server is a weak ref to. But either way, there's a strong ref to the server object so it shouldn't go away. nsMsgIncomingServer supports threadsafe addrefs and remove refs, and I believe it's the underlying object that really handles the weakref stuff. I think it's OK to do what we're doing, as long as we're careful. I'm pretty sure the assertion doesn't have to do with that. I haven't seen that assertion in a while, so I forget what causes it.

>m_server is a weak ref, but the local comptr is a strong ref to the underlying >object that m_server is a weak ref to. But either way, there's a strong ref to >the server object so it shouldn't go away. Didn't realize that m_server is declared as weak reference in imapprotocol. That also explains why it is not allowed to use WeakReference pointers directly. also noticed that I've pasted wrong assertion to commment #7. The following one was concerning me; ##!!! ASSERTION: nsWeakReference not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file nsWeakReference.cpp, line 146

Ah, that one I think you're right about. What does the rest of the stack look like? If it's when the protocol object is getting destroyed, we need to be destroying that weak ref on the same thread that we created it. Either we need to make sure the protocol object is destroyed on the ui thread, or we need to clear the weak ref on the ui thread. If it's the mock channel stuff when we retry a url, we need to try to do the mock channel stuff on the ui thread...I'll look into it.

One of the places we get this assertion is in CloseStreams() when m_server is set to nsnull by protocol thread. Perfectly inline with your explanation. Also explains why I don't get this assertion when I proxy TellThreadToDie over UI thread.

Emre: is the status of this bug up to date? Looks like it might be a blocker if it's still happening.

Yes, it is up-to-date and should be fixed as soon as possible by proxying GetBoolPref("mail.auth_login", &prefBool); call. I will try to come up with a patch before beta1.

ugh, instead of proxying, which is heavy weight, involves changing the interface, etc, how about making the call in SetupWithUrl or some other routine that we know is called from the ui thread, and caching the result in the protocol object?

David, thoughts.

Comment on attachment 348067 [details] [diff] [review] Proposed fix the var name should be m_authLogin; I think the comment is probably too specific - in general, we're doing a bunch of setup that needs to run on the UI thread, but will later be accessed on the imap thread. A comment to that effect would be helpful.

actually, looking at the exisitng comment in the code, it says what you're doing should be in ::Initialize() - so if you could put it there, that would be good.

Member name is changed, initialization i moved to ::Initialize

Comment on attachment 348125 [details] [diff] [review] Take 2 looks good, thx

fix checked in, changeset: 1133:4ccd97dc684c

maybe this regressed, but we're still touching the prefs service indirectly from the imap thread. Patch upcoming.

GetRealUsername reads the username from prefs, which means the imap thread reads prefs. This fix makes us proxy the read over to the ui thread. w/o this fix, I often can't run my builds because the ui thread and the imap thread access prefs at the same time, which makes prefs assert like crazy.

fix checked in. I don't know if we'd want to consider this for 3.0x - I don't know that it has release build ramifications, but it sure hoses debug builds.

Comment on attachment 412338 [details] [diff] [review] proposed fix The only way we can take this is if we put it into 3.0 (due to the idl changes), flagging for approval for now (for discussion later).

Comment on attachment 412338 [details] [diff] [review] proposed fix From the sounds of it we don't think this will fix anything in particular, therefore not taking for 3.0 at the moment to reduce the risk.