Bug 1560014 - Make nsMsgAccountManager::RemoveAccount() return error when given an already removed account. r=mkmelin
authoraceman <acelists@atlas.sk>
Sun, 23 Jun 2019 23:56:35 +0200
changeset 35929 70cded2918d3b83c273da5a55a72f91ce7b2a442
parent 35928 f7382be702d4214d3f3046576bd8bdaa011f6be5
child 35930 0b65696ec2396a59b8cc85a601a0baffc0b0a5a0
push id392
push userclokep@gmail.com
push dateMon, 02 Sep 2019 20:17:19 +0000
reviewersmkmelin
bugs1560014
Bug 1560014 - Make nsMsgAccountManager::RemoveAccount() return error when given an already removed account. r=mkmelin
mail/components/extensions/test/browser/head.js
mail/components/extensions/test/xpcshell/head.js
mail/test/mozmill/account/test-account-deletion.js
mail/test/mozmill/account/test-account-tree.js
mail/test/mozmill/account/test-mail-account-setup-wizard.js
mail/test/mozmill/composition/test-draft-identity.js
mail/test/mozmill/instrumentation/test-instrument-setup.js
mail/test/mozmill/shared-modules/test-account-manager-helpers.js
mailnews/base/prefs/content/removeAccount.js
mailnews/base/src/nsMsgAccountManager.cpp
mailnews/imap/test/unit/test_imapAuthMethods.js
mailnews/test/resources/IMAPpump.js
--- a/mail/components/extensions/test/browser/head.js
+++ b/mail/components/extensions/test/browser/head.js
@@ -24,17 +24,16 @@ function createAccount() {
   let account = MailServices.accounts.accounts.enumerate().getNext();
   info(`Created account ${account.toString()}`);
 
   return account;
 }
 
 function cleanUpAccount(account) {
   info(`Cleaning up account ${account.toString()}`);
-  MailServices.accounts.removeIncomingServer(account.incomingServer, true);
   MailServices.accounts.removeAccount(account, true);
 }
 
 function addIdentity(account) {
   let identity = MailServices.accounts.createIdentity();
   identity.email = "mochitest@localhost";
   account.addIdentity(identity);
   account.defaultIdentity = identity;
--- a/mail/components/extensions/test/xpcshell/head.js
+++ b/mail/components/extensions/test/xpcshell/head.js
@@ -20,17 +20,16 @@ function createAccount() {
   account.incomingServer = MailServices.accounts.localFoldersServer;
   info(`Created account ${account.toString()}`);
 
   return account;
 }
 
 function cleanUpAccount(account) {
   info(`Cleaning up account ${account.toString()}`);
-  MailServices.accounts.removeIncomingServer(account.incomingServer, true);
   MailServices.accounts.removeAccount(account, true);
 }
 
 registerCleanupFunction(() => {
   [...MailServices.accounts.accounts.enumerate()].forEach(cleanUpAccount);
 });
 
 function createMessages(folder, count) {
--- a/mail/test/mozmill/account/test-account-deletion.js
+++ b/mail/test/mozmill/account/test-account-deletion.js
@@ -19,17 +19,17 @@ var RELATIVE_ROOT = "../shared-modules";
 var MODULE_REQUIRES = [
   "folder-display-helpers",
   "window-helpers",
   "account-manager-helpers",
   "content-tab-helpers",
   "pref-window-helpers",
 ];
 
-var gPopAccount, gImapAccount, gNntpAccount, gOriginalAccountCount;
+var gPopAccount, gImapAccount, gOriginalAccountCount;
 
 function setupModule(module) {
   for (let lib of MODULE_REQUIRES) {
     collector.getModule(lib).installInto(module);
   }
 
   // There may be pre-existing accounts from other tests.
   gOriginalAccountCount = MailServices.accounts.allServers.length;
@@ -78,16 +78,17 @@ function test_account_data_deletion1() {
   assert_true(accountDir.isDirectory());
 
   // Get some existing file in the POP3 account data dir.
   let inboxFile = accountDir.clone();
   inboxFile.append("Inbox.msf");
   assert_true(inboxFile.isFile());
 
   remove_account(gPopAccount, tab, true, false);
+  gPopAccount = null;
   assert_true(accountDir.exists());
 
   close_advanced_settings(tab);
 }
 
 /**
  * Bug 274452
  * Check if files of an account can be deleted.
@@ -100,12 +101,13 @@ function test_account_data_deletion2() {
   assert_true(accountDir.isDirectory());
 
   // Get some file in the IMAP account data dir.
   let inboxFile = accountDir.clone();
   inboxFile.append("INBOX.msf");
   assert_true(inboxFile.isFile());
 
   remove_account(gImapAccount, tab, true, true);
+  gImapAccount = null;
   assert_false(accountDir.exists());
 
   close_advanced_settings(tab);
 }
--- a/mail/test/mozmill/account/test-account-tree.js
+++ b/mail/test/mozmill/account/test-account-tree.js
@@ -46,18 +46,21 @@ function setupModule(module) {
   gPopAccount.incomingServer = popServer;
   gPopAccount.addIdentity(identity);
 
   // Now there should be one more account.
   assert_equals(MailServices.accounts.allServers.length, gOriginalAccountCount + 1);
 }
 
 function teardownModule(module) {
-  // Remove our test account to leave the profile clean.
-  MailServices.accounts.removeAccount(gPopAccount);
+  if (gPopAccount) {
+    // Remove our test account to leave the profile clean.
+    MailServices.accounts.removeAccount(gPopAccount);
+    gPopAccount = null;
+  }
   // There should be only the original accounts left.
   assert_equals(MailServices.accounts.allServers.length, gOriginalAccountCount);
 }
 
 /**
  * Test for bug 536248.
  * Check if the account manager dialog remembers the open state of accounts.
  */
@@ -151,16 +154,17 @@ function test_selection_after_account_de
     }
   }
 
   // Get position of the current account in the account list.
   let accountIndex = accountList.indexOf(gPopAccount);
 
   // Remove our account.
   remove_account(gPopAccount, tab);
+  gPopAccount = null;
   // Now there should be only the original accounts left.
   assert_equals(MailServices.accounts.allServers.length, gOriginalAccountCount);
 
   // See if the currently selected account is the one next in the account list.
   let accountTree = content_tab_e(tab, "accounttree");
   let accountRow = accountTree.view.selection.currentIndex;
   wait_for_account_tree_selection(tab, accountRow);
   assert_equals(accountTree.view.getItemAtIndex(accountRow)._account,
--- a/mail/test/mozmill/account/test-mail-account-setup-wizard.js
+++ b/mail/test/mozmill/account/test-mail-account-setup-wizard.js
@@ -40,23 +40,25 @@ function setupModule(module) {
 // Remove an account in the Account Manager, but not via the UI.
 function remove_account_internal(tab, aAccount, aOutgoing) {
   let win = tab.browser.contentWindow;
 
   try {
     // Remove the account and incoming server
     let serverId = aAccount.incomingServer.serverURI;
     MailServices.accounts.removeAccount(aAccount);
+    aAccount = null;
     if (serverId in win.accountArray)
       delete win.accountArray[serverId];
     win.selectServer(null, null);
 
     // Remove the outgoing server
+    let smtpKey = aOutgoing.key;
     MailServices.smtp.deleteServer(aOutgoing);
-    win.replaceWithDefaultSmtpServer(aOutgoing.key);
+    win.replaceWithDefaultSmtpServer(smtpKey);
   } catch (ex) {
     throw new Error("failure to remove account: " + ex + "\n");
   }
 }
 
 function test_mail_account_setup() {
   // Set the pref to load a local autoconfig file.
   let pref_name = "mailnews.auto_config_url";
--- a/mail/test/mozmill/composition/test-draft-identity.js
+++ b/mail/test/mozmill/composition/test-draft-identity.js
@@ -217,16 +217,17 @@ function teardownModule(module) {
   for (let id = 1; id < gIdentities.length; id++) {
     gAccount.removeIdentity(MailServices.accounts.getIdentity(gIdentities[id].key));
   }
 
   // The last identity of an account can't be removed so clear all its prefs
   // which effectively destroys it.
   MailServices.accounts.getIdentity(gIdentities[0].key).clearAllValues();
   MailServices.accounts.removeAccount(gAccount);
+  gAccount = null;
 
   // Clear our drafts.
   be_in_folder(gDrafts);
   let draftCount;
   while ((draftCount = gDrafts.getTotalMessages(false)) > 0) {
     press_delete();
     mc.waitFor(() => (gDrafts.getTotalMessages(false) < draftCount));
   }
--- a/mail/test/mozmill/instrumentation/test-instrument-setup.js
+++ b/mail/test/mozmill/instrumentation/test-instrument-setup.js
@@ -69,16 +69,15 @@ function test_mail_account_setup() {
 
 // Remove the accounts we added.
 function tearDownModule(module) {
   let incomingServer = MailServices.accounts.FindServer("roger.sterling", user.incomingHost, "pop3");
   assert_equals(incomingServer.hostName, user.incomingHost);
   let account = MailServices.accounts.FindAccountForServer(incomingServer);
 
   let identity = account.defaultIdentity;
-  MailServices.accounts.removeIncomingServer(incomingServer, true);
   let outgoingServer = MailServices.smtp.getServerByKey(identity.smtpServerKey);
   assert_equals(outgoingServer.hostname, user.outgoingHost);
   MailServices.smtp.deleteServer(outgoingServer);
-  MailServices.accounts.removeAccount(account);
+  MailServices.accounts.removeAccount(account, true);
 
   Services.prefs.clearUserPref("mailnews.auto_config_url");
 }
--- a/mail/test/mozmill/shared-modules/test-account-manager-helpers.js
+++ b/mail/test/mozmill/shared-modules/test-account-manager-helpers.js
@@ -207,16 +207,17 @@ function remove_account(aAccount, tab, a
     cdc.window.document.documentElement.acceptDialog();
   });
 
   let treeBuilt = false;
   cth.content_tab_e(tab, "account-tree-children")
      .addEventListener("account-tree-built", () => { treeBuilt = true; },
                        {once: true});
 
+  aAccount = null;
   // Use the Remove item in the Account actions menu.
   mc.click(cth.content_tab_eid(tab, "accountActionsButton"));
   mc.click_menus_in_sequence(cth.content_tab_e(tab, "accountActionsDropdown"),
                                       [ {id: "accountActionsDropdownRemove"} ]);
   wh.wait_for_modal_dialog("removeAccountDialog");
   mc.waitFor(() => treeBuilt, "Timeout waiting for account tree rebuild");
   wait_for_account_tree_selection(tab);
 }
--- a/mailnews/base/prefs/content/removeAccount.js
+++ b/mailnews/base/prefs/content/removeAccount.js
@@ -102,16 +102,19 @@ function removeAccount() {
     // Remove the requested account data.
     if (removeAccount) {
       try {
         // Remove password information first.
         account.incomingServer.forgetPassword();
       } catch (e) { /* It is OK if this fails. */ }
       // Remove account
       MailServices.accounts.removeAccount(account, removeData);
+      account = null;
+      delete window.arguments[0].account;
+      gServer = null;
       window.arguments[0].result = true;
     } else if (removeData) {
       // Remove files only.
       // TODO: bug 1302193
       window.arguments[0].result = false;
     }
 
     document.getElementById("status").selectedPanel =
--- a/mailnews/base/src/nsMsgAccountManager.cpp
+++ b/mailnews/base/src/nsMsgAccountManager.cpp
@@ -583,20 +583,21 @@ void nsMsgAccountManager::removeListener
 NS_IMETHODIMP
 nsMsgAccountManager::RemoveAccount(nsIMsgAccount *aAccount,
                                    bool aRemoveFiles = false) {
   NS_ENSURE_ARG_POINTER(aAccount);
   nsresult rv = LoadAccounts();
   NS_ENSURE_SUCCESS(rv, rv);
 
   bool accountRemoved = m_accounts.RemoveElement(aAccount);
+  if (!accountRemoved) return NS_ERROR_INVALID_ARG;
 
   rv = OutputAccountsPref();
   // If we couldn't write out the pref, restore the account.
-  if (NS_FAILED(rv) && accountRemoved) {
+  if (NS_FAILED(rv)) {
     m_accounts.AppendElement(aAccount);
     return rv;
   }
 
   // If it's the default, choose a new default account.
   if (m_defaultAccount.get() == aAccount) AutosetDefaultAccount();
 
   // XXX - need to figure out if this is the last time this server is
--- a/mailnews/imap/test/unit/test_imapAuthMethods.js
+++ b/mailnews/imap/test/unit/test_imapAuthMethods.js
@@ -115,37 +115,36 @@ function nextTest() {
     // do_check_eq(thisTest.expectSuccess,
     //    rootFolder.containsChildNamed("somemailbox")); TODO
     do_check_transaction(server.playTransaction(), thisTest.transaction, false);
 
     do {
       incomingServer.closeCachedConnections();
     } while (incomingServer.serverBusy);
     incomingServer.shutdown();
-    incomingServer.clearAllValues();
     deleteIMAPServer(incomingServer);
+    incomingServer = null;
     MailServices.accounts.closeCachedConnections();
     MailServices.accounts.shutdownServers();
     MailServices.accounts.UnloadAccounts();
     server.stop();
   } catch (e) {
     // server.stop();
     // endTest();
     do_throw(e);
   }
 
   nextTest();
 }
 
 function deleteIMAPServer(incomingServer) {
   if (!incomingServer)
     return;
-  MailServices.accounts.removeIncomingServer(incomingServer, false); // TODO cleanup files = true fails
-  // incomingServer = null;
-  MailServices.accounts.removeAccount(MailServices.accounts.defaultAccount);
+//  MailServices.accounts.removeIncomingServer(incomingServer, false); // TODO cleanup files = true fails
+  MailServices.accounts.removeAccount(MailServices.accounts.defaultAccount, true);
 }
 
 
 function run_test() {
   do_test_pending();
 
   registerAlertTestUtils();
 
--- a/mailnews/test/resources/IMAPpump.js
+++ b/mailnews/test/resources/IMAPpump.js
@@ -124,11 +124,12 @@ function teardownIMAPPump() {
   IMAPPump.inbox = null;
   try {
     let serverSink = IMAPPump.incomingServer.QueryInterface(Ci.nsIImapServerSink);
     serverSink.abortQueuedUrls();
     IMAPPump.incomingServer.closeCachedConnections();
     IMAPPump.server.resetTest();
     IMAPPump.server.stop();
     MailServices.accounts.removeIncomingServer(IMAPPump.incomingServer, false);
+    IMAPPump.incomingServer = null;
     localAccountUtils.clearAll();
   } catch (ex) { dump(ex); }
 }