I'm not sure if this is related to the nsMsgKeySet::GetLastMember bug Cavin is looking at or not. Bug 141299. 1. With mailnews, subscribe to news://news.mozilla.org/netscape.public.mozilla.mail-news. 2. Once you downloaded the messages, quit (don't do this under Purify). 3. In Purify, launch mozilla -news, and in the browser, type 'news://news.mozilla.org/netscape.public.mozilla.mail-news'. 4. Wait for it to switch windows. 5. Shut down the extra mail window (we don't focus the window you opened up in the 1st part of step #3. 6. Lastly, shutdown the browser and the mailnews window. [W] MLK: Memory leak of 20 bytes from 1 block allocated in nsMsgKeySet::Create (char const*) Distribution of leaked blocks Allocation location new(UINT) [MSVCRT.DLL] nsMsgKeySet::Create(char const*) [nsMsgKeySet.cpp:238] nsMsgKeySet* nsMsgKeySet::Create(const char* value /* , MSG_NewsHost* host */) => { #ifdef DEBUG_MSGKEYSET printf("create from %s\n",value); #endif nsMsgNewsFolder::SetReadSetFromStr(char const*) [nsNewsFolder.cpp:1532] if (mReadSet) delete mReadSet; => mReadSet = nsMsgKeySet::Create(newsrcLine); if (!mReadSet) return NS_ERROR_OUT_OF_MEMORY; nsMsgNewsFolder::AddNewsgroup(char const*,char const*,nsIMsgFolder * *) [nsNewsFolder.cpp:241] if (NS_FAILED(rv)) return rv; // cache this for when we open the db => rv = newsFolder->SetReadSetFromStr(setStr); rv = folder->SetParent(this); NS_ENSURE_SUCCESS(rv,rv); nsMsgNewsFolder::HandleNewsrcLine(char *,UINT) [nsNewsFolder.cpp:1150] // we're subscribed, so add it nsCOMPtr <nsIMsgFolder> child; => rv = AddNewsgroup(line, setStr, getter_AddRefs(child)); if (NS_FAILED(rv)) return -1; } nsMsgNewsFolder::HandleLine(char *,UINT) [nsNewsFolder.cpp:1082] PRInt32 nsMsgNewsFolder::HandleLine(char* line, PRUint32 line_size) => { return HandleNewsrcLine(line, line_size); } nsMsgLineBuffer::ConvertAndSendBuffer(void) [nsMsgLineBuffer.cpp:270] nsMsgLineBuffer::BufferInput(char const*,int) [nsMsgLineBuffer.cpp:206] nsMsgNewsFolder::LoadNewsrcFileAndCreateNewsgroups(void) [nsNewsFolder.cpp:1067] nsMsgNewsFolder::CreateSubFolders(nsFileSpec&) [nsNewsFolder.cpp:184] nsMsgNewsFolder::GetSubFolders(nsIEnumerator * *) [nsNewsFolder.cpp:349] nsMsgFolderDataSource::createFolderOpenNode(nsIMsgFolder *,nsIRDFNode * *) [nsMsgFolderDataSource.cpp:1367] // call GetSubFolders() to ensure mFlags is set correctly // from the folder cache on startup nsCOMPtr<nsIEnumerator> subFolders; => nsresult rv = folder->GetSubFolders(getter_AddRefs (subFolders)); if (NS_FAILED(rv)) return NS_RDF_NO_VALUE; nsMsgFolderDataSource::createFolderNode(nsIMsgFolder *,nsIRDFResource *,nsIRDFNode * *) [nsMsgFolderDataSource.cpp:1003] nsMsgFolderDataSource::GetTarget(nsIRDFResource *,nsIRDFResource *,int,nsIRDFNode * *) [nsMsgFolderDataSource.cpp:369] GetTargetHasAssertion(nsIRDFDataSource *,nsIRDFResource *,nsIRDFResource *,int,nsIRDFNode *,int *) [nsMsgRDFUtils.cpp:116] nsMsgFolderDataSource::DoFolderHasAssertion(nsIMsgFolder *,nsIRDFResource *,nsIRDFNode *,int,int *) [nsMsgFolderDataSource.cpp:2129] nsMsgFolderDataSource::HasAssertion(nsIRDFResource *,nsIRDFResource *,nsIRDFNode *,int,int *) [nsMsgFolderDataSource.obj:526] nsXULTreeBuilder::IsContainerOpen(nsIRDFResource *,int *) [nsXULTreeBuilder.cpp:1817] nsXULTreeBuilder::OpenSubtreeOf(Subtree::nsTreeRows *,nsIRDFResource *,int *) [nsXULTreeBuilder.cpp:1653] nsXULTreeBuilder::OpenContainer(int,nsIRDFResource *) [nsXULTreeBuilder.cpp:1566]

From the looks of it this is because of the change for bug 127707. the checkin was tagged as 80869 which is because it was attachment 80869 [details] [diff] [review].

could you attach a -uw diff? what's the point of this change? DownloadNewsArticlesToOfflineStore *downloadState = new DownloadNewsArticlesToOfflineStore(window, mDatabase, nsnull); - if (downloadState) - return downloadState->DownloadArticles(window, this, &srcKeyArray); - else + if (!downloadState) return NS_ERROR_OUT_OF_MEMORY; + + return downloadState->DownloadArticles(window, this, &srcKeyArray); and which of these seemingly unrelated changes actually fixes the leak? Is it just the change to Shutdown()? In which case, perhaps a patch with just that change would be quicker to review.

Alas, I have no Purify anymore (neither does David, I suspect, unless his copy is his own, and not AOL's), since I had to return all of my machines...

Comment on attachment 127659 [details] [diff] [review] fix leak [@@ -1845,7 +1847,10 @@] This all looks good but unfortunately it now conflicts :-( > what's the point of this change? Simple early returns make the code clearer, also avoiding else after return, especially with the current revision: DownloadNewsArticlesToOfflineStore *downloadState = new DownloadNewsArticlesToOfflineStore(msgWindow, mDatabase, this); if (downloadState) { m_downloadingMultipleMessages = PR_TRUE; return downloadState->DownloadArticles(msgWindow, this, &srcKeyArray); } else return NS_ERROR_OUT_OF_MEMORY; should be: DownloadNewsArticlesToOfflineStore *downloadState = new DownloadNewsArticlesToOfflineStore(msgWindow, mDatabase, this); if (!downloadState) return NS_ERROR_OUT_OF_MEMORY; m_downloadingMultipleMessages = PR_TRUE; return downloadState->DownloadArticles(msgWindow, this, &srcKeyArray);

sure, I agree. But I still would like to know what actually fixes the leak, and have a -uw diff.

Comment on attachment 135201 [details] [diff] [review] -w the leak is fixed by the change to nsMsgNewsFolder::Shutdown looks OK, thx, except that K&R is not the prevailing braces style in this file it's if (a) { .. }

Comment on attachment 135201 [details] [diff] [review] -w the leak is fixed by the change to nsMsgNewsFolder::Shutdown >+ if (mReadSet) { >+ nsCOMPtr<nsINewsDatabase> db = do_QueryInterface(mDatabase); >+ if (db) >+ db->SetReadSet(nsnull); >+ delete mReadSet; > mReadSet = nsnull; >+ } >+ ok, but the ownership here is not obvious. please add a comment to this effect: "the nsINewsDatabase holds a weak ref to the readset, and we outlive the db - so it's safe to delete it here." also, fwiw, you don't need that |if (mReadSet)| wrapper - everything you do inside it is nullsafe. remove it if you think that not having a readset is a rare case (since the QI + fncall costs perf). this code is begging for an autoptr, but i don't know whether this beast can be reinited after Shutdown() or not, so best to leave the deletion in the Shutdown() method and not push it into a dtor :) r=dwitte.

This has been checked in. Mark it fixed?

mozilla/mailnews/news/src/nsNewsFolder.cpp 1.249