Bug 1202105 - Prevent duplicate key when creating new message header. r=aceman

mailnews/local/src/nsMsgBrkMBoxStore.cpp

@@ -652,12 +652,23 @@ nsMsgBrkMBoxStore::GetNewMsgOutputStream(nsIMsgFolder *aFolder,
   }
   int64_t filePos;
   seekable->Tell(&filePos);
+
   if (db && !*aNewMsgHdr)
   {
-    // if mbox is close to 4GB, auto-assign the msg key.
-    nsMsgKey key = filePos > 0xFFFFFF00 ? nsMsgKey_None : (nsMsgKey) filePos;
+    nsMsgKey key = nsMsgKey_None;
+    // The key should not need to be the filePos anymore, but out of caution
+    // we will continue setting key to filePos for mboxes smaller than 0xFF000000.
+    if (filePos <= 0xFF000000)
+    {
+      // After compact, we can have duplicated keys (see bug 1202105) so only
+      // set key to filePos if there is no collision.
+      bool hasKey = true;
+      if (NS_SUCCEEDED(db->ContainsKey((nsMsgKey) filePos, &hasKey)) && !hasKey)
+        key = (nsMsgKey) filePos;
+    }
     db->CreateNewHdr(key, aNewMsgHdr);
   }
+
   if (*aNewMsgHdr)
   {
     char storeToken[100];

mailnews/local/src/nsParseMailbox.cpp

@@ -1393,6 +1393,11 @@ nsresult nsParseMailMessageState::FinalizeHeaders()
       ret = m_backupMailDB->GetMsgHdrForMessageID(
               rawMsgId.get(), getter_AddRefs(oldHeader));
 
+    // m_envelope_pos is set in nsImapMailFolder::ParseAdoptedHeaderLine to be
+    // the UID of the message, so that the key can get created as UID. That of
+    // course is extremely confusing, and we really need to clean that up. We
+    // really should not conflate the meaning of envelope position, key, and
+    // UID.
     if (NS_SUCCEEDED(ret) && oldHeader)
         ret = m_mailDB->CopyHdrFromExistingHdr(m_envelope_pos,
                 oldHeader, false, getter_AddRefs(m_newMsgHdr));

mailnews/local/test/unit/test_duplicateKey.js

@@ -0,0 +1,77 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * This test deletes intermediate messages, then compacts, then adds more
+ * messages, testing for duplicated keys in bug 1202105.
+ */
+
+load("../../../resources/POP3pump.js");
+Components.utils.import("resource://testing-common/mailnews/PromiseTestUtils.jsm");
+
+add_task(function* runPump() {
+
+  gPOP3Pump.files = ["../../../data/bugmail1",
+                     "../../../data/bugmail1",
+                     "../../../data/bugmail1",
+                     "../../../data/bugmail1",
+                     "../../../data/bugmail1"];
+  yield gPOP3Pump.run();
+
+  // get message headers for the inbox folder
+  var hdrs = showMessages(localAccountUtils.inboxFolder);
+  Assert.equal(hdrs.length, 5, "Check initial db count");
+
+  // Deletes 2 middle messages.
+  let deletes = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
+  deletes.appendElement(hdrs[1], false);
+  deletes.appendElement(hdrs[2], false);
+
+  // Note the listener won't work because this is a sync delete,
+  // but it should!
+  localAccountUtils.inboxFolder
+                   .deleteMessages(deletes, // in nsIArray messages,
+                                   null, //in nsIMsgWindow msgWindow,
+                                   true, // in boolean deleteStorage,
+                                   true, // in boolean isMove,
+                                   null, // in nsIMsgCopyServiceListener,
+                                   false); //in boolean allowUndo
+
+  dump("Messages after delete\n");
+  hdrs = showMessages(localAccountUtils.inboxFolder);
+  Assert.equal(hdrs.length, 3, "Check db length after deleting two messages");
+
+  // compact
+  var listener = new PromiseTestUtils.PromiseUrlListener();
+  localAccountUtils.inboxFolder.compact(listener, null);
+  yield listener.promise;
+
+  dump("Messages after compact\n");
+  hdrs = showMessages(localAccountUtils.inboxFolder);
+  Assert.equal(hdrs.length, 3, "Check db length after compact");
+
+  // Add some more messages. This fails in nsMsgDatabase::AddNewHdrToDB with
+  // NS_ERROR("adding hdr that already exists") before bug 1202105.
+  gPOP3Pump.files = ["../../../data/draft1"];
+  yield gPOP3Pump.run();
+
+  dump("Messages after new message\n");
+  hdrs = showMessages(localAccountUtils.inboxFolder);
+  Assert.equal(hdrs.length, 4, "Check db length after adding one message");
+
+  gPOP3Pump = null;
+});
+
+function run_test() {
+  run_next_test();
+}
+
+function showMessages(folder) {
+  let enumerator = folder.msgDatabase.EnumerateMessages();
+  var hdrs = [];
+  while (enumerator.hasMoreElements()) {
+    hdrs.push(enumerator.getNext().QueryInterface(Ci.nsIMsgDBHdr));
+    dump("key " + (hdrs.length - 1) + " is " + hdrs[hdrs.length - 1].messageKey + "\n");
+  }
+  return hdrs;
+}

mailnews/local/test/unit/xpcshell.ini

@@ -4,6 +4,7 @@ tail = tail_local.js
 support-files = data/*
 
 [test_bug457168.js]
+[test_duplicateKey.js]
 [test_fileName.js]
 [test_folderLoaded.js]
 [test_localFolder.js]