Bug 199809 Memory leak in notification handler in nsMsgSendLater. r=Neil,sr=bienvenu
authorMark Banner <bugzilla@standard8.plus.com>
Thu, 04 Dec 2008 09:05:36 +0000
changeset 1312 5063d38688e8b00c24374266153ecb2ffd96c8f5
parent 1311 637e72124cb579a88ff0363cb0e793a9c18a4d58
child 1313 28f85630f344683e85f5ff52f6b01e9b9d8441aa
push idunknown
push userunknown
push dateunknown
reviewersNeil, bienvenu
bugs199809
Bug 199809 Memory leak in notification handler in nsMsgSendLater. r=Neil,sr=bienvenu
mailnews/compose/src/nsMsgSendLater.cpp
mailnews/compose/src/nsMsgSendLater.h
mailnews/compose/test/unit/test_sendMessageLater.js
--- a/mailnews/compose/src/nsMsgSendLater.cpp
+++ b/mailnews/compose/src/nsMsgSendLater.cpp
@@ -83,19 +83,16 @@ nsMsgSendLater::nsMsgSendLater()
   mTempFile = nsnull;
   mOutFile = nsnull;
   mTotalSentSuccessfully = 0;
   mTotalSendCount = 0;
   mMessageFolder = nsnull;
   mMessage = nsnull;
   mLeftoverBuffer = nsnull;
 
-  mListenerArray = nsnull;
-  mListenerArrayCount = 0;
-
   m_to = nsnull;
   m_bcc = nsnull;
   m_fcc = nsnull;
   m_newsgroups = nsnull;
   m_newshost = nsnull;
   m_headers = nsnull;
   m_flags = 0;
   m_headersFP = 0;
@@ -1117,103 +1114,64 @@ nsMsgSendLater::GetMsgWindow(nsIMsgWindo
   *aMsgWindow = m_window;
   NS_IF_ADDREF(*aMsgWindow);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsMsgSendLater::AddListener(nsIMsgSendLaterListener *aListener)
 {
-  if ( (mListenerArrayCount > 0) || mListenerArray )
-  {
-    ++mListenerArrayCount;
-    mListenerArray = (nsIMsgSendLaterListener **) 
-                  PR_Realloc(*mListenerArray, sizeof(nsIMsgSendLaterListener *) * mListenerArrayCount);
-    if (!mListenerArray)
-      return NS_ERROR_OUT_OF_MEMORY;
-    else
-    {
-      mListenerArray[mListenerArrayCount - 1] = aListener;
-      return NS_OK;
-    }
-  }
-  else
-  {
-    mListenerArrayCount = 1;
-    mListenerArray = (nsIMsgSendLaterListener **) PR_Malloc(sizeof(nsIMsgSendLaterListener *) * mListenerArrayCount);
-    if (!mListenerArray)
-      return NS_ERROR_OUT_OF_MEMORY;
-
-    memset(mListenerArray, 0, (sizeof(nsIMsgSendLaterListener *) * mListenerArrayCount));
-  
-    mListenerArray[0] = aListener;
-    NS_ADDREF(mListenerArray[0]);
-    return NS_OK;
-  }
+  NS_ENSURE_ARG_POINTER(aListener);
+  mListenerArray.AppendElement(aListener);
+  return NS_OK;
 }
 
 NS_IMETHODIMP
 nsMsgSendLater::RemoveListener(nsIMsgSendLaterListener *aListener)
 {
-  PRInt32 i;
-  for (i=0; i<mListenerArrayCount; i++)
-    if (mListenerArray[i] == aListener)
-    {
-      NS_RELEASE(mListenerArray[i]);
-      mListenerArray[i] = nsnull;
-      return NS_OK;
-    }
-
-  return NS_ERROR_INVALID_ARG;
+  NS_ENSURE_ARG_POINTER(aListener);
+  return mListenerArray.RemoveElement(aListener) ? NS_OK : NS_ERROR_INVALID_ARG;
 }
 
+#define NOTIFY_LISTENERS(propertyfunc_, params_) \
+  PR_BEGIN_MACRO                                 \
+  nsTObserverArray<nsCOMPtr<nsIMsgSendLaterListener> >::ForwardIterator iter(mListenerArray); \
+  nsCOMPtr<nsIMsgSendLaterListener> listener;    \
+  while (iter.HasMore()) {                       \
+    listener = iter.GetNext();                   \
+    listener->propertyfunc_ params_;             \
+  }                                              \
+  PR_END_MACRO
 
-nsresult
+void
 nsMsgSendLater::NotifyListenersOnStartSending(PRUint32 aTotalMessageCount)
 {
-  PRInt32 i;
-  for (i=0; i<mListenerArrayCount; i++)
-    if (mListenerArray[i] != nsnull)
-      mListenerArray[i]->OnStartSending(aTotalMessageCount);
-
-  return NS_OK;
+  NOTIFY_LISTENERS(OnStartSending, (aTotalMessageCount));
 }
 
-nsresult
-nsMsgSendLater::NotifyListenersOnProgress(PRUint32 aCurrentMessage, PRUint32 aTotalMessage)
+void
+nsMsgSendLater::NotifyListenersOnProgress(PRUint32 aCurrentMessage,
+                                          PRUint32 aTotalMessage)
 {
-  PRInt32 i;
-  for (i=0; i<mListenerArrayCount; i++)
-    if (mListenerArray[i] != nsnull)
-      mListenerArray[i]->OnProgress(aCurrentMessage, aTotalMessage);
-
-  return NS_OK;
+  NOTIFY_LISTENERS(OnProgress, (aCurrentMessage, aTotalMessage));
 }
 
-nsresult
+void
 nsMsgSendLater::NotifyListenersOnStatus(const PRUnichar *aMsg)
 {
-  PRInt32 i;
-  for (i=0; i<mListenerArrayCount; i++)
-    if (mListenerArray[i] != nsnull)
-      mListenerArray[i]->OnStatus(aMsg);
-
-  return NS_OK;
+  NOTIFY_LISTENERS(OnStatus, (aMsg));
 }
 
-nsresult
-nsMsgSendLater::NotifyListenersOnStopSending(nsresult aStatus, const PRUnichar *aMsg, 
-                                             PRUint32 aTotalTried, PRUint32 aSuccessful)
+void
+nsMsgSendLater::NotifyListenersOnStopSending(nsresult aStatus,
+                                             const PRUnichar *aMsg,
+                                             PRUint32 aTotalTried,
+                                             PRUint32 aSuccessful)
 {
-  PRInt32 i;
-  for (i=0; i<mListenerArrayCount; i++)
-    if (mListenerArray[i] != nsnull)
-      mListenerArray[i]->OnStopSending(aStatus, aMsg, aTotalTried, aSuccessful);
-
-  return NS_OK;
+  NOTIFY_LISTENERS(OnStopSending, (aStatus, aMsg, aTotalTried, aSuccessful));
 }
 
 // XXX todo
 // maybe this should just live in the account manager?
 nsresult
 nsMsgSendLater::GetIdentityFromKey(const char *aKey, nsIMsgIdentity  **aIdentity)
 {
   NS_ENSURE_ARG_POINTER(aIdentity);
--- a/mailnews/compose/src/nsMsgSendLater.h
+++ b/mailnews/compose/src/nsMsgSendLater.h
@@ -43,16 +43,17 @@
 #include "nsIMsgIdentity.h"
 #include "nsIEnumerator.h"
 #include "nsISupportsArray.h"
 #include "nsIMsgFolder.h"
 #include "nsIMsgSendListener.h"
 #include "nsIMsgSendLaterListener.h"
 #include "nsIMsgSendLater.h"
 #include "nsIMsgWindow.h"
+#include "nsTObserverArray.h"
 
 ////////////////////////////////////////////////////////////////////////////////////
 // This is the listener class for the send operation. We have to create this class 
 // to listen for message send completion and eventually notify the caller
 ////////////////////////////////////////////////////////////////////////////////////
 class nsMsgSendLater;
 
 class SendOperationListener : public nsIMsgSendListener,
@@ -99,36 +100,36 @@ public:
   nsresult                  SetOrigMsgDisposition();
   // Necessary for creating a valid list of recipients
   nsresult                  BuildHeaders();
   nsresult                  DeliverQueuedLine(char *line, PRInt32 length);
   nsresult                  RebufferLeftovers(char *startBuf,  PRUint32 aLen);
   nsresult                  BuildNewBuffer(const char* aBuf, PRUint32 aCount, PRUint32 *totalBufSize);
 
   // methods for listener array processing...
-  nsresult  NotifyListenersOnStartSending(PRUint32 aTotalMessageCount);
-  nsresult  NotifyListenersOnProgress(PRUint32 aCurrentMessage, PRUint32 aTotalMessage);
-  nsresult  NotifyListenersOnStatus(const PRUnichar *aMsg);
-  nsresult  NotifyListenersOnStopSending(nsresult aStatus, const PRUnichar *aMsg, 
-                                           PRUint32 aTotalTried, PRUint32 aSuccessful);
+  void NotifyListenersOnStartSending(PRUint32 aTotalMessageCount);
+  void NotifyListenersOnProgress(PRUint32 aCurrentMessage,
+                                 PRUint32 aTotalMessage);
+  void NotifyListenersOnStatus(const PRUnichar *aMsg);
+  void NotifyListenersOnStopSending(nsresult aStatus, const PRUnichar *aMsg, 
+                                    PRUint32 aTotalTried, PRUint32 aSuccessful);
 
   // counters and things for enumeration 
   PRUint32                  mTotalSentSuccessfully;
   PRUint32                  mTotalSendCount;
   nsCOMPtr<nsISupportsArray> mMessagesToSend;
   nsCOMPtr<nsIEnumerator> mEnumerator;
   nsCOMPtr<nsIMsgFolder>    mMessageFolder;
   nsCOMPtr<nsIMsgWindow>    m_window;
  
   // Private Information
 private:
   nsresult GetIdentityFromKey(const char *aKey, nsIMsgIdentity **aIdentity);
 
-  nsIMsgSendLaterListener   **mListenerArray;
-  PRInt32                   mListenerArrayCount;
+  nsTObserverArray<nsCOMPtr<nsIMsgSendLaterListener> > mListenerArray;
 
   nsCOMPtr<nsIMsgDBHdr>      mMessage;
 
   //
   // File output stuff...
   //
   nsCOMPtr<nsIFile>         mTempFile;
   nsCOMPtr<nsIOutputStream> mOutFile;
new file mode 100644
--- /dev/null
+++ b/mailnews/compose/test/unit/test_sendMessageLater.js
@@ -0,0 +1,250 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/**
+ * Protocol tests for SMTP.
+ *
+ * This test verifies:
+ * - Sending a message to an SMTP server (which is also covered elsewhere).
+ * - Correct reception of the message by the SMTP server.
+ * - Correct saving of the message to the sent folder.
+ *
+ * Originally written to test bug 429891 where saving to the sent folder was
+ * mangling the message.
+ */
+var type = null;
+var test = null;
+var server;
+var sentFolder;
+var transaction;
+var originalData;
+var finished = false;
+var identity = null;
+var testFile = do_get_file("../mailnews/compose/test/unit/data/429891_testcase.eml");
+
+const kSender = "from@invalid.com";
+const kTo = "invalid@invalid.com";
+
+// This listener handles the post-sending of the actual message and checks the
+// sequence and ensures the data is correct.
+function msll() {
+}
+
+msll.prototype = {
+  // nsIMsgSendLaterListener
+  OnStartSending: function (aTotal) {
+  },
+  OnProgress: function (aCurrentMessage, aTotal) {
+  },
+  OnStatus: function (aMsg) {
+  },
+  OnStopSending: function (aStatus, aMsg, aTotal, aSuccessful) {
+    do_test_finished();
+    print("msll onStopSending\n");
+    try {
+      do_check_eq(aStatus, 0);
+
+      do_check_transaction(transaction,
+                           ["EHLO test",
+                            "MAIL FROM:<" + kSender + "> SIZE=" + originalData.length,
+                            "RCPT TO:<" + kTo + ">",
+                            "DATA"]);
+
+      // Compare data file to what the server received
+      do_check_eq(originalData, server._handler.post);
+
+      // Now wait till the copy is finished for the sent message
+      do_test_pending();
+    } catch (e) {
+      do_throw(e);
+    } finally {
+      server.stop();
+
+      var thread = gThreadManager.currentThread;
+      while (thread.hasPendingEvents())
+        thread.processNextEvent(true);
+    }
+  }
+};
+
+// This listener is used to find out when the copying of the message to the
+// unsent message folder is completed, and hence can fire off the actual
+// sending of the message.
+function copyListener() {
+}
+
+copyListener.prototype = {
+  // nsIMsgSendListener
+  onStartSending: function (aMsgID, aMsgSize) {
+  },
+  onProgress: function (aMsgID, aProgress, aProgressMax) {
+  },
+  onStatus: function (aMsgID, aMsg) {
+  },
+  onStopSending: function (aMsgID, aStatus, aMsg, aReturnFile) {
+  },
+  onGetDraftFolderURI: function (aFolderURI) {
+  },
+  onSendNotPerformed: function (aMsgID, aStatus) {
+  },
+
+  // nsIMsgCopyServiceListener
+  OnStartCopy: function () {
+  },
+  OnProgress: function (aProgress, aProgressMax) {
+  },
+  SetMessageKey: function (aKey) {
+  },
+  GetMessageId: function (aMessageId) {
+  },
+  OnStopCopy: function (aStatus) {
+    do_test_finished();
+
+    try {
+      do_check_eq(aStatus, 0);
+
+      var msgSendLater = Cc["@mozilla.org/messengercompose/sendlater;1"]
+                           .createInstance(Ci.nsIMsgSendLater);
+
+      let folder = msgSendLater.getUnsentMessagesFolder(identity);
+
+      // Check we have a message in the unsent message folder
+      do_check_eq(folder.getTotalMessages(false), 1);
+
+      // Now do a comparison of what is in the sent mail folder
+      var fileData = loadFileToString(folder.filePath);
+
+      // Skip the headers etc that mailnews adds
+      var pos = fileData.indexOf("From:");
+      do_check_neq(pos, -1);
+
+      fileData = fileData.substr(pos);
+
+      // Check the data is matching.
+      do_check_eq(originalData, fileData);
+
+      do_test_pending();
+      do_timeout(sendMessageLater(), 0);
+    } catch (e) {
+      do_throw(e);
+    } finally {
+      server.stop();
+
+      var thread = gThreadManager.currentThread;
+      while (thread.hasPendingEvents())
+        thread.processNextEvent(true);
+
+      finished = true;
+    }
+  },
+
+  // QueryInterface
+  QueryInterface: function (iid) {
+    if (iid.equals(Ci.nsIMsgSendListener) ||
+        iid.equals(Ci.nsIMsgCopyServiceListener) ||
+        iid.equals(Ci.nsISupports))
+      return this;
+
+    Components.returnCode = Components.results.NS_ERROR_NO_INTERFACE;
+    return null;
+  }
+};
+
+// This function does the actual send later
+function sendMessageLater()
+{
+  do_test_finished();
+
+  // Set up the SMTP server.
+  server = setupServerDaemon();
+
+  type = "sendMessageLater";
+
+  // Handle the server in a try/catch/finally loop so that we always will stop
+  // the server if something fails.
+  try {
+    // Start the fake SMTP server
+    server.start(SMTP_PORT);
+
+    // A test to check that we are sending files correctly, including checking
+    // what the server receives and what we output.
+    test = "sendMessageLater";
+
+    var msgSendLater = Cc["@mozilla.org/messengercompose/sendlater;1"]
+      .createInstance(Ci.nsIMsgSendLater);
+
+    var messageListener = new msll();
+
+    msgSendLater.AddListener(messageListener);
+
+    // Send the unsent message
+    msgSendLater.SendUnsentMessages(identity);
+
+    server.performTest();
+
+    transaction = server.playTransaction();
+
+    do_timeout(10000, "if (!finished) do_throw('Notifications of message send/copy not received');");
+
+    do_test_pending();
+
+  } catch (e) {
+    do_throw(e);
+  } finally {
+    server.stop();
+
+    var thread = gThreadManager.currentThread;
+    while (thread.hasPendingEvents())
+      thread.processNextEvent(true);
+  }
+}
+
+function run_test() {
+  // Test file - for bug 429891
+  originalData = loadFileToString(testFile);
+
+  Components.classes["@mozilla.org/preferences-service;1"]
+            .getService(Components.interfaces.nsIPrefBranch)
+    .setBoolPref("mail.really_delete_draft", false);
+
+  // Ensure we have a local mail account, an normal account and appropriate
+  // servers and identities.
+  loadLocalMailAccount();
+
+  var acctMgr = Cc["@mozilla.org/messenger/account-manager;1"]
+                  .getService(Ci.nsIMsgAccountManager);
+  acctMgr.setSpecialFolders();
+
+  var account = acctMgr.createAccount();
+  incomingServer = acctMgr.createIncomingServer("test", "localhost", "pop3");
+
+  var smtpServer = getBasicSmtpServer();
+  identity = getSmtpIdentity(kSender, smtpServer);
+
+  account.addIdentity(identity);
+  account.defaultIdentity = identity;
+  account.incomingServer = incomingServer;
+
+  sentFolder = gLocalIncomingServer.rootMsgFolder.addSubfolder("Sent");
+
+  do_check_eq(identity.doFcc, true);
+
+  // Now prepare to actually "send" the message later, i.e. dump it in the
+  // unsent messages folder.
+
+  var compFields = Cc["@mozilla.org/messengercompose/composefields;1"]
+                     .createInstance(Ci.nsIMsgCompFields);
+
+  compFields.from = identity.email;
+  compFields.to = kTo;
+
+  var cl = new copyListener(true);
+
+  var msgSend = Cc["@mozilla.org/messengercompose/send;1"]
+                  .createInstance(Ci.nsIMsgSend);
+
+  msgSend.sendMessageFile(identity, "", compFields, testFile,
+                          false, false, Ci.nsIMsgSend.nsMsgQueueForLater,
+                          null, cl, null, null);
+
+  // Now we wait till we get copy notification of completion.
+  do_test_pending();
+}