Closed Bug 344205 Opened 13 years ago Closed 10 months ago

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

Categories

(MailNews Core :: Networking: IMAP, defect, major)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 65.0

People

(Reporter: cjek420, Assigned: gds)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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.
unrelated to bug 204371?
Assignee: mscott → bienvenu
Component: General → Networking: IMAP
Product: Thunderbird → Core
QA Contact: general → networking.imap
I don't think its related to bug 204371, also reporter gone. Need to test using latest version.
Version: unspecified → Trunk
Keywords: qawanted
MoCo have fake IMAP server, probably they can include such test for this.
OS: Mac OS X → All
Hardware: Macintosh → All
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.
Kelly could you test using TB3 beta1? I believe it still in trunk, but want make sure to confirm it.
yep, same problem on 3.0b1
Status: UNCONFIRMED → NEW
Ever confirmed: true
Product: Core → MailNews Core
All the imap servers I have access to support the IDLE command.
(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 ?
reporter, curtis, is indeed gone. hotmail reports user unknown
Duplicate of this bug: 551968
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++
Keywords: qawanted
Summary: Do not handle server returning NO to IMAP IDLE command → Does not handle NO response to IMAP IDLE command from server that supports IDLE
Severity: normal → major
Summary: Does not handle NO response to IMAP IDLE command from server that supports IDLE → Does not handle NO response to IMAP IDLE command from server that supports IDLE (Tb issues DONE even though NO to IDLE)
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.
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...
Assignee: mozilla → nobody
Flags: in-testsuite?
See Also: → 468490
Blocks: 468490
See Also: 468490
(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).
Flags: needinfo?(gds)
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.
Flags: needinfo?(gds)
Attached patch fix-idle-bad-or-no.diff (obsolete) — Splinter Review
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.
Assignee: nobody → gds
Attached patch fix-idle-bad-or-no-v2.diff (obsolete) — Splinter Review
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.
Attachment #9018957 - Attachment is obsolete: true
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?
Attachment #9019521 - Flags: review?(jorgk)
Attachment #9019521 - Flags: feedback?(mkmelin+mozilla)
Attached image 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.
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.
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.
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.
Duplicate of this bug: 1501631
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.
Attachment #9019521 - Flags: review?(jorgk)
Attachment #9019521 - Flags: review+
Attachment #9019521 - Flags: feedback?(mkmelin+mozilla)
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.
Attachment #9019503 - Attachment is obsolete: true
Attachment #9019521 - Attachment is obsolete: true
Attachment #9022792 - Flags: review?(jorgk)
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.
Attachment #9022792 - Flags: review?(jorgk) → review+
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();
}
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a5fe17002ce3
React correctly to NO/BAD tagged response to imap IDLE. r=jorgk
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
You need to log in before you can comment on or make changes to this bug.