Bug 1202105 - Prevent duplicate key when creating new message header. r=aceman, a=rkent
authorR Kent James <rkent@caspia.com>
Thu, 08 Oct 2015 12:49:00 +0200
changeset 22233 3d8ac06bccccb9c97ef22d06289f50e0538f1676
parent 22232 e839c236d4998c949be461b8b183c45276a466ef
child 22234 f0827fae517d0d592a97e0ce3454320b9ad37ddd
child 22236 cec10a3287c5e7624a7a6a3a3bdf9b2cdfae902a
push id85
push userkent@caspia.com
push dateMon, 08 Feb 2016 23:38:37 +0000
treeherdercomm-esr38@3d8ac06bcccc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaceman, rkent
bugs1202105
Bug 1202105 - Prevent duplicate key when creating new message header. r=aceman, a=rkent
mailnews/local/src/nsMsgBrkMBoxStore.cpp
mailnews/local/src/nsParseMailbox.cpp
mailnews/local/test/unit/test_duplicateKey.js
mailnews/local/test/unit/xpcshell.ini
--- a/mailnews/local/src/nsMsgBrkMBoxStore.cpp
+++ b/mailnews/local/src/nsMsgBrkMBoxStore.cpp
@@ -647,22 +647,33 @@ nsMsgBrkMBoxStore::GetNewMsgOutputStream
     seekable = do_QueryInterface(*aResult, &rv);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = seekable->Seek(nsISeekableStream::NS_SEEK_END, 0);
     NS_ENSURE_SUCCESS(rv, rv);
     m_outputStreams.Put(URI, *aResult);
   }
   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];
     PR_snprintf(storeToken, sizeof(storeToken), "%lld", filePos);
     (*aNewMsgHdr)->SetMessageOffset(filePos);
     (*aNewMsgHdr)->SetStringProperty("storeToken", storeToken);
   }
   return rv;
--- a/mailnews/local/src/nsParseMailbox.cpp
+++ b/mailnews/local/src/nsParseMailbox.cpp
@@ -1386,16 +1386,21 @@ nsresult nsParseMailMessageState::Finali
      */
     nsCOMPtr<nsIMsgDBHdr> oldHeader;
     nsresult ret = NS_OK;
 
     if (m_backupMailDB && !rawMsgId.IsEmpty())
       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));
     else if (!m_newMsgHdr)
     {
       // Should assert that this is not a local message
       ret = m_mailDB->CreateNewHdr(m_envelope_pos, getter_AddRefs(m_newMsgHdr));
     }
new file mode 100644
--- /dev/null
+++ b/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;
+}
--- a/mailnews/local/test/unit/xpcshell.ini
+++ b/mailnews/local/test/unit/xpcshell.ini
@@ -1,14 +1,15 @@
 [DEFAULT]
 head = head_maillocal.js
 tail = tail_local.js
 support-files = data/*
 
 [test_bug457168.js]
+[test_duplicateKey.js]
 [test_fileName.js]
 [test_folderLoaded.js]
 [test_localFolder.js]
 [test_mailboxContentLength.js]
 [test_mailboxProtocol.js]
 [test_movemailDownload.js]
 skip-if = os == "win"
 [test_msgCopy.js]