Closed Bug 124497 Opened 22 years ago Closed 22 years ago

Memory leak of 80 bytes from 1 block allocated in nsLocalFile::Clone

Categories

(Core Graveyard :: Plug-ins, defect, P1)

x86
Windows 2000
defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.0

People

(Reporter: stephend, Assigned: alecf)

Details

(Keywords: memory-leak)

Attachments

(2 files, 1 obsolete file)

Build: Latest trunk Windows 2000 build (as of 3:10 pm, 2/8/2002).

Summary: 

Steps to Reproduce:

1.  Have only one addressbook card in your Personal Address Book.
2.  Launch mozilla -addressbook.
3.  Select the card.
4.  Use the delete button to delete it.
5.  Shutdown.

[W] MLK: Memory leak of 80 bytes from 1 block allocated in nsLocalFile::Clone
(nsIFile * *)
    Distribution of leaked blocks
        80 bytes from 1 block of 80 bytes (0x04b4f260) 
    Allocation location
    new(UINT)      [MSVCRT.DLL]
    nsLocalFile::Clone(nsIFile * *) [nsLocalFileWin.cpp:657]
        *file = nsnull;
    
        // Just copy-construct ourselves
 =>     nsCOMPtr<nsILocalFile> localFile = new nsLocalFile(*this);
        if (localFile == NULL)
          return NS_ERROR_OUT_OF_MEMORY;
    
    nsDirectoryService::Get(char const*,nsID const&,void * *) 
[nsDirectoryService.cpp:678]
            if (!cachedFile)
                return NS_ERROR_NULL_POINTER;
    
 =>         cachedFile->Clone(getter_AddRefs(cloneFile));
            return cloneFile->QueryInterface(uuid, result);
        }
    
    NS_GetSpecialDirectory(char const*,nsIFile * *) [nsIFile.h:802]
    nsPluginHostImpl::Destroy(void) [nsPluginHostImpl.cpp:3184]
    
      // Lets remove any of the temporary files that we created.
      nsCOMPtr<nsIFile> pluginTmp;
 =>   nsresult rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs
(pluginTmp));
      if (NS_FAILED(rv)) return rv;
    
      rv = pluginTmp->Append(kPluginTmpDirName);
    nsPluginHostImpl::Observe(nsISupports *,char const*,WORD const*) 
[nsPluginHostImpl.cpp:5775]
      if (!nsCRT::strcmp(NS_XPCOM_SHUTDOWN_OBSERVER_ID, aTopic) ||
          !nsCRT::strcmp("quit-application", aTopic))
      {
 =>     Destroy();
      }
      return NS_OK;
me
Assignee: dougt → neeti
Target Milestone: --- → mozilla0.9.9
stephen: is this leak still reproducible?
With Purify and the latest win2k tip, yes.
From the purify stack trace, the nsLocalFile is being allocated in 
nsPluginHostImpl.
Assignee: neeti → av
Component: XPCOM → Plug-ins
QA Contact: scc → shrir
Target Milestone: mozilla0.9.9 → ---
This is because the Clone function is broken, I believe. The copy-construct used
copies the ref count
someone want to give this a shot?
Comment on attachment 72638 [details] [diff] [review]
attempted fix, use NS_NewLocalFile

r=pavlov
Attachment #72638 - Flags: review+
to me the patch looks exactly as it used to be in revision 1.75
before dougt's checkins with comment:
"fixes 122892. Optimization Clone of nsLocalFile.  Instead of reiniting object, 
we use default c++ copy construction."
---
cvs diff -u  -r 1.76 -r 1.75 nsLocalFileWin.cpp
RCS file: /cvsroot/mozilla/xpcom/io/nsLocalFileWin.cpp,v
retrieving revision 1.76
retrieving revision 1.75
diff -u -r1.76 -r1.75
--- nsLocalFileWin.cpp  2 Feb 2002 01:25:57 -0000       1.76
+++ nsLocalFileWin.cpp  31 Jan 2002 21:55:01 -0000      1.75
@@ -650,18 +650,18 @@
 NS_IMETHODIMP
 nsLocalFile::Clone(nsIFile **file)
 {
-    NS_ENSURE_ARG(file);
-    *file = nsnull;
+    nsresult rv;

-    // Just copy-construct ourselves
-    nsCOMPtr<nsILocalFile> localFile = new nsLocalFile(*this);
-    if (localFile == NULL)
-      return NS_ERROR_OUT_OF_MEMORY;
+    nsCOMPtr<nsILocalFile> localFile;

-    *file = localFile;
-    NS_ADDREF(*file);
-
-    return NS_OK;
+    rv = NS_NewLocalFile(mWorkingPath.get(), mFollowSymlinks, getter_AddRefs(lo
calFile));
+
+    if (NS_SUCCEEDED(rv) && localFile)
+    {
+        return localFile->QueryInterface(NS_GET_IID(nsIFile), (void**)file);
+    }
+
+    return rv;
---
ccing dougt.
ah, then maybe dbaron's approach is better. I'm not a huge fan of accessing
mRefCnt directly.. we could also:
- create the object with new (no parameters) and then
- manually copy over the stat info from "this" to the newly created nsLocalFile
(i.e. not the interface)
http://bugzilla.mozilla.org/attachment.cgi?id=72638&action=view fixes the leaks 
in nsLocalFile::Clone, but not the other leaks sent to alecf, dbradley and 
dbaron via mail.
Stephen, can you double check. I just applied the patch, ran Purify and most all
of my PR_Malloc leaks are gone. There's just a hand full left that but appear
unrelated to the one you sent via e-mail.
Dbradley: yup.  After I wrote my comment #10, I pulled and built, and see the 
same results you do, amazingly!  When can we land this? :-)
Comment on attachment 72638 [details] [diff] [review]
attempted fix, use NS_NewLocalFile

we need a different approach (one of the two mentioned here) - I'll see if I
can cobble something together today.
Attachment #72638 - Flags: needs-work+
Actually this was the result of revision 1.78, 1.75 was backed out. Bug 122523
was the bug dealing with this optimization. Should we not reopen it, and then
dupe this one back to it? Bug 122892 and Bug 122523 seem to be aiming at the
same thing as well.
Ok, I think I figured out what really happened. CVS Blame shows that the code
was introduced by bug 122523 in revision 1.78. If you look at the first patch
with r/sr it contains just one hunk for this file. CVS Blame shows two hunks. I
think dougt may have accidentially reintroduced this code when this was checked
in. So maybe the simple fix is just to back out that hunk that was accidentially
checked in. And leave figure out how to optimize it, to bug 122892.
*sigh* I'll take this. I'm going to switch to directly modifying mRefCnt, and
manually calling MOZ_COUNT_CTOR() - I'm not sure if calling MOZ_COUNT_CTOR is
the right thing though. anyone know for certain? 
Assignee: av → alecf
ok, here's a better fix using dbaron's suggestion. Anyone want to review?

I had to switch away from nsCOMPtr so I could access the concrete class's
mRefCnt variable.
Attachment #72638 - Attachment is obsolete: true
I'm not sure if I need MOZ_COUNT_CTOR() or not. Ignore it in the patch, we'll
leave it out for now.
Status: NEW → ASSIGNED
Keywords: mozilla1.0
Priority: -- → P1
Target Milestone: --- → mozilla1.0
Comment on attachment 74407 [details] [diff] [review]
use copy constructor, but reset mRefCnt

sr=darin, looks right to me.
Attachment #74407 - Flags: superreview+
I guess what makes me a little leary is that we're fixing an accidental
check-in. And we're leaving the other platforms with the unoptimized versions.
So the patch in bug 122892 is half in and half out.
I did this patch on my windows box, I'll test on unix/mac later.. assuming it
compiles, can I get reviews?
Comment on attachment 74407 [details] [diff] [review]
use copy constructor, but reset mRefCnt

r=dbradley
Attachment #74407 - Flags: review+
Minor nit on the third patch, should we get these to be pretty much identical?
There's a minor different that I see, in testing localFile for null.
Comment on attachment 74416 [details] [diff] [review]
fix unix, mac, and OS/2 as well

Wait - the Mac impl doesn't need this patch. Notice it defines its own copy
constructor and it does call NS_INIT_REFCNT.
doh! good catch, Conrad. I'll leave mac alone.
r=mkaply for the OS/2
Comment on attachment 74407 [details] [diff] [review]
use copy constructor, but reset mRefCnt

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #74407 - Flags: approval+
The latest patch fixes it quite nicely on my Win2k box.  Thanks for the quick 
response, all!
ok, the windows patch has been checked in..

still need:
sr= on the os/2 and unix patch
r= on the unix patch

(ignore the mac patch)
Comment on attachment 74416 [details] [diff] [review]
fix unix, mac, and OS/2 as well

r=dbaron on the Unix patch.  I'm marking "has review" since the unix and OS/2
patches are both reviewed, as if the mac patch weren't in the diff.
Attachment #74416 - Flags: review+
Comment on attachment 74416 [details] [diff] [review]
fix unix, mac, and OS/2 as well

sr=darin
Attachment #74416 - Flags: superreview+
Comment on attachment 74416 [details] [diff] [review]
fix unix, mac, and OS/2 as well

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #74416 - Flags: approval+
ok, that last patch has landed.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
reopening, it's baaaaaaack.  comet & company went orange with this
checkin, it has been backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Also note, Tp (pageloader time) went up 2.5% with this checkin,
times are back down after backout.
well, I'm going to re-mark this fix, since the actual bug is the leaking, which
was fixed. The 2nd patch was just an attempt to optimize unix & os/2.
Status: REOPENED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
QA Contact: shrir → stephend
verified fixed, trunk with Purify.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: