344205 - Does not handle NO response to IMAP IDLE command from server that supports IDLE (Tb issues DONE even though NO to IDLE)

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Curtis King]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en) AppleWebKit/418.8 (KHTML, like Gecko) Safari/419.3
Build Identifier: version 3 alpha 1 (20060706)

2 OK [CAPABILITY UIDPLUS LITERAL+ UNSELECT NAMESPACE SORT MULTIAPPEND THREAD=ORDEREDSUBJECT THREAD=REFERENCES X-DRAFT-I01-ESEARCH LIST-EXTENDED IDLE URLAUTH CATENATE CONDSTORE STARTTLS] AUTHENTICATE Completed
3 namespace
* NAMESPACE (("" "/")) NIL (("Shared Folders/" "/"))
3 OK NAMESPACE Completed
4 lsub "" "*"
* LSUB () "/" Trash
4 OK LSUB Completed
5 lsub "" "Shared Folders/*"
5 OK LSUB Completed
6 list "" "INBOX"
* LIST (\Noinferiors) "/" INBOX
6 OK LIST Completed
7 select "INBOX"
* FLAGS (\answered \flagged \deleted \draft \seen)
* OK [PERMANENTFLAGS (\answered \flagged \deleted \draft \seen \*)]  
* 15 EXISTS
* 0 RECENT
* OK [UNSEEN 4]  
* OK [UIDVALIDITY 1152410760]  
* OK [UIDNEXT 18]  
* OK [URLMECH INTERNAL]
* OK [HIGHESTMODSEQ 305]  
7 OK [READ-WRITE] SELECT Completed
8 UID fetch 1:* (FLAGS)
* 1 FETCH (FLAGS (\seen) UID 1)
* 2 FETCH (FLAGS (\seen) UID 2)
* 3 FETCH (FLAGS (\seen) UID 3)
* 4 FETCH (FLAGS () UID 4)
* 5 FETCH (FLAGS (\seen) UID 5)
* 6 FETCH (FLAGS () UID 7)
* 7 FETCH (FLAGS () UID 8)
* 8 FETCH (FLAGS (\seen) UID 9)
* 9 FETCH (FLAGS (\seen) UID 11)
* 10 FETCH (FLAGS (\seen) UID 12)
* 11 FETCH (FLAGS (\seen) UID 13)
* 12 FETCH (FLAGS (\seen) UID 14)
* 13 FETCH (FLAGS () UID 15)
* 14 FETCH (FLAGS (\seen) UID 16)
* 15 FETCH (FLAGS (\seen) UID 17)
8 OK UID FETCH Completed
9 IDLE
9 NO IDLE Internal Error
DONE
DONE BAD Null command
10 authenticate CRAM-MD5
10 BAD session in wrong state for the command
11 authenticate CRAM-MD5
11 BAD session in wrong state for the command
12 logout
* BYE LOGOUT received
12 OK LOGOUT Completed


Reproducible: Always

Steps to Reproduce:
1. Have the IMAP server return NO to IDLE command
2. select a message or change a flag.
3. Client now thinks it's not authenticated which it is.
4. Need to logout

Actual Results:  
Need to logout

Expected Results:  
Don't use IDLE for that session.

Comment 1

[Wayne Mery (:wsmwk)]

unrelated to bug 204371?

Comment 2

[Nikolay Shopik]

I don't think its related to bug 204371, also reporter gone. Need to test using latest version.

Comment 3

[Nikolay Shopik]

MoCo have fake IMAP server, probably they can include such test for this.

Comment 4

[Kelly Caudill]

The problem still exists with Thunderbird 2.0.0.19 on Windows.
From the server telemetry for the user:

 <0.288<8 IDLE^M
 >0.000>8 NO IDLE not supported for remote mailboxes^M
 <1.812<DONE^M
 >0.000>* BAD Invalid tag^M
 CONNECTION CLOSED^M

The server immediately responded "NO", then 1.8 seconds later Thunderbird sent the "DONE".  Here's a worse scenario:

 <0.247<8 IDLE
 >0.000>8 NO IDLE not supported for remote mailboxes
 <559.660<DONE
 >0.000>* BAD Invalid tag

Thunderbird spent 10 minutes thinking it was in IDLE.

Comment 5

[Nikolay Shopik]

Kelly could you test using TB3 beta1? I believe it still in trunk, but want make sure to confirm it.

Comment 6

[Kelly Caudill]

yep, same problem on 3.0b1

Comment 7

[Ludovic Hirlimann [:Usul]]

All the imap servers I have access to support the IDLE command.

Comment 8

[Ludovic Hirlimann [:Usul]]

(In reply to comment #7)
> All the imap servers I have access to support the IDLE command.

Tried to use http://www.imapproxy.org but still can't reproduce. Curtis would you be willing to give us access to a server that exhibits this behavior ?

Comment 9

[Wayne Mery (:wsmwk)]

reporter, curtis, is indeed gone. hotmail reports user unknown

Comment 11

[Eino Tuominen]

In nsImapProtocol.cpp there is:

void nsImapProtocol::Idle()
{
  IncrementCommandTagNumber();

  if (m_urlInProgress)
    return;
  nsCAutoString command (GetServerCommandTag());
  command += " IDLE"CRLF;
  nsresult rv = SendData(command.get());
  if (NS_SUCCEEDED(rv))
  {
      m_idle = PR_TRUE;
      // we'll just get back a continuation char at first.
      // + idling...
      ParseIMAPandCheckForNewMail();
      // this will cause us to get notified of data or the socket getting closed.
      // That notification will occur on the socket transport thread - we just
      // need to poke a monitor so the imap thread will do a blocking read
      // and parse the data.
      nsCOMPtr <nsIAsyncInputStream> asyncInputStream = do_QueryInterface(m_inputStream);
      if (asyncInputStream)
        asyncInputStream->AsyncWait(this, 0, 0, nsnull);
  }
}

Note the comment after issuing the IDLE command: ' // we'll just get back a continuation char at first.' It seems to me the code just assumes the server response and does not check the actual response from server.

Disclaimer: I'm not very familiar with C++

Comment 12

[Eino Tuominen]

I don't want to sound like I'm making demands, but are there any plans to fix this bug? Is there anything I can help with? I can provide you with a test server to reproduce the problem if that's of any use.

Comment 13

[David :Bienvenu]

Hi Eino, Yes, thx, a test account would be helpful, since we're unlikely to have access to a server that advertises IDLE but fails...

Comment 14

[gene smith]

(In reply to David :Bienvenu from comment #13)
> Hi Eino, Yes, thx, a test account would be helpful, since we're unlikely to
> have access to a server that advertises IDLE but fails...

Maybe can force IDLE on a non-idle server and see this problem? I need to try it (NI to myself).

Comment 15

[gene smith]

This keep appearing in my overdue NI email. I wonder if this is a real-world problem now. Why would a server report IDLE capability and then refuse the command with NO response? If so, it's a major server bug. Will try to duplicate this when I get a chance but clearing NI.

Comment 16

[gene smith]

Attachment: fix-idle-bad-or-no.diff

This shows a fix for the problem. It checks that the IDLE command actually responds with + and if it doesn't, an error is flagged so that IDLE state is not actually entered. Assuming this is a transient error, the code will try to send the IDLE the next time the folder is selected and if + is returned, IDLE state will be entered as it normally would.

This is just an interim diff (only for test purposes) since it actually causes tb to send the IDLE command even if the imap server doesn't have the IDLE capability. This causes a server that doesn't support IDLE to always produce a BAD or NO response to the IDLE command. If the server does support IDLE, this code will have no effect (other than the printfs or if there is a transient IDLE response error).

There are also several temporary printf("gds: ....\n") for tracing purposes.

Comment 17

[gene smith]

Attachment: fix-idle-bad-or-no-v2.diff

There is a major problem with the previous diff in that any normal untagged response that occurred during IDLE was getting treated as though it were a bad/no idle response. Also, no notification of a problem was signaled/alerted to the user when the IDLE command was rejected (except via printfs).

This still forces IDLE for all servers so that IDLE command errors occur and has lots of printfs. A clean patch will follow without the force and printfs.

Comment 18

[gene smith]

Attachment: 344205-allow-idle-bad-or-no.patch

Here's a cleaned up patch that is the same as the last diff but w/o the printf's and without forcing IDLE capability.

The notification that occurs uses the method that takes the error response text from the failed IDLE command that the server produces and puts it verbatim in the message. Alternatively, a new string could be added to the bundle for maybe better localization.

I also had a problem showing the notification/alert since the "server sink" and "current url" have to be not null. When a connection is trying to go to IDLE the pointer vars for these are null. Therefore, to get the alert to show, I had to save the "lastest" non-null values for these pointers and use those values in the notification function.

Anyhow, I thought this would be easy but it wasn't. Also, as I mentioned before, not sure this really solves a real-world problem since I've never seen a server that supports IDLE refuse the command. Also, maybe fake-server tests are needed?

Comment 19

[gene smith]

Attachment: idle-error-notices.png

The attachment shows how the notifications look when this error occurs sending the IDLE command:

No.     Time               SPort  DPort  Proto    Len   info
   9747 23:12:38.534953115 52160  142    IMAP     75    Request: 12 IDLE
   9748 23:12:38.571269936 142    52160  IMAP     101   Response: 12 BAD Unrecognized command: IDLE

The response text from the server, except for the "12 BAD", are placed verbatim into the message.

Comment 20

[Eino Tuominen]

Gene, thanks for making progress on this. You say you've never seen a (capable) server refuse an IDLE. One example is (at least was at the time) Oracle Communications Messaging Server (formerly iPlanet Messaging Server and others). If I recall this correctly the server supported IDLE command on normal user mailboxes but did not support it on shared/external mailboxes. There is a couple of examples with IMAP protocol trace on this ticket. This is no longer an issue to me personally but it's always good to see old lingering bugs fixed.

Comment 21

[gene smith]

Eino, I looked at RFC 2177 hoping to see something about some mailboxes not supporting idle when IDLE capability is advertised. Couldn't find anything. However if you look at the possible responses to IDLE you see:

   Result:     OK - IDLE completed after client sent "DONE"
               NO - failure: the server will not allow the IDLE  <------
                    command at this time                         <------
              BAD - command unknown or arguments invalid

So I guess it is alright for a server to generally support IDLE but, for any reason, not allow it "at this time". So rather than going into continuous logout/login cycle that seems to occurs when NO or BAD idle response occurs without my patch, it might be good to go ahead and incorporate it.

Comment 22

[gene smith]

See related Bug 1501631 for an example of a server that seems to regularly respond to IDLE with NO. It may be a duplicate of this bug, not sure yet.

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 9019521 [details] [diff] [review]
344205-allow-idle-bad-or-no.patch

Review of attachment 9019521 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK by visual inspection. You of course have much more experience with this and do a million tests. So it's hard for me to review. I can only check plausibility and formal stuff. So although it's *very* necessary to improve our IMAP offering and I'm very grateful that you invest the time to do it, these are not my favourite reviews, hence the delay for which I apologise.

Let's get the nits fixed and get this landed.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +5143,5 @@
> +  if (aForIdle && !m_imapServerSink && !m_runningUrl)
> +  {
> +    m_imapServerSink = m_imapServerSinkLatest;
> +    m_runningUrl = m_runningUrlLatest;
> +    resetToNull = true;

Why don't you just do this:
nsCOMPtr<nsIMsgMailNewsUrl> mailnewsUrl = do_QueryInterface(m_runningUrlLatest);
m_imapServerSinkLatest->FEAlertFromServer(nsDependentCString(aServerEvent), mailnewsUrl);
No need to set and reset those values.

@@ +7910,5 @@
>        if (asyncInputStream)
>          asyncInputStream->AsyncWait(this, 0, 0, nullptr);
> +    }
> +    else
> +      m_idle = false;

Not: Please add braces to else clause.

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ +248,5 @@
> +        if (inIdle && !((fNextToken && *fNextToken == '+') || untagged))
> +        {
> +          // IDLE "response" + will not be "eaten" as described above since it
> +          // is not an authentication response. So if IDLE response does not
> +          // begin with '+' (continuation) or  '*' (untagged and probably useful

Nit: Double-space.

@@ +249,5 @@
> +        {
> +          // IDLE "response" + will not be "eaten" as described above since it
> +          // is not an authentication response. So if IDLE response does not
> +          // begin with '+' (continuation) or  '*' (untagged and probably useful
> +          // response) then something is wrong and  it is probably a tagged

Nit: Doble-space.

@@ +263,5 @@
> +          }
> +          // Show an alert notication containing the server response to bad IDLE.
> +          fServerConnection.AlertUserEventFromServer(fCurrentLine, true);
> +        }
> +        else

Please add braces to else.

Comment 25

[gene smith]

Attachment: 344205-allow-idle-bad-or-no.patch--v2

Hi Jorg, I've incorporated your requested changes with a bit of restructuring in AlertUserEventFromServer(). I need to test some more, with the changes, against the home.pl server that occasionally produces "NO" response to IDLE. I will let you know ASAP if it is OK. Thanks.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 9022792 [details] [diff] [review]
344205-allow-idle-bad-or-no.patch--v2

Thanks, you've addressed what I asked for. Please let me know whether/when it's good to go.

Comment 27

[gene smith]

It's good to go. Sorry for the delay, got sidetracked looking at something not related. 

I see the alerts for IDLE failure. But I wanted to make sure that non-IDLE bad/no server responses still cause a notification and they do. This is needed because of the changes in nsImapProtocol::AlertUserEventFromServer().

For the record, added temporary test code like this to insert an error on every 50th imap response:

// RFC3501:  resp-cond-state = ("OK" / "NO" / "BAD") SP resp-text
//                             ; Status condition
void nsImapServerResponseParser::resp_cond_state(bool isTagged)
{
  // According to RFC3501, Sec. 7.1, the untagged NO response "indicates a
  // warning; the command can still complete successfully."
  // However, the untagged BAD response "indicates a protocol-level error for
  // which the associated command can not be determined; it can also indicate an
  // internal server failure."
  // Thus, we flag an error for a tagged NO response and for any BAD response.
  static int tempTestGdsForceError = 0;
  tempTestGdsForceError++;
  if ((isTagged && !PL_strcasecmp(fNextToken, "NO")) ||
      !PL_strcasecmp(fNextToken, "BAD"))
    fCurrentCommandFailed = true;
  else
  if (tempTestGdsForceError == 50 && !PL_strcasecmp(fNextToken, "OK"))
  {
      fCurrentCommandFailed = true;
      tempTestGdsForceError = 0;
  }
  AdvanceToNextToken();
  if (ContinueParse())
    resp_text();
}

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a5fe17002ce3
React correctly to NO/BAD tagged response to imap IDLE. r=jorgk