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

RESOLVED FIXED in Thunderbird 6.0

Status

MailNews Core
Networking
--
blocker
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Ian Neal, Assigned: Bienvenu)

Tracking

Trunk
Thunderbird 6.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
/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

7 years ago
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().
Duplicate of this bug: 658498
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.
(Assignee)

Comment 4

7 years ago
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.
(Assignee)

Comment 5

7 years ago
Created attachment 533956 [details] [diff] [review]
fix build bustage
Assignee: nobody → dbienvenu
(Assignee)

Comment 6

7 years ago
Created attachment 533957 [details] [diff] [review]
right patch

I've checked this in...http://hg.mozilla.org/comm-central/rev/754b3e5c79a0
Attachment #533956 - Attachment is obsolete: true
Why is the bug still open ?
(Assignee)

Comment 8

7 years ago
(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.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 9

7 years ago
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.
(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
Target Milestone: --- → Thunderbird 3.4
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)
(Assignee)

Comment 12

7 years ago
thx for the heads up, Patrick.
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

7 years ago
(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?
Depends on: 658580
(Assignee)

Comment 15

7 years ago
(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.
You need to log in before you can comment on or make changes to this bug.