Bug 831190 - Check for empty folder name in createSubfolder() of local message stores. r=Neil, r=Standard8

mail/base/content/folderPane.js

@@ -1945,6 +1945,10 @@ let gFolderTreeController = {
       dualUseFolders = folder.server.dualUseFolders;
 
     function newFolderCallback(aName, aFolder) {
+      // createSubfolder can throw an exception, causing the newFolder dialog
+      // to not close and wait for another input.
+      // TODO: Rewrite this logic and also move the opening of alert dialogs from
+      // nsMsgLocalMailFolder::CreateSubfolderInternal to here (bug 831190#c16).
       if (aName)
         aFolder.createSubfolder(aName, msgWindow);
     }

mailnews/imap/src/nsImapMailFolder.cpp

@@ -855,7 +855,9 @@ NS_IMETHODIMP nsImapMailFolder::GetMessages(nsISimpleEnumerator* *result)
 
 NS_IMETHODIMP nsImapMailFolder::CreateSubfolder(const nsAString& folderName, nsIMsgWindow *msgWindow)
 {
-  NS_ENSURE_TRUE(!folderName.IsEmpty(), NS_ERROR_FAILURE);
+  if (folderName.IsEmpty())
+    return NS_MSG_ERROR_INVALID_FOLDER_NAME;
+
   nsresult rv;
   nsAutoString trashName;
   GetTrashFolderName(trashName);

mailnews/local/src/nsLocalMailFolder.cpp

@@ -558,19 +558,15 @@ nsMsgLocalMailFolder::CreateSubfolderInternal(const nsAString& folderName,
   if (rv == NS_MSG_ERROR_INVALID_FOLDER_NAME)
   {
     ThrowAlertMsg("folderCreationFailed", msgWindow);
-    // I'm returning this value so the dialog stays up
-    return NS_MSG_FOLDER_EXISTS;
   }
-  if (rv == NS_MSG_FOLDER_EXISTS)
+  else if (rv == NS_MSG_FOLDER_EXISTS)
   {
     ThrowAlertMsg("folderExists", msgWindow);
-    return NS_MSG_FOLDER_EXISTS;
   }
 
-  nsCOMPtr<nsIMsgFolder> child = *aNewFolder;
-
   if (NS_SUCCEEDED(rv))
   {
+    nsCOMPtr<nsIMsgFolder> child = *aNewFolder;
     //we need to notify explicitly the flag change because it failed when we did AddSubfolder
     child->OnFlagChange(mFlags);
     child->SetPrettyName(folderName);  //because empty trash will create a new trash folder
@@ -578,6 +574,7 @@ nsMsgLocalMailFolder::CreateSubfolderInternal(const nsAString& folderName,
     if (aNewFolder)
       child.swap(*aNewFolder);
   }
+
   return rv;
 }
 

mailnews/local/src/nsMsgBrkMBoxStore.cpp

@@ -70,6 +70,11 @@ NS_IMETHODIMP nsMsgBrkMBoxStore::CreateFolder(nsIMsgFolder *aParent,
                                               const nsAString &aFolderName,
                                               nsIMsgFolder **aResult)
 {
+  NS_ENSURE_ARG_POINTER(aParent);
+  NS_ENSURE_ARG_POINTER(aResult);
+  if (aFolderName.IsEmpty())
+    return NS_MSG_ERROR_INVALID_FOLDER_NAME;
+
   nsCOMPtr<nsIFile> path;
   nsCOMPtr<nsIMsgFolder> child;
   nsresult rv = aParent->GetFilePath(getter_AddRefs(path));

mailnews/local/src/nsMsgMaildirStore.cpp

@@ -170,6 +170,11 @@ NS_IMETHODIMP nsMsgMaildirStore::CreateFolder(nsIMsgFolder *aParent,
                                               const nsAString &aFolderName,
                                               nsIMsgFolder **aResult)
 {
+  NS_ENSURE_ARG_POINTER(aParent);
+  NS_ENSURE_ARG_POINTER(aResult);
+  if (aFolderName.IsEmpty())
+    return NS_MSG_ERROR_INVALID_FOLDER_NAME;
+
   nsCOMPtr <nsIFile> path;
   nsresult rv = aParent->GetFilePath(getter_AddRefs(path));
   NS_ENSURE_SUCCESS(rv, rv);

mailnews/local/test/unit/test_fileName.js

@@ -4,11 +4,25 @@
  */
 
 Components.utils.import("resource:///modules/mailServices.js");
-
-var server;
+Components.utils.import("resource://gre/modules/Services.jsm");
 
 function run_test() {
-  // test file with ':' in the name (generated from Mozilla 1.8 branch).
+  let testFolderName = "";
+  let OSname = Cc["@mozilla.org/system-info;1"].getService(Ci.nsIPropertyBag2).getProperty("name");
+  if (OSname == "Windows_NT") {
+    // On Windows test file with ' ' in the name.
+    testFolderName = "bugmail 1";
+  } else if (OSname == "Linux") {
+    // On Linux test file with '`' in the name.
+    testFolderName = "bugmail`1";
+  } else if (OSname == "Darwin") {
+    // On Mac test file with ':' in the name (generated from Mozilla 1.8 branch).
+    testFolderName = "bugmail:1";
+  } else {
+    // Not sure what this OS is so just use a safe name.
+    testFolderName = "bugmail1";
+  }
+
   let bugmail = do_get_file("../../../data/bugmail-1");
   let bugmailmsf = do_get_file("../../../data/bugmail-1.msf");
   let localMailDir = do_get_profile().clone();
@@ -18,12 +32,12 @@ function run_test() {
   pop3dir.append("Mail");
   pop3dir.append("poptest");
   // Copy the file to the local mail directory
-  bugmail.copyTo(localMailDir, "bugmail:1");
-  bugmailmsf.copyTo(localMailDir, "bugmail:1.msf");
+  bugmail.copyTo(localMailDir, testFolderName);
+  bugmailmsf.copyTo(localMailDir, testFolderName + ".msf");
 
   // Copy the file to the pop3 server mail directory
-  bugmail.copyTo(pop3dir, "bugmail:1");
-  bugmailmsf.copyTo(pop3dir, "bugmail:1.msf");
+  bugmail.copyTo(pop3dir, testFolderName);
+  bugmailmsf.copyTo(pop3dir, testFolderName + ".msf");
 
   // These preferences set up a local folders account so we'll use the
   // contents of the Local Folders dir we've already pre-populated.
@@ -50,25 +64,44 @@ function run_test() {
   Services.prefs.setCharPref("mail.server.server2.userName", "user");
   // This basically says to ignore the time stamp in the .msf file
   Services.prefs.setIntPref("mail.db_timestamp_leeway", 0x7FFFFFFF);
-  
+
   localAccountUtils.incomingServer = MailServices.accounts.localFoldersServer;
   // force load of accounts.
   let defaultAccount = MailServices.accounts.defaultAccount;
 
   let pop3Server = MailServices.accounts.FindServer("user", "poptest", "pop3");
-  var rootFolder = localAccountUtils.incomingServer.rootMsgFolder
-    .QueryInterface(Ci.nsIMsgLocalMailFolder);
+  let rootFolder = localAccountUtils.incomingServer.rootMsgFolder
+                                    .QueryInterface(Ci.nsIMsgLocalMailFolder);
   let pop3Root = pop3Server.rootMsgFolder;
-  
+
   // Note: Inbox is not created automatically when there is no deferred server,
   // so we need to create it.
   localAccountUtils.inboxFolder = rootFolder.createLocalSubfolder("Inbox");
   // a local inbox should have a Mail flag!
   localAccountUtils.inboxFolder.setFlag(Ci.nsMsgFolderFlags.Mail);
-  
+
   let rootFolder = localAccountUtils.incomingServer.rootMsgFolder;
-  let bugmail = rootFolder.getChildNamed("bugmail:1");
+  let bugmail = rootFolder.getChildNamed(testFolderName);
   do_check_eq(bugmail.getTotalMessages(false), 1);
-  bugmail = pop3Root.getChildNamed("bugmail:1");
+  bugmail = pop3Root.getChildNamed(testFolderName);
   do_check_eq(bugmail.getTotalMessages(false), 1);
+
+  // Check if creating an empty folder returns a proper error
+  // instead of crash (bug 831190).
+  try {
+    rootFolder.createSubfolder("", null);
+    do_throw("createSubfolder() should have failed on empty folder name.");
+  } catch (e) {
+    // NS_MSG_ERROR_INVALID_FOLDER_NAME
+    do_check_eq(e.result, 2153054242);
+  }
+
+  // And try to create an existing folder again.
+  try {
+    rootFolder.createSubfolder(testFolderName, null);
+    do_throw("createSubfolder() should have failed on existing folder.");
+  } catch (e) {
+    // NS_MSG_FOLDER_EXISTS
+    do_check_eq(e.result, 2153054227);
+  }
 }

mailnews/local/test/unit/xpcshell.ini

@@ -6,10 +6,6 @@ support-files = data/*
 
 [test_bug457168.js]
 [test_fileName.js]
-# Currently, we're only doing a mac-specific test. If we extend
-# this test to include other platforms, we'd need to only do the
-# ':' file name test on the mac.
-skip-if = os != "mac"
 [test_folderLoaded.js]
 [test_localFolder.js]
 [test_mailboxContentLength.js]

mailnews/news/src/nsNewsFolder.cpp

@@ -516,7 +516,7 @@ NS_IMETHODIMP nsMsgNewsFolder::CreateSubfolder(const nsAString& newsgroupName,
 {
   nsresult rv = NS_OK;
   if (newsgroupName.IsEmpty())
-    return NS_ERROR_FAILURE;
+    return NS_MSG_ERROR_INVALID_FOLDER_NAME;
 
   nsCOMPtr<nsIMsgFolder> child;
   // Create an empty database for this mail folder, set its name from the user