From cc54b03f9ad53b6850ee98b9a81eb6fc357763b2 Mon Sep 17 00:00:00 2001
From: Ping Chen <remotenonsense@gmail.com>
Date: Tue, 13 Dec 2022 21:29:00 +1100
Subject: [PATCH] Bug 1762690 - Start idle only if ImapClient is still online.
 r=mkmelin

Differential Revision: https://phabricator.services.mozilla.com/D164530

--HG--
extra : amend_source : 8c143a19df108500126c85452226ecda158a5081
---
 mailnews/imap/src/ImapClient.jsm         | 20 +++++++---
 mailnews/imap/src/ImapIncomingServer.jsm | 49 +++++++++++++-----------
 mailnews/imap/src/ImapService.jsm        |  3 +-
 3 files changed, 42 insertions(+), 30 deletions(-)

diff --git a/mailnews/imap/src/ImapClient.jsm b/mailnews/imap/src/ImapClient.jsm
index ec77666536..32e905b3f9 100644
--- a/mailnews/imap/src/ImapClient.jsm
+++ b/mailnews/imap/src/ImapClient.jsm
@@ -73,6 +73,13 @@ class ImapClient {
     this._loadPrefs();
   }
 
+  /**
+   * @type {boolean} - Whether the socket is open.
+   */
+  get isOnline() {
+    return this._socket?.readyState == "open";
+  }
+
   /**
    * Load imap related preferences, many behaviors depend on these pref values.
    */
@@ -105,7 +112,7 @@ class ImapClient {
    * Initiate a connection to the server
    */
   connect() {
-    if (this._socket?.readyState == "open") {
+    if (this.isOnline) {
       // Reuse the connection.
       this.onReady();
       this._setSocketTimeout(this._prefs.tcpTimeout);
@@ -129,7 +136,7 @@ class ImapClient {
    * @param {number} timeout - The timeout in seconds.
    */
   _setSocketTimeout(timeout) {
-    this._socket.transport.setTimeout(
+    this._socket.transport?.setTimeout(
       Ci.nsISocketTransport.TIMEOUT_READ_WRITE,
       timeout
     );
@@ -638,14 +645,16 @@ class ImapClient {
       }
     };
     this._sendTagged("IDLE");
+    this._setSocketTimeout(PR_UINT32_MAX);
     this._idling = true;
     this._idleTimer = setTimeout(() => {
       this.endIdle(() => {
         this._actionNoop();
       });
       // Per rfc2177, should terminate the IDLE and re-issue it at least every
-      // 29 minutes.
-    }, 20 * 60 * 1000);
+      // 29 minutes. But in practice many servers timeout before that. A noop
+      // every 5min is better than timeout.
+    }, 5 * 60 * 1000);
     this._logger.debug(`Idling in ${this.folder.URI}`);
   }
 
@@ -767,7 +776,7 @@ class ImapClient {
       this._logger.debug(`C: ${str}`);
     }
 
-    if (this._socket?.readyState != "open") {
+    if (!this.isOnline) {
       if (!str.includes("LOGOUT")) {
         this._logger.warn(
           `Failed to send because socket state is ${this._socket?.readyState}`
@@ -1615,7 +1624,6 @@ class ImapClient {
     this._reset();
     // Tell ImapIncomingServer this client can be reused now.
     this.onFree?.();
-    this._setSocketTimeout(PR_UINT32_MAX);
   };
 
   /** @see nsIImapProtocol */
diff --git a/mailnews/imap/src/ImapIncomingServer.jsm b/mailnews/imap/src/ImapIncomingServer.jsm
index 0bd3ce11a8..d9ffe833b3 100644
--- a/mailnews/imap/src/ImapIncomingServer.jsm
+++ b/mailnews/imap/src/ImapIncomingServer.jsm
@@ -550,33 +550,38 @@ class ImapIncomingServer extends MsgIncomingServer {
     if (!client) {
       return;
     }
-    client.onFree = async () => {
+    let startIdle = async () => {
+      if (!this.useIdle || !this._capabilities.includes("IDLE")) {
+        return;
+      }
+
+      // IDLE is configed and supported, use IDLE to receive server pushes.
+      let hasInboxConnection = this._connections.some(c =>
+        this._isInboxConnection(c)
+      );
+      let alreadyIdling =
+        client.folder &&
+        this._connections.find(
+          c => c != client && !c.busy && c.folder == client.folder
+        );
+      if (!hasInboxConnection) {
+        client.selectFolder(
+          this.rootFolder.getFolderWithFlags(Ci.nsMsgFolderFlags.Inbox)
+        );
+      } else if (client.folder && !alreadyIdling) {
+        client.idle();
+      } else if (alreadyIdling) {
+        client.folder = null;
+      }
+    };
+    client.onFree = () => {
       client.busy = false;
       let resolve = this._connectionWaitingQueue.shift();
       if (resolve) {
         // Resolve the first waiting in queue.
         resolve(true);
-      } else {
-        let hasInboxConnection = this._connections.some(c =>
-          this._isInboxConnection(c)
-        );
-        let alreadyIdling =
-          client.folder &&
-          this._connections.find(
-            c => c != client && !c.busy && c.folder == client.folder
-          );
-        if (this.useIdle && this._capabilities.includes("IDLE")) {
-          // IDLE is configed and supported, use IDLE to receive server pushes.
-          if (!hasInboxConnection) {
-            client.selectFolder(
-              this.rootFolder.getFolderWithFlags(Ci.nsMsgFolderFlags.Inbox)
-            );
-          } else if (client.folder && !alreadyIdling) {
-            client.idle();
-          } else if (alreadyIdling) {
-            client.folder = null;
-          }
-        }
+      } else if (client.isOnline) {
+        startIdle();
       }
     };
     handler(client);
diff --git a/mailnews/imap/src/ImapService.jsm b/mailnews/imap/src/ImapService.jsm
index e9fc0eb7f2..a45792461c 100644
--- a/mailnews/imap/src/ImapService.jsm
+++ b/mailnews/imap/src/ImapService.jsm
@@ -428,8 +428,7 @@ class ImapService {
    *   instance, and do some actions.
    */
   _withClient(folder, handler) {
-    let server = folder.QueryInterface(Ci.nsIMsgImapMailFolder)
-      .imapIncomingServer;
+    let server = folder.server.QueryInterface(Ci.nsIMsgIncomingServer);
     let runningUrl = Services.io
       .newURI(`imap://${server.hostName}:${server.port}`)
       .QueryInterface(Ci.nsIMsgMailNewsUrl);