658503 - Landing of |Bug 654201 - avoid nsHttpConnection::IsAlive() running event loop for unused SSL connections| broke nsImapProtocol

Status: RESOLVED FIXED

(MailNews Core :: Networking, defect)

Description

[Ian Neal]

/builds/slave/comm-cen-trunk-lnx/build/mailnews/imap/src/nsImapProtocol.cpp: In member function ‘void nsImapProtocol::TellThreadToDie()’:
/builds/slave/comm-cen-trunk-lnx/build/mailnews/imap/src/nsImapProtocol.cpp:1262:41: error: no matching function for call to ‘nsISocketTransport::IsAlive(PRBool*)’
../../../mozilla/dist/include/nsISocketTransport.h:96:97: note: candidate is: virtual nsresult nsISocketTransport::IsAlive(PRBool, PRBool*)
/builds/slave/comm-cen-trunk-lnx/build/mailnews/imap/src/nsImapProtocol.cpp: In member function ‘virtual PRBool nsImapProtocol::ProcessCurrentURL()’:
/builds/slave/comm-cen-trunk-lnx/build/mailnews/imap/src/nsImapProtocol.cpp:1658:47: warning: suggest parentheses around ‘&&’ within ‘||’
/builds/slave/comm-cen-trunk-lnx/build/mailnews/imap/src/nsImapProtocol.cpp: In member function ‘virtual nsresult nsImapProtocol::CanHandleUrl(nsIImapUrl*, PRBool*, PRBool*)’:
/builds/slave/comm-cen-trunk-lnx/build/mailnews/imap/src/nsImapProtocol.cpp:2223:39: error: no matching function for call to ‘nsISocketTransport::IsAlive(PRBool*)’
../../../mozilla/dist/include/nsISocketTransport.h:96:97: note: candidate is: virtual nsresult nsISocketTransport::IsAlive(PRBool, PRBool*)

Comment 1

[Robert Kaiser]

Judging from the comments and changes in http://hg.mozilla.org/mozilla-central/rev/f1d79c22fd71 it sounds like just adding a PR_FALSE parameter should fix the bustage and preserve previous behavior (just like it was done for FTP in that patch), but given what the parameter was added for and mailnews protocols potentially (hopefully) using SSL, we may actually want to implement something that parallels the HTTP case of the patch - for all those that can use SSL.
IMAP is probably only the first to hit it, others are probably around as well - we need to change every instance of calling ::IsAlive().

Comment 3

[Joshua Cranmer [:jcranmer]]

Grep indicates in my build that IMAP is the only consumer of IsAlive; fitting, as it is the only mailnews protocol that tries to do serious stuff with connections other than "open one and spew text down the center".

The problem comes that SSL_recv is apparently capable of spinning the event loop, which can cause reentrancy issues. TellThreadToDie is already run from a separate thread, so it has no issues here. CanHandleUrl is read from the main thread.

In essence, we have two issues:
IsAlive(PR_FALSE) -> more correct, reentrancy issues
IsAlive(PR_TRUE) -> less correct (can't detect EOF of socket), no reentrancy.

It's not clear to me whether or not the problem will happen if SSL is used or if the problem is caused by nonsupport for reentrancy in the immediate calling code.

Comment 4

[David :Bienvenu]

We won't call IsAlive on a connection that's not established (we have to have sent at least one command on the connection first) so we won't hit the case of calling isAlive on an ssl connection that hasn't completed the ssl handshake. Except, of course, in the STARTTLS case...if the deadlock turns out to be an issue, we could add some additional check that we're not in the middle of an ssl handshake before calling isAlive. Being in the selected state would be a proxy for that; explicitly setting some state is an other option. For now, I'd like to just fix the build, so I'll land an IsAlive(PR_FALSE) patch to fix the bustage.

Comment 5

[David :Bienvenu]

Attachment: fix build bustage

Comment 6

[David :Bienvenu]

Attachment: right patch

I've checked this in...http://hg.mozilla.org/comm-central/rev/754b3e5c79a0

Comment 7

[Ludovic Hirlimann [:Usul]]

Why is the bug still open ?

Comment 8

[David :Bienvenu]

(In reply to comment #7)
> Why is the bug still open ?

To see if there any re-entrancy issues per #c4 - but if it turns out there are issues, those can be addressed in a new bug, I guess.

Comment 9

[Robert Kaiser]

The original bug apparently already landed for Mozilla 5, so if we want to release something off what's currently comm-beta, we'll need to land this there as well.

Comment 10

[Justin Wood (:Callek)]

(In reply to comment #9)
> The original bug apparently already landed for Mozilla 5, so if we want to
> release something off what's currently comm-beta, we'll need to land this
> there as well.

Not according to build logs and what I see with hg log.

I'm backing out on what is now comm-beta (which has this bug included) to fix the bustage.

http://hg.mozilla.org/releases/comm-beta/pushloghtml?changeset=83bbca25b656

Comment 11

[Patrick McManus [:mcmanus]]

as a heads up, bug 658580 is going to revert the patch that started this bug (changing IsAlive()). as part of a patchset that fixes the original issue in a more comprehensive way (i.e. you don't need to set the flag to get the right behavior, and some other theoretical cases of the same class of bug are also prevented)

Comment 12

[David :Bienvenu]

thx for the heads up, Patrick.

Comment 13

[Mark Banner (:standard8)]

I backed out the patch on this bug, now that bug 658580 is reverted:

http://hg.mozilla.org/comm-central/rev/2f905aedc4a8

Leaving this as fixed as that seems most appropriate.

Comment 14

[Robert Kaiser]

(In reply to comment #13)
> I backed out the patch on this bug, now that bug 658580 is reverted

This has also been done on current mozilla-beta now, so we need to revert it on what is comm-beta atm as well.

David, Mark, Justin, anyone of you able to do that?

Comment 15

[David :Bienvenu]

(In reply to comment #14)
> (In reply to comment #13)
> > I backed out the patch on this bug, now that bug 658580 is reverted
> 
> This has also been done on current mozilla-beta now, so we need to revert it
> on what is comm-beta atm as well.
> 
> David, Mark, Justin, anyone of you able to do that?

Yeah, I can do it - just need to rebuild my comm-beta tree to make sure it builds.

Comment 16

[David :Bienvenu]

backed out on comm-beta
http://hg.mozilla.org/releases/comm-beta/rev/8c7086999257