Bug 344205 - React correctly to NO/BAD tagged response to imap IDLE.

User is notifified and IDLE state is not entered if IDLE command fails.

Tag #1273

mailnews/imap/src/nsImapProtocol.cpp

@@ -655,6 +655,7 @@ nsImapProtocol::SetupSinkProxy()
       res = m_runningUrl->GetImapServerSink(getter_AddRefs(aImapServerSink));
       if (aImapServerSink) {
         m_imapServerSink = new ImapServerSinkProxy(aImapServerSink);
+        m_imapServerSinkLatest =  m_imapServerSink;
       } else {
         return NS_ERROR_ILLEGAL_VALUE;
       }
@@ -695,6 +696,7 @@ nsresult nsImapProtocol::SetupWithUrl(nsIURI * aURL, nsISupports* aConsumer)
   if (aURL)
   {
     m_runningUrl = do_QueryInterface(aURL, &rv);
+    m_runningUrlLatest = m_runningUrl;
     if (NS_FAILED(rv)) return rv;
     nsCOMPtr<nsIMsgMailNewsUrl> mailnewsUrl = do_QueryInterface(m_runningUrl);
     nsCOMPtr<nsIMsgIncomingServer> server = do_QueryReferent(m_server);
@@ -1425,6 +1427,11 @@ nsImapProtocol::ImapThreadMainLoop()
                 == nsImapServerResponseParser::kFolderSelected)
         {
           Idle(); // for now, lets just do it. We'll probably want to use a timer
+          if (!m_idle)
+          {
+            // Server rejected IDLE. Treat like IDLE not enabled or available.
+            m_imapMailFolderSink = nullptr;
+          }
         }
         else // if not idle, don't need to remember folder sink
           m_imapMailFolderSink = nullptr;
@@ -5105,14 +5112,26 @@ nsImapProtocol::AlertUserEvent(const char * message)
 }
 
 void
-nsImapProtocol::AlertUserEventFromServer(const char * aServerEvent)
+nsImapProtocol::AlertUserEventFromServer(const char * aServerEvent, bool aForIdle)
 {
-    if (m_imapServerSink && aServerEvent)
+  if (aServerEvent)
+  {
+    // If called due to BAD/NO imap IDLE response, the server sink and running url
+    // are typically null when IDLE command is sent. So use the stored latest
+    // values for these so that the error alert notification occurs.
+    if (aForIdle && !m_imapServerSink && !m_runningUrl && m_imapServerSinkLatest)
+    {
+      nsCOMPtr<nsIMsgMailNewsUrl> mailnewsUrl = do_QueryInterface(m_runningUrlLatest);
+      m_imapServerSinkLatest->FEAlertFromServer(nsDependentCString(aServerEvent),
+                                                mailnewsUrl);
+    }
+    else if (m_imapServerSink)
     {
       nsCOMPtr<nsIMsgMailNewsUrl> mailnewsUrl = do_QueryInterface(m_runningUrl);
       m_imapServerSink->FEAlertFromServer(nsDependentCString(aServerEvent),
                                           mailnewsUrl);
     }
+  }
 }
 
 void nsImapProtocol::ResetProgressInfo()
@@ -7882,10 +7901,12 @@ void nsImapProtocol::Idle()
   nsresult rv = SendData(command.get());
   if (NS_SUCCEEDED(rv))
   {
+    // we'll just get back a continuation char at first.
+    // + idling...
+    ParseIMAPandCheckForNewMail();
+    if (GetServerStateParser().LastCommandSuccessful())
+    {
       m_idle = 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
@@ -7893,6 +7914,11 @@ void nsImapProtocol::Idle()
       nsCOMPtr <nsIAsyncInputStream> asyncInputStream = do_QueryInterface(m_inputStream);
       if (asyncInputStream)
         asyncInputStream->AsyncWait(this, 0, 0, nullptr);
+    }
+    else
+    {
+      m_idle = false;
+    }
   }
 }
 

mailnews/imap/src/nsImapProtocol.h

@@ -237,7 +237,7 @@ public:
   void DiscoverMailboxSpec(nsImapMailboxSpec * adoptedBoxSpec);
   void AlertUserEventUsingName(const char* aMessageId);
   void AlertUserEvent(const char * message);
-  void AlertUserEventFromServer(const char * aServerEvent);
+  void AlertUserEventFromServer(const char * aServerEvent, bool aForIdle = false);
 
   void ProgressEventFunctionUsingName(const char* aMsgId);
   void ProgressEventFunctionUsingNameWithString(const char* aMsgName, const char *
@@ -316,6 +316,7 @@ private:
   // finish processng a url and it is set whenever we call Load on a url
   bool m_urlInProgress;
   nsCOMPtr<nsIImapUrl> m_runningUrl; // the nsIImapURL that is currently running
+  nsCOMPtr<nsIImapUrl> m_runningUrlLatest;
   nsImapAction m_imapAction;  // current imap action associated with this connnection...
 
   nsCString             m_hostName;
@@ -371,6 +372,7 @@ private:
   RefPtr<ImapMailFolderSinkProxy> m_imapMailFolderSink;
   RefPtr<ImapMessageSinkProxy>    m_imapMessageSink;
   RefPtr<ImapServerSinkProxy>     m_imapServerSink;
+  RefPtr<ImapServerSinkProxy>     m_imapServerSinkLatest;
   RefPtr<ImapProtocolSinkProxy>   m_imapProtocolSink;
 
   // helper function to setup imap sink interface proxies

mailnews/imap/src/nsImapServerResponseParser.cpp

@@ -188,6 +188,9 @@ void nsImapServerResponseParser::ParseIMAPServerResponse(const char *aCurrentCom
     if (commandToken && ContinueParse())
       PreProcessCommandToken(commandToken, aCurrentCommand);
 
+    // For checking expected response to IDLE command below.
+    bool untagged = false;
+
     if (ContinueParse())
     {
       ResetLexAnalyzer();
@@ -212,6 +215,7 @@ void nsImapServerResponseParser::ParseIMAPServerResponse(const char *aCurrentCom
             else if (!inIdle && !fCurrentCommandFailed && !aGreetingWithCapability)
               AdvanceToNextToken();
           }
+          untagged = true;
         }
 
         // command continuation request [RFC3501, Sec. 7.5]
@@ -244,7 +248,29 @@ void nsImapServerResponseParser::ParseIMAPServerResponse(const char *aCurrentCom
       // fWaitingForMoreClientInput so we don't lose that information....
       if ((fNextToken && *fNextToken == '+') || inIdle)
       {
-        fWaitingForMoreClientInput = true;
+        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
+          // response) then something is wrong and it is probably a tagged
+          // NO or BAD due to transient error or bad configuration of the server.
+          if (!PL_strcmp(fCurrentCommandTag, fNextToken))
+          {
+            response_tagged();
+          }
+          else
+          {
+            // Expected tag doesn't match the received tag. Not good, start over.
+            response_fatal();
+          }
+          // Show an alert notication containing the server response to bad IDLE.
+          fServerConnection.AlertUserEventFromServer(fCurrentLine, true);
+        }
+        else
+        {
+          fWaitingForMoreClientInput = true;
+        }
       }
       // if we aren't still waiting for more input....
       else if (!fWaitingForMoreClientInput && !aGreetingWithCapability)
@@ -262,7 +288,7 @@ void nsImapServerResponseParser::ParseIMAPServerResponse(const char *aCurrentCom
           // a failed command may change the eIMAPstate
           ProcessBadCommand(commandToken);
           if (fReportingErrors && !aIgnoreBadAndNOResponses)
-            fServerConnection.AlertUserEventFromServer(fCurrentLine);
+            fServerConnection.AlertUserEventFromServer(fCurrentLine, false);
         }
       }
     }