Bug 342632 - Allow defaultAccount to return success with nullptr result when there is no usable account.

Tag #1273

7 files changed, 94 insertions, 93 deletions

diff --git a/mailnews/base/prefs/content/AccountManager.js b/mailnews/base/prefs/content/AccountManager.js
index 83117f4ad..81fb115de 100644
--- a/mailnews/base/prefs/content/AccountManager.js
+++ b/mailnews/base/prefs/content/AccountManager.js
@@ -704,7 +704,7 @@ function onSetDefault(event) {
   if (event.target.getAttribute("disabled") == "true")
     return;
 
-  let previousDefault = getDefaultAccount();
+  let previousDefault = MailServices.accounts.defaultAccount;
   MailServices.accounts.defaultAccount = currentAccount;
   markDefaultServer(currentAccount, previousDefault);
 
@@ -771,7 +771,7 @@ function onRemoveAccount(event) {
   // Either the default account was deleted so there is a new one
   // or the default account was not changed. Either way, there is
   // no need to unmark the old one.
-  markDefaultServer(getDefaultAccount(), null);
+  markDefaultServer(MailServices.accounts.defaultAccount, null);
 }
 
 function saveAccount(accountValues, account)
@@ -928,7 +928,7 @@ function updateItems(tree, account, addAccountItem, setDefaultItem, removeItem)
     // problem. Either way, we don't want the user to act on it.
     let server = account.incomingServer;
 
-    if (account != getDefaultAccount() &&
+    if (account != MailServices.accounts.defaultAccount &&
         server.canBeDefaultServer && account.identities.length > 0)
       canSetDefault = true;
 
@@ -1379,18 +1379,6 @@ function getCurrentAccount()
 }
 
 /**
- * Returns the default account without throwing exception if there is none.
- * The account manager can be opened even if there are no account yet.
- */
-function getDefaultAccount() {
-  try {
-    return MailServices.accounts.defaultAccount;
-  } catch (e) {
-    return null; // No default account yet.
-  }
-}
-
-/**
  * Get the array of persisted form elements for the given page.
  */
 function getPageFormElements() {
@@ -1605,7 +1593,7 @@ var gAccountTree = {
       treeitem._account = account;
     }
 
-    markDefaultServer(getDefaultAccount(), null);
+    markDefaultServer(MailServices.accounts.defaultAccount, null);
 
     // Now add the outgoing server node.
     var treeitem = document.createElement("treeitem");
diff --git a/mailnews/base/prefs/content/AccountWizard.js b/mailnews/base/prefs/content/AccountWizard.js
index 1903cbc88..90d729418 100644
--- a/mailnews/base/prefs/content/AccountWizard.js
+++ b/mailnews/base/prefs/content/AccountWizard.js
@@ -53,7 +53,9 @@ var gPrefsBundle, gMessengerBundle;
 // the current nsIMsgAccount
 var gCurrentAccount;
 
-// default account
+// The default account before we create a new account.
+// We need to store this as just asking for the default account may switch
+// it to the newly created one if there was none before.
 var gDefaultAccount;
 
 // the current associative array that
@@ -85,14 +87,9 @@ function onAccountWizardLoad() {
 
   checkForInvalidAccounts();
 
-  try {
-    gDefaultAccount = MailServices.accounts.defaultAccount;
-  }
-  catch (ex) {
-    // no default account, this is expected the first time you launch mail
-    // on a new profile
-    gDefaultAccount = null;
-  }
+  // It is fine if there is no default account, this is expected the first
+  // time you launch mail on a new profile.
+  gDefaultAccount = MailServices.accounts.defaultAccount;
 
   // Set default value for global inbox checkbox
   var checkGlobalInbox = document.getElementById("deferStorage");
@@ -799,8 +796,7 @@ function getPreConfigDataForAccount(account)
 
 function AccountToAccountData(account, defaultAccountData)
 {
-  dump("AccountToAccountData(" + account + ", " +
-       defaultAccountData + ")\n");
+  dump("AccountToAccountData(" + account + ", " + defaultAccountData + ")\n");
   var accountData = defaultAccountData;
   if (!accountData)
     accountData = new Object;
@@ -966,10 +962,10 @@ function onFlush() {
   */
 function EnableCheckMailAtStartUpIfNeeded(newAccount)
 {
-  // Check if default account exists and if that account is alllowed to be
-  // a default account. If no such account, make this one as the default account 
+  // Check if default account existed.
+  // If no such account, make this one the default account
   // and turn on the new mail check at startup for the current account   
-  if (!(gDefaultAccount && gDefaultAccount.incomingServer.canBeDefaultServer)) { 
+  if (!gDefaultAccount) {
     MailServices.accounts.defaultAccount = newAccount;
     newAccount.incomingServer.loginAtStartUp = true;
     newAccount.incomingServer.downloadOnBiff = true;
@@ -981,7 +977,7 @@ function SetSmtpRequiresUsernameAttribute(accountData)
   // If this is the default server, time to set the smtp user name
   // Set the generic attribute for requiring user name for smtp to true.
   // ISPs can override the pref via rdf files.
-  if (!(gDefaultAccount && gDefaultAccount.incomingServer.canBeDefaultServer)) { 
+  if (!gDefaultAccount) {
     accountData.smtpRequiresUsername = true;
   }
 }
diff --git a/mailnews/base/public/nsIMsgAccountManager.idl b/mailnews/base/public/nsIMsgAccountManager.idl
index d5a06d2c8..53ea2351e 100644
--- a/mailnews/base/public/nsIMsgAccountManager.idl
+++ b/mailnews/base/public/nsIMsgAccountManager.idl
@@ -71,9 +71,13 @@ interface nsIMsgAccountManager : nsISupports {
 
   /* account list stuff */
 
-  /* defaultAccount should always be set if there are any accounts
-   * in the account manager. You can only set the defaultAccount to an
-   * account already in the account manager */
+  /**
+   * Returns the account that was marked as the default one.
+   * Only some server types can serve as default account.
+   * If there is no such account, null is returned.
+   * You can only set the defaultAccount to an
+   * account already in the account manager.
+   */
   attribute nsIMsgAccount defaultAccount;
 
   /**
diff --git a/mailnews/base/search/content/searchWidgets.xml b/mailnews/base/search/content/searchWidgets.xml
index 80ebe38c4..69f3b2ae2 100644
--- a/mailnews/base/search/content/searchWidgets.xml
+++ b/mailnews/base/search/content/searchWidgets.xml
@@ -224,10 +224,12 @@
             let identitiesRaw = MailServices.accounts
               .getIdentitiesForServer(gFilterList.folder.server);
             let identities = Array.from(this.fixIterator(identitiesRaw,
-                                          Components.interfaces.nsIMsgIdentity));
+                                                         Ci.nsIMsgIdentity));
 
-            if (!identities.length) // typically if this is Local Folders
-              identities.push(MailServices.accounts.defaultAccount.defaultIdentity);
+            if (identities.length == 0) { // typically if this is Local Folders
+              if (MailServices.accounts.defaultAccount)
+                identities.push(MailServices.accounts.defaultAccount.defaultIdentity);
+            }
 
             let templateFound = false;
             let foldersScanned = [];
diff --git a/mailnews/base/src/nsMessengerWinIntegration.cpp b/mailnews/base/src/nsMessengerWinIntegration.cpp
index 98888fcb3..ff0c08041 100644
--- a/mailnews/base/src/nsMessengerWinIntegration.cpp
+++ b/mailnews/base/src/nsMessengerWinIntegration.cpp
@@ -1089,7 +1089,8 @@ nsMessengerWinIntegration::SetupInbox()
 
   nsCOMPtr <nsIMsgAccount> account;
   rv = accountManager->GetDefaultAccount(getter_AddRefs(account));
-  if (NS_FAILED(rv)) {
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (!account) {
     // this can happen if we launch mail on a new profile
     // we don't have a default account yet
     mDefaultAccountMightHaveAnInbox = false;
diff --git a/mailnews/base/src/nsMsgAccountManager.cpp b/mailnews/base/src/nsMsgAccountManager.cpp
index 9dd73d6f9..4ebac58a5 100644
--- a/mailnews/base/src/nsMsgAccountManager.cpp
+++ b/mailnews/base/src/nsMsgAccountManager.cpp
@@ -654,9 +654,9 @@ nsMsgAccountManager::RemoveAccount(nsIMsgAccount *aAccount,
     return rv;
   }
 
-  // if it's the default, clear the default account
+  // If it's the default, choose a new default account.
   if (m_defaultAccount.get() == aAccount)
-    SetDefaultAccount(nullptr);
+    AutosetDefaultAccount();
 
   // XXX - need to figure out if this is the last time this server is
   // being used, and only send notification then.
@@ -727,7 +727,9 @@ nsMsgAccountManager::OutputAccountsPref()
                                 mAccountKeyList.get());
 }
 
-/* get the default account. If no default account, pick the first account */
+/**
+ * Get the default account. If no default account, return null.
+ */
 NS_IMETHODIMP
 nsMsgAccountManager::GetDefaultAccount(nsIMsgAccount **aDefaultAccount)
 {
@@ -737,74 +739,72 @@ nsMsgAccountManager::GetDefaultAccount(nsIMsgAccount **aDefaultAccount)
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (!m_defaultAccount) {
-    uint32_t count = m_accounts.Length();
-    if (!count) {
-      *aDefaultAccount = nullptr;
-      return NS_ERROR_FAILURE;
-    }
-
     nsCString defaultKey;
     rv = m_prefs->GetCharPref(PREF_MAIL_ACCOUNTMANAGER_DEFAULTACCOUNT, getter_Copies(defaultKey));
 
-    if (NS_SUCCEEDED(rv))
+    if (NS_SUCCEEDED(rv)) {
       rv = GetAccount(defaultKey, getter_AddRefs(m_defaultAccount));
-
-    if (NS_FAILED(rv) || !m_defaultAccount) {
-      nsCOMPtr<nsIMsgAccount> firstAccount;
-      uint32_t index;
-      bool foundValidDefaultAccount = false;
-      for (index = 0; index < count; index++) {
-        nsCOMPtr<nsIMsgAccount> account(m_accounts[index]);
-
-        // get incoming server
-        nsCOMPtr <nsIMsgIncomingServer> server;
-        // server could be null if created by an unloaded extension
-        (void) account->GetIncomingServer(getter_AddRefs(server));
-
-        bool canBeDefaultServer = false;
-        if (server)
-        {
-          server->GetCanBeDefaultServer(&canBeDefaultServer);
-          if (!firstAccount)
-            firstAccount = account;
-        }
-
-        // if this can serve as default server, set it as default and
-        // break out of the loop.
-        if (canBeDefaultServer) {
-          SetDefaultAccount(account);
-          foundValidDefaultAccount = true;
-          break;
-        }
-      }
-
-      if (!foundValidDefaultAccount) {
-        // Get the first account and use it.
-        // We need to fix this scenario, e.g. in bug 342632.
-        NS_WARNING("No valid default account found.");
-        if (firstAccount) {
-          NS_WARNING("Just using the first one (FIXME).");
-          SetDefaultAccount(firstAccount);
-        }
+      if (NS_SUCCEEDED(rv) && m_defaultAccount) {
+        bool canBeDefault = false;
+        rv = CheckDefaultAccount(m_defaultAccount, canBeDefault);
+        if (NS_FAILED(rv) || !canBeDefault)
+          m_defaultAccount = nullptr;
       }
     }
   }
 
-  if (!m_defaultAccount) {
-    // Absolutely no usable account found. Error out.
-    NS_ERROR("Default account is null, when not expected!");
-    *aDefaultAccount = nullptr;
-    return NS_ERROR_FAILURE;
+  NS_IF_ADDREF(*aDefaultAccount = m_defaultAccount);
+  return NS_OK;
+}
+
+/**
+ * Check if the given account can be default.
+ */
+nsresult
+nsMsgAccountManager::CheckDefaultAccount(nsIMsgAccount *aAccount, bool &aCanBeDefault)
+{
+  aCanBeDefault = false;
+  nsCOMPtr<nsIMsgIncomingServer> server;
+  // Server could be null if created by an unloaded extension.
+  nsresult rv = aAccount->GetIncomingServer(getter_AddRefs(server));
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (server) {
+    // Check if server can be default.
+    rv = server->GetCanBeDefaultServer(&aCanBeDefault);
+  }
+  return rv;
+}
+
+/**
+ * Pick the first account that can be default and make it the default.
+ */
+nsresult
+nsMsgAccountManager::AutosetDefaultAccount()
+{
+  for (nsIMsgAccount* account : m_accounts) {
+    bool canBeDefault = false;
+    nsresult rv = CheckDefaultAccount(account, canBeDefault);
+    if (NS_SUCCEEDED(rv) && canBeDefault) {
+      return SetDefaultAccount(account);
+    }
   }
-  NS_ADDREF(*aDefaultAccount = m_defaultAccount);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsMsgAccountManager::SetDefaultAccount(nsIMsgAccount *aDefaultAccount)
 {
+  if (!aDefaultAccount)
+    return NS_ERROR_INVALID_ARG;
+
   if (m_defaultAccount != aDefaultAccount)
   {
+    bool canBeDefault = false;
+    nsresult rv = CheckDefaultAccount(aDefaultAccount, canBeDefault);
+    if (NS_FAILED(rv) || !canBeDefault) {
+      // Report failure if we were explicitly asked to use an unusable server.
+      return NS_ERROR_INVALID_ARG;
+    }
     nsCOMPtr<nsIMsgAccount> oldAccount = m_defaultAccount;
     m_defaultAccount = aDefaultAccount;
     (void) setDefaultAccountPref(aDefaultAccount);
@@ -3677,7 +3677,7 @@ nsMsgAccountManager::GetSortOrder(nsIMsgIncomingServer* aServer, int32_t* aSortO
   if (NS_SUCCEEDED(rv) && defaultAccount) {
     nsCOMPtr<nsIMsgIncomingServer> defaultServer;
     rv = m_defaultAccount->GetIncomingServer(getter_AddRefs(defaultServer));
-    if (NS_SUCCEEDED(rv) && defaultServer && (aServer == defaultServer)) {
+    if (NS_SUCCEEDED(rv) && (aServer == defaultServer)) {
       *aSortOrder = 0;
       return NS_OK;
     }
diff --git a/mailnews/base/src/nsMsgAccountManager.h b/mailnews/base/src/nsMsgAccountManager.h
index d5a116e57..11d19590b 100644
--- a/mailnews/base/src/nsMsgAccountManager.h
+++ b/mailnews/base/src/nsMsgAccountManager.h
@@ -142,6 +142,16 @@ private:
 
   nsresult GetLocalFoldersPrettyName(nsString &localFoldersName);
 
+  /**
+   * Check if the given account can be the set as the default account.
+   */
+  nsresult CheckDefaultAccount(nsIMsgAccount* aAccount, bool &aCanBeDefault);
+
+  /**
+   * Find a new account that can serve as default.
+   */
+  nsresult AutosetDefaultAccount();
+
   // sets the pref for the default server
   nsresult setDefaultAccountPref(nsIMsgAccount *aDefaultAccount);