fix crash proxy releasing null pointer, original patch by bz, tweaked by me, sr=mscott 391259

mailnews/imap/public/nsIImapMailFolderSink.idl

@@ -105,11 +105,6 @@ interface nsIImapMailFolderSink : nsISupports {
   void NotifySearchHit(in nsIMsgMailNewsUrl aUrl, in string hitLine);
 
   void copyNextStreamMessage(in boolean copySucceeded, in nsISupports copyState);
-  // these two hokey methods are needed so we can try to make sure the imap objects are released
-  // on the UI thread. This in turn ensures that the objects the imap object holds on to
-  // are only released / destroyed from the UI thread. This is used for urls and copy state objects now.
-  void prepareToReleaseObject(in nsISupports aISupports);
-  void releaseObject();
   void closeMockChannel(in nsIImapMockChannel aChannel);
   void setUrlState(in nsIImapProtocol aProtocol, in nsIMsgMailNewsUrl aUrl, in boolean isRunning, in nsresult status);
   void releaseUrlCacheEntry(in nsIMsgMailNewsUrl aUrl);

mailnews/imap/src/nsImapMailFolder.cpp

@@ -4007,21 +4007,6 @@ nsImapMailFolder::OnlineCopyCompleted(nsIImapProtocol *aProtocol, ImapOnlineCopy
   return rv;
 }
 
-NS_IMETHODIMP
-nsImapMailFolder::PrepareToReleaseObject(nsISupports * aSupports)
-{
-  NS_ASSERTION(!mSupportsToRelease, "can't prepare to release w/o releasing prev object");
-  mSupportsToRelease = aSupports;
-  return NS_OK;
-}
-
-NS_IMETHODIMP
-nsImapMailFolder::ReleaseObject()
-{
-  mSupportsToRelease = nsnull;
-  return NS_OK;
-}
-
 NS_IMETHODIMP
 nsImapMailFolder::CloseMockChannel(nsIImapMockChannel * aChannel)
 {

mailnews/imap/src/nsImapMailFolder.h

@@ -459,8 +459,6 @@ protected:
   PRUint32     m_aclFlags;
   PRUint32     m_supportedUserFlags;
 
-  nsCOMPtr<nsISupports> mSupportsToRelease;
-
   static nsIAtom* mImapHdrDownloadedAtom;
 
   // offline imap support

mailnews/imap/src/nsImapProtocol.cpp

@@ -1,4 +1,4 @@
-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
@@ -845,25 +845,37 @@ nsresult nsImapProtocol::SetupWithUrl(nsIURI * aURL, nsISupports* aConsumer)
 void nsImapProtocol::ReleaseUrlState(PRBool rerunning)
 {
   // clear out the socket's reference to the notification callbacks for this transaction
-  if (m_transport)
   {
-    nsAutoCMonitor mon (this);
-    m_transport->SetSecurityCallbacks(nsnull);
-    m_transport->SetEventSink(nsnull, nsnull);
+    nsAutoCMonitor mon(this);
+    if (m_transport)
+    {
+      m_transport->SetSecurityCallbacks(nsnull);
+      m_transport->SetEventSink(nsnull, nsnull);
+    }
   }
 
   if (m_mockChannel && !rerunning)
   {
+    // Proxy the close of the channel to the ui thread.
     if (m_imapMailFolderSink)
       m_imapMailFolderSink->CloseMockChannel(m_mockChannel);
     else
       m_mockChannel->Close();
-    // Proxy the release of the channel to the main thread.  This is something
-    // that the xpcom proxy system should do for us!
-    nsCOMPtr<nsIThread> thread = do_GetMainThread();
-    nsIImapMockChannel *doomed = nsnull;
-    m_mockChannel.swap(doomed);
-    NS_ProxyRelease(thread, doomed);
+
+    {
+      // grab a monitor so m_mockChannel doesn't get cleared out
+      // from under us.
+      nsAutoCMonitor mon(this);
+      if (m_mockChannel)
+      {
+        // Proxy the release of the channel to the main thread.  This is something
+        // that the xpcom proxy system should do for us!
+        nsCOMPtr<nsIThread> thread = do_GetMainThread();
+        nsIImapMockChannel *doomed = nsnull;
+        m_mockChannel.swap(doomed);
+        NS_ProxyRelease(thread, doomed);
+      }
+    }
   }
 
   m_channelContext = nsnull; // this might be the url - null it out before the final release of the url
@@ -871,50 +883,46 @@ void nsImapProtocol::ReleaseUrlState(PRBool rerunning)
 
   // Proxy the release of the listener to the main thread.  This is something
   // that the xpcom proxy system should do for us!
-  if (m_channelListener)
   {
-    nsCOMPtr<nsIThread> thread = do_GetMainThread();
-    nsIStreamListener *doomed = nsnull;
-    m_channelListener.swap(doomed);
-    NS_ProxyRelease(thread, doomed);
-  }
-
-  m_channelInputStream = nsnull;
-  m_channelOutputStream = nsnull;
-  if (m_runningUrl)
-  {
-    nsCOMPtr<nsIMsgMailNewsUrl>  mailnewsurl = do_QueryInterface(m_runningUrl);
+    // grab a monitor so the m_channelListener doesn't get cleared.
+    nsAutoCMonitor mon(this);
+    if (m_channelListener)
     {
-      nsCOMPtr <nsIImapMailFolderSink> saveFolderSink = m_imapMailFolderSink;
-      {
-        nsAutoCMonitor mon (this);
-        m_runningUrl = nsnull; // force us to release our last reference on the url
-        m_imapMailFolderSink = nsnull;
-        m_urlInProgress = PR_FALSE;
-      }
-
-      // we want to make sure the imap protocol's last reference to the url gets released
-      // back on the UI thread. This ensures that the objects the imap url hangs on to
-      // properly get released back on the UI thread. In order to do this, we need a
-      // a fancy texas two step where we first give the ui thread the url we want to
-      // release, then we forget about our copy. Then we tell it to release the url
-      // for real.
-      if (saveFolderSink)
-      {
-        nsCOMPtr <nsISupports> supports = do_QueryInterface(mailnewsurl);
-        saveFolderSink->PrepareToReleaseObject(supports);
-        supports = nsnull;
-        mailnewsurl = nsnull;
-        // at this point in time, we MUST have released all of our references to
-        // the url from the imap protocol. otherwise this whole exercise is moot.
-        saveFolderSink->ReleaseObject();
-        saveFolderSink = nsnull;
-      }
+      nsCOMPtr<nsIThread> thread = do_GetMainThread();
+      nsIStreamListener *doomed = nsnull;
+      m_channelListener.swap(doomed);
+      NS_ProxyRelease(thread, doomed);
     }
   }
-  else
-    m_imapMailFolderSink = nsnull;
+  m_channelInputStream = nsnull;
+  m_channelOutputStream = nsnull;
+  
+  nsCOMPtr<nsIMsgMailNewsUrl> mailnewsurl;
+  nsCOMPtr<nsIImapMailFolderSink> saveFolderSink;
 
+  if (m_runningUrl)
+  {
+    mailnewsurl = do_QueryInterface(m_runningUrl);
+    saveFolderSink = m_imapMailFolderSink;
+
+    m_runningUrl = nsnull; // force us to release our last reference on the url
+    m_urlInProgress = PR_FALSE;
+  }
+
+  // Need to null this out whether we have an m_runningUrl or not
+  m_imapMailFolderSink = nsnull;
+
+  // we want to make sure the imap protocol's last reference to the url gets released
+  // back on the UI thread. This ensures that the objects the imap url hangs on to
+  // properly get released back on the UI thread. 
+  if (saveFolderSink)
+  {
+    nsCOMPtr<nsIThread> thread = do_GetMainThread();
+    nsIMsgMailNewsUrl *doomed = nsnull;
+    mailnewsurl.swap(doomed);
+    NS_ProxyRelease(thread, doomed);
+    saveFolderSink = nsnull;
+  }
 }
 
 
@@ -967,6 +975,8 @@ NS_IMETHODIMP nsImapProtocol::Run()
 }
 
 // called from UI thread.
+// XXXbz except this is called from TellThreadToDie, which can get called on
+// either the UI thread or the IMAP protocol thread, per comments.
 void nsImapProtocol::CloseStreams()
 {
   PR_CEnterMonitor(this);
@@ -979,11 +989,15 @@ void nsImapProtocol::CloseStreams()
   }
   m_inputStream = nsnull;
   m_outputStream = nsnull;
+  // XXXbz given that this can get called from off the UI thread, does the
+  // release of m_channelListener need to be proxied?
   m_channelListener = nsnull;
   m_channelContext = nsnull;
   if (m_mockChannel)
   {
       m_mockChannel->Close();
+      // XXXbz given that this can get called from off the UI thread, does the
+      // release of m_mockChannel need to be proxied?
       m_mockChannel = nsnull;
   }
   m_channelInputStream = nsnull;
@@ -1517,14 +1531,18 @@ PRBool nsImapProtocol::ProcessCurrentURL()
 
   if (imapMailFolderSink)
   {
-      imapMailFolderSink->PrepareToReleaseObject(copyState);
-      imapMailFolderSink->CopyNextStreamMessage(GetServerStateParser().LastCommandSuccessful()
+    imapMailFolderSink->CopyNextStreamMessage(GetServerStateParser().LastCommandSuccessful()
                                                 && GetConnectionStatus() >= 0, copyState);
-      copyState = nsnull;
-      imapMailFolderSink->ReleaseObject();
-      // we might need this to stick around for IDLE support
-      m_imapMailFolderSink = imapMailFolderSink;
-      imapMailFolderSink = nsnull;
+    if (copyState)
+    {
+      nsCOMPtr<nsIThread> thread = do_GetMainThread();
+      nsISupports *doomed = nsnull;
+      copyState.swap(doomed);
+      NS_ProxyRelease(thread, doomed);
+    }
+    // we might need this to stick around for IDLE support
+    m_imapMailFolderSink = imapMailFolderSink;
+    imapMailFolderSink = nsnull;
   }
 
   // now try queued urls, now that we've released this connection.