Bug 1787963 - Fix offline local->IMAP message move operation. r=mkmelin

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

mailnews/imap/src/nsImapMailFolder.cpp

@@ -6806,7 +6806,6 @@ nsresult nsImapMailFolder::CopyMessagesOffline(
     if (txnMgr) txnMgr->EndBatch(false);
   }
 
-  // Do this before delete, as it destroys the messages
   if (!msgHdrsCopied.IsEmpty()) {
     nsCOMPtr<nsIMsgFolderNotificationService> notifier(
         do_GetService(NS_MSGNOTIFICATIONSERVICE_CONTRACTID));
@@ -6816,7 +6815,38 @@ nsresult nsImapMailFolder::CopyMessagesOffline(
     }
   }
 
-  if (isMove && NS_SUCCEEDED(rv) && (deleteToTrash || deleteImmediately)) {
+  // NOTE (Bug 1787963):
+  // If we're performing a move, by rights we should be deleting the source
+  // message(s) here. But that would mean they won't be available when we try
+  // to run the offline move operation once we're back online. So we'll just
+  // leave things as they are:
+  //  - the message(s) copied into the destination folder
+  //  - the original message(s) left in the source folder
+  //  - the offline move operation all queued up for when we go back online
+  // When we do go back online, the offline move op will be performed and
+  // the source message(s) will be deleted. For real.
+  // Would be nice to have some marker to hide or grey out messages which are
+  // in this state of impending doom... but it's a pretty obscure corner case
+  // and we've already got quite enough of those.
+  //
+  // BUT... CopyMessagesOffline() is also used when online (ha!), *if* we're
+  // copying between folders on the same nsIMsgIncomingServer, in order to
+  // support undo. In that case we _do_ want to go ahead with the delete now.
+
+  nsCOMPtr<nsIMsgIncomingServer> srcServer;
+  if (NS_SUCCEEDED(rv)) {
+    rv = srcFolder->GetServer(getter_AddRefs(srcServer));
+  }
+  nsCOMPtr<nsIMsgIncomingServer> destServer;
+  if (NS_SUCCEEDED(rv)) {
+    rv = GetServer(getter_AddRefs(destServer));
+  }
+  bool sameServer;
+  if (NS_SUCCEEDED(rv)) {
+    rv = destServer->Equals(srcServer, &sameServer);
+  }
+  if (NS_SUCCEEDED(rv) && sameServer && isMove &&
+      (deleteToTrash || deleteImmediately)) {
     DeleteStoreMessages(keysToDelete, srcFolder);
     srcFolder->EnableNotifications(nsIMsgFolder::allMessageCountNotifications,
                                    false);
@@ -6824,7 +6854,6 @@ nsresult nsImapMailFolder::CopyMessagesOffline(
     srcFolder->EnableNotifications(nsIMsgFolder::allMessageCountNotifications,
                                    true);
   }
-
   nsCOMPtr<nsISupports> srcSupport = do_QueryInterface(srcFolder);
   OnCopyCompleted(srcSupport, rv);
 

mailnews/imap/test/unit/test_offlineMoveLocalToIMAP.js

@@ -0,0 +1,125 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/**
+ * Test to check that offline IMAP operation for a local->IMAP message
+ * move completes correctly once we go back online.
+ */
+
+// NOTE: PromiseTestUtils and MailServices already imported
+
+const { MessageGenerator } = ChromeUtils.import(
+  "resource://testing-common/mailnews/MessageGenerator.jsm"
+);
+
+function setupTest() {
+  // Turn off autosync_offline_stores because
+  // fetching messages is invoked after copying the messages.
+  // (i.e. The fetching process will be invoked after OnStopCopy)
+  // It will cause crash with an assertion
+  // (ASSERTION: tried to add duplicate listener: 'index == -1') on teardown.
+
+  Services.prefs.setBoolPref(
+    "mail.server.default.autosync_offline_stores",
+    false
+  );
+
+  setupIMAPPump();
+
+  // These hacks are required because we've created the inbox before
+  // running initial folder discovery, and adding the folder bails
+  // out before we set it as verified online, so we bail out, and
+  // then remove the INBOX folder since it's not verified.
+  IMAPPump.inbox.hierarchyDelimiter = "/";
+  IMAPPump.inbox.verifiedAsOnlineFolder = true;
+}
+
+function teardownTest() {
+  teardownIMAPPump();
+}
+
+function goOffline() {
+  IMAPPump.incomingServer.closeCachedConnections();
+  IMAPPump.server.stop();
+  Services.io.offline = true;
+}
+
+// Go back into online mode, and wait until offline IMAP operations are completed.
+async function goOnline() {
+  IMAPPump.daemon.closing = false;
+  Services.io.offline = false;
+  IMAPPump.server.start();
+
+  let offlineManager = Cc[
+    "@mozilla.org/messenger/offline-manager;1"
+  ].getService(Ci.nsIMsgOfflineManager);
+  offlineManager.goOnline(
+    false, // sendUnsentMessages
+    true, // playbackOfflineImapOperations
+    null // msgWindow
+  );
+  // No way to signal when offline IMAP operations are complete... so we
+  // just blindly wait and cross our fingers :-(
+  await PromiseTestUtils.promiseDelay(2000);
+}
+
+async function loadTestMessage(folder) {
+  let copyListener = new PromiseTestUtils.PromiseCopyListener();
+  let file = do_get_file("../../../data/bugmail1");
+  MailServices.copy.copyFileMessage(
+    file,
+    folder,
+    null,
+    false,
+    0,
+    "",
+    copyListener,
+    null
+  );
+  await copyListener.promise;
+}
+
+add_task(async function testOfflineMoveLocalToIMAP() {
+  setupTest();
+
+  // Install a test message in the local folder.
+  await loadTestMessage(localAccountUtils.inboxFolder);
+
+  goOffline();
+
+  // Move messages in local folder to the IMAP inbox.
+  // We're offline so this should result in a queued-up offline IMAP
+  // operation, which will execute when we go back online.
+  let copyListener = new PromiseTestUtils.PromiseCopyListener();
+  let msgs = [...localAccountUtils.inboxFolder.msgDatabase.EnumerateMessages()];
+  MailServices.copy.copyMessages(
+    localAccountUtils.inboxFolder,
+    msgs,
+    IMAPPump.inbox, // dest folder
+    true, // move
+    copyListener,
+    null,
+    false // undo?
+  );
+  await copyListener.promise;
+
+  // Now, go back online and see if the operation has been performed
+
+  await goOnline();
+
+  let imapINBOX = IMAPPump.inbox.QueryInterface(Ci.nsIMsgImapMailFolder);
+  let listener = new PromiseTestUtils.PromiseUrlListener();
+  imapINBOX.updateFolderWithListener(null, listener);
+  await listener.promise;
+
+  // Local folder should be empty, contents now in IMAP inbox.
+  let localCount = [
+    ...localAccountUtils.inboxFolder.msgDatabase.EnumerateMessages(),
+  ].length;
+  let imapCount = [...IMAPPump.inbox.msgDatabase.EnumerateMessages()].length;
+  Assert.equal(imapCount, msgs.length);
+  Assert.equal(localCount, 0);
+
+  teardownTest();
+});

mailnews/imap/test/unit/xpcshell-cpp.ini

@@ -26,6 +26,7 @@ skip-if = os == 'mac'
 [test_offlineCopy.js]
 [test_offlineDraftDataloss.js]
 [test_offlinePlayback.js]
+[test_offlineMoveLocalToIMAP.js]
 [test_offlineStoreLocking.js]
 [test_partsOnDemand.js]
 [test_preserveDataOnMove.js]

mailnews/imap/test/unit/xpcshell_maildir-cpp.ini

@@ -23,6 +23,7 @@ prefs =
 [test_offlineCopy.js]
 [test_offlineDraftDataloss.js]
 [test_offlinePlayback.js]
+[test_offlineMoveLocalToIMAP.js]
 [test_offlineStoreLocking.js]
 [test_partsOnDemand.js]
 [test_preserveDataOnMove.js]