Memory leak of 20 bytes from 1 block allocated in nsMsgKeySet::Create(char const*)

RESOLVED FIXED

Status

RESOLVED FIXED
16 years ago
10 years ago

People

(Reporter: stephend, Assigned: timeless)

Tracking

({memory-leak})

Trunk
x86
Windows 2000
memory-leak

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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]
Keywords: mlk
QA Contact: gayatri → stephend
Severity: major → normal
(Assignee)

Comment 1

15 years ago
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].
Assignee: bienvenu → timeless
Depends on: 127707
(Assignee)

Comment 2

15 years ago
Created attachment 127659 [details] [diff] [review]
fix leak [@@ -1845,7 +1847,10 @@]
(Assignee)

Updated

15 years ago
Attachment #127659 - Flags: superreview?(bienvenu)
Attachment #127659 - Flags: review?(stephend)

Comment 3

15 years ago
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.
Attachment #127659 - Flags: review?(stephend) → review?(technutz)
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...
Attachment #127659 - Flags: review?(technutz) → review?

Comment 5

15 years ago
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);

Comment 6

15 years ago
sure, I agree. But I still would like to know what actually fixes the leak, and
have a -uw diff.
(Assignee)

Updated

15 years ago
Attachment #127659 - Flags: superreview?(bienvenu)
(Assignee)

Comment 7

15 years ago
Created attachment 135200 [details] [diff] [review]
revised patch
Attachment #127659 - Attachment is obsolete: true
(Assignee)

Comment 8

15 years ago
Created attachment 135201 [details] [diff] [review]
-w the leak is fixed by the change to nsMsgNewsFolder::Shutdown
(Assignee)

Updated

15 years ago
Attachment #135201 - Flags: superreview?(bienvenu)

Comment 9

15 years ago
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)
{
  ..
}
Attachment #135201 - Flags: superreview?(bienvenu) → superreview+

Comment 10

15 years ago
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.
Attachment #135201 - Flags: review+
This has been checked in.  Mark it fixed?
(Assignee)

Comment 12

15 years ago
mozilla/mailnews/news/src/nsNewsFolder.cpp 	1.249
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.