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)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.0
People
(Reporter: stephend, Assigned: alecf)
Details
(Keywords: memory-leak)
Attachments
(2 files, 1 obsolete file)
840 bytes,
patch
|
dbradley
:
review+
darin.moz
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
2.49 KB,
patch
|
dbaron
:
review+
darin.moz
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
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;
Reporter | ||
Comment 3•22 years ago
|
||
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
Comment 5•22 years ago
|
||
This is because the Clone function is broken, I believe. The copy-construct used copies the ref count
Assignee | ||
Comment 6•22 years ago
|
||
someone want to give this a shot?
Comment 7•22 years ago
|
||
Comment on attachment 72638 [details] [diff] [review] attempted fix, use NS_NewLocalFile r=pavlov
Attachment #72638 -
Flags: review+
Comment 8•22 years ago
|
||
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.
Assignee | ||
Comment 9•22 years ago
|
||
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)
Reporter | ||
Comment 10•22 years ago
|
||
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.
Comment 11•22 years ago
|
||
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.
Reporter | ||
Comment 12•22 years ago
|
||
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? :-)
Assignee | ||
Comment 13•22 years ago
|
||
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+
Comment 14•22 years ago
|
||
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.
Comment 15•22 years ago
|
||
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.
Assignee | ||
Comment 16•22 years ago
|
||
*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
Assignee | ||
Comment 17•22 years ago
|
||
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
Assignee | ||
Comment 18•22 years ago
|
||
I'm not sure if I need MOZ_COUNT_CTOR() or not. Ignore it in the patch, we'll leave it out for now.
Assignee | ||
Updated•22 years ago
|
Comment 19•22 years ago
|
||
Comment on attachment 74407 [details] [diff] [review] use copy constructor, but reset mRefCnt sr=darin, looks right to me.
Attachment #74407 -
Flags: superreview+
Comment 20•22 years ago
|
||
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.
Assignee | ||
Comment 21•22 years ago
|
||
I did this patch on my windows box, I'll test on unix/mac later.. assuming it compiles, can I get reviews?
Comment 22•22 years ago
|
||
Comment on attachment 74407 [details] [diff] [review] use copy constructor, but reset mRefCnt r=dbradley
Attachment #74407 -
Flags: review+
Comment 23•22 years ago
|
||
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 24•22 years ago
|
||
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.
Assignee | ||
Comment 25•22 years ago
|
||
doh! good catch, Conrad. I'll leave mac alone.
Comment 26•22 years ago
|
||
r=mkaply for the OS/2
Comment 27•22 years ago
|
||
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+
Reporter | ||
Comment 28•22 years ago
|
||
The latest patch fixes it quite nicely on my Win2k box. Thanks for the quick response, all!
Assignee | ||
Comment 29•22 years ago
|
||
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 31•22 years ago
|
||
Comment on attachment 74416 [details] [diff] [review] fix unix, mac, and OS/2 as well sr=darin
Attachment #74416 -
Flags: superreview+
Comment 32•22 years ago
|
||
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+
Assignee | ||
Comment 33•22 years ago
|
||
ok, that last patch has landed.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 34•22 years ago
|
||
reopening, it's baaaaaaack. comet & company went orange with this checkin, it has been backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 35•22 years ago
|
||
Also note, Tp (pageloader time) went up 2.5% with this checkin, times are back down after backout.
Assignee | ||
Comment 36•22 years ago
|
||
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 ago → 22 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•22 years ago
|
QA Contact: shrir → stephend
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•