383489 - IMAP code touches the pref service from off the main thread

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

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

Comment 1

[Mike Schroepfer]

is this a regression?

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Mike Schroepfer]

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.

Comment 4

[David :Bienvenu]

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

Comment 5

[Emre Birol]

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?

Comment 6

[David :Bienvenu]

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.

Comment 7

[Emre Birol]

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

Comment 8

[David :Bienvenu]

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.

Comment 9

[Emre Birol]

>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

Comment 10

[David :Bienvenu]

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.

Comment 11

[Emre Birol]

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. 

Comment 12

[Magnus Melin [:mkmelin]]

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

Comment 13

[Emre Birol]

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.

Comment 14

[David :Bienvenu]

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?

Comment 15

[Emre Birol]

Attachment: Proposed fix

David, thoughts.

Comment 16

[David :Bienvenu]

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.

Comment 17

[David :Bienvenu]

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.

Comment 18

[Emre Birol]

Attachment: Take 2

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

Comment 19

[David :Bienvenu]

Comment on attachment 348125 [details] [diff] [review]
Take 2

looks good, thx

Comment 20

[David :Bienvenu]

fix checked in, changeset:   1133:4ccd97dc684c

Comment 21

[David :Bienvenu]

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

Comment 22

[David :Bienvenu]

Attachment: proposed fix

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.

Comment 23

[David :Bienvenu]

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 24

[Mark Banner (:standard8)]

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 25

[Mark Banner (:standard8)]

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.