Memory leak of 672 bytes from 4 blocks allocated in PR_Malloc

VERIFIED FIXED

Status

SeaMonkey
MailNews: Address Book & Contacts
VERIFIED FIXED
15 years ago
13 years ago

People

(Reporter: stephend@netscape.com (gone - use stephen.donner@gmail.com instead), Assigned: dwitte@gmail.com)

Tracking

({mlk})

Trunk
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

5.40 KB, patch
dwitte@gmail.com
: review+
Details | Diff | Splinter Review
Build: Latest trunk pull on Windows 2000 under Purify.

Steps to Reproduce:

1.  mozilla.exe -addressbook
2.  Click 'New Card'
3.  Type "S" into any field.
4.  Hit Save.

    [W] MLK: Memory leak of 672 bytes from 4 blocks allocated in PR_Malloc
        Distribution of leaked blocks
        Allocation location
        malloc         [dbgheap.c:129]
        PR_Malloc      [prmem.c:474]
        #if defined (WIN16)
            return PR_MD_malloc( (size_t) size);
        #else
     =>     return malloc(size);
        #endif
        }
        
        nsAbDirProperty::GetDirectoryProperties(nsIAbDirectoryProperties * *)
[nsAbDirProperty.cpp:321]
          NS_ENSURE_ARG_POINTER(aDirectoryProperties);
        
          nsresult rv;
     =>   DIR_Server *server = (DIR_Server *)PR_Malloc(sizeof(DIR_Server));
          if (!server)
            return NS_ERROR_OUT_OF_MEMORY;
        
        nsAbDirectoryDataSource::createDirectoryTreeNameSortNode(nsIAbDirectory
*,nsIRDFNode * *) [nsDirectoryDataSource.cpp:712]
        
          // Get directory type.
          nsCOMPtr <nsIAbDirectoryProperties> properties;
     =>   rv = directory->GetDirectoryProperties(getter_AddRefs(properties));
          NS_ENSURE_SUCCESS(rv,rv);
          PRUint32 dirType;
          rv = properties->GetDirType(&dirType);
        nsAbDirectoryDataSource::createDirectoryNode(nsIAbDirectory
*,nsIRDFResource *,nsIRDFNode * *) [nsDirectoryDataSource.cpp:578]
          if ((kNC_IsWriteable == property))
            rv = createDirectoryIsWriteableNode(directory, target);
          if ((kNC_DirTreeNameSort == property))
     =>   rv = createDirectoryTreeNameSortNode(directory, target);
          return rv;
        }
        
        nsAbDirectoryDataSource::GetTarget(nsIRDFResource *,nsIRDFResource
*,int,nsIRDFNode * *) [nsDirectoryDataSource.cpp:219]
        
          nsCOMPtr<nsIAbDirectory> directory(do_QueryInterface(source, &rv));
          if (NS_SUCCEEDED(rv) && directory)
     =>     rv = createDirectoryNode(directory, property, target);
          else
            return NS_RDF_NO_VALUE;
          return rv;
        CompositeDataSourceImpl::GetTarget(nsIRDFResource *,nsIRDFResource
*,int,nsIRDFNode * *) [nsCompositeDataSource.cpp:825]
            for (PRInt32 i = 0; i < count; ++i) {
                nsresult rv;
                rv = mDataSources[i]->GetTarget(aSource, aProperty, aTruthValue,
     =>                                         aResult);
                if (NS_FAILED(rv))
                    return rv;
        
        nsTemplateRule::ComputeAssignmentFor(nsConflictSet&,nsTemplateMatch
*,int,Value *)const [nsTemplateRule.cpp:334]
                    mDataSource->GetTarget(source,
                                           binding->mProperty,
                                           PR_TRUE,
     =>                                    getter_AddRefs(target));
        
                    // Store the assignment in the match so we won't need to
                    // retrieve it again.
    nsTemplateMatch::GetAssignmentFor(nsConflictSet&,int,Value *)
[nsTemplateMatch.cpp:55]
                return PR_TRUE;
            }
            else {
     =>         return mRule->ComputeAssignmentFor(aConflictSet, this,
aVariable, aValue);
            }
        }
        
    nsXULTreeBuilder::CompareMatches(nsTemplateMatch *,nsTemplateMatch *)
[nsXULTreeBuilder.cpp:1906]
            nsIRDFNode* leftNode = VALUE_TO_IRDFNODE(leftValue);
        
            Value rightValue;
     =>     aRight->GetAssignmentFor(mConflictSet, mSortVariable, &rightValue);
            nsIRDFNode* rightNode = VALUE_TO_IRDFNODE(rightValue);
QA Contact: nbaca → technutz
mass re-assign.
Assignee: racham → sspitzer
(Assignee)

Comment 2

15 years ago
taking.
Assignee: sspitzer → dwitte
(Assignee)

Comment 3

15 years ago
Created attachment 123241 [details] [diff] [review]
patch v1

uh... this DIR_Server stuff is really crying out for C++...

this patch does a few things. 1) fixes the leak. 2) cleans up some code. 3)
replaces a bunch of PRBools with PRPackedBools in the DIR_Server struct (!)

we really should just throw that DIR_Server on the stack in
nsAbDirProperty::GetDirectoryProperties(), but that gets messy since it's a
plain-old-C struct, so freeing all its strings is a hell of a mess. easier to
use the helper function DIR_DeleteServer() provided - which wants to free the
struct too... and I don't really want to rewrite that now. ;)
(Assignee)

Comment 4

15 years ago
Comment on attachment 123241 [details] [diff] [review]
patch v1

hitting up caillon/dmose for reviews. sorry guys ;)
Attachment #123241 - Flags: superreview?(dmose)
Attachment #123241 - Flags: review?(caillon)
(Assignee)

Comment 5

15 years ago
*** Bug 203592 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 6

15 years ago
*** Bug 203594 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 7

15 years ago
*** Bug 203595 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

15 years ago
Attachment #123241 - Attachment is obsolete: true
Attachment #123241 - Flags: superreview?(dmose)
Attachment #123241 - Flags: review?(caillon)
(Assignee)

Comment 8

15 years ago
Created attachment 123247 [details] [diff] [review]
patch v1.1

blah, i meant to use rv |= there, thanks for catching caillon.
(Assignee)

Comment 9

15 years ago
Created attachment 123248 [details] [diff] [review]
patch v1.1, diff -y

caillon asked for a diff -y, so here it is ;)
(Assignee)

Updated

15 years ago
Attachment #123247 - Flags: superreview?(dmose)
Attachment #123247 - Flags: review?(caillon)
Attachment #123247 - Flags: review?(caillon) → review+
The patch fixes all of the dup leak bugs, as well as the leak mentioned herein.

Comment on attachment 123247 [details] [diff] [review]
patch v1.1

You've changed the semantics of this code in two ways:

* in the old code, if any of the setters failed in a debug build, it would
NS_ERROR.

* the old code would return an error if there was a failure; this doesn't.  I
don't think you necessarily need to propagate the exact error code that the
child saw back up the stack like the old code did, but at least document the
possible error codes that this method might return in the IDL with @exception.

Also, why bother setting *aDirectoryProperties to null at the beginning of the
method?
Attachment #123247 - Flags: superreview?(dmose) → superreview-
(Assignee)

Comment 12

15 years ago
Created attachment 123658 [details] [diff] [review]
patch v1.2

updated per dmose's comments on irc, thx!
Attachment #123247 - Attachment is obsolete: true
Attachment #123248 - Attachment is obsolete: true
Comment on attachment 123658 [details] [diff] [review]
patch v1.2

sr=dmose@mozilla.org
Attachment #123658 - Flags: superreview+
(Assignee)

Comment 14

15 years ago
Comment on attachment 123658 [details] [diff] [review]
patch v1.2

carrying over r=caillon, requesting approval for 1.4.

this is a pretty trivial patch that fixes a few leaks in addressbook code, and
improves memory footprint a smidgen.
Attachment #123658 - Flags: review+
Attachment #123658 - Flags: approval1.4?

Comment 15

15 years ago
Comment on attachment 123658 [details] [diff] [review]
patch v1.2

a=asa (on behalf of drivers) for checkin to 1.4
Attachment #123658 - Flags: approval1.4? → approval1.4+
(Assignee)

Comment 16

15 years ago
checked in to trunk.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 17

15 years ago
woah... fyi:

-624 bytes codesize on linux comet, -96 on winnt beast. that's a big saving on
linux just for killing multiple return statements... it looks like msvc does a
much better job of optimizing these than gcc does.
tested this with a clean tree under Purify, verified fixed.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.