203593 - Memory leak of 672 bytes from 4 blocks allocated in PR_Malloc

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, defect)

Description

[[e-mail] (gone - use [e-mail] instead)]

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);

Comment 1

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 2

[[e-mail]]

taking.

Comment 3

[[e-mail]]

Attachment: 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. ;)

Comment 4

[[e-mail]]

Comment on attachment 123241 [details] [diff] [review]
patch v1

hitting up caillon/dmose for reviews. sorry guys ;)

Comment 5

[[e-mail]]

*** Bug 203592 has been marked as a duplicate of this bug. ***

Comment 6

[[e-mail]]

*** Bug 203594 has been marked as a duplicate of this bug. ***

Comment 7

[[e-mail]]

*** Bug 203595 has been marked as a duplicate of this bug. ***

Comment 8

[[e-mail]]

Attachment: patch v1.1

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

Comment 9

[[e-mail]]

Attachment: patch v1.1, diff -y

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

Comment 10

[[e-mail] (gone - use [e-mail] instead)]

The patch fixes all of the dup leak bugs, as well as the leak mentioned herein.

Comment 11

[OOO until Jun 29 - Dan Mosedale (:dmosedale, :dmose)]

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?

Comment 12

[[e-mail]]

Attachment: patch v1.2

updated per dmose's comments on irc, thx!

Comment 13

[OOO until Jun 29 - Dan Mosedale (:dmosedale, :dmose)]

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

sr=[e-mail]

Comment 14

[[e-mail]]

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.

Comment 15

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to 1.4

Comment 16

[[e-mail]]

checked in to trunk.

Comment 17

[[e-mail]]

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.

Comment 18

[[e-mail] (gone - use [e-mail] instead)]

tested this with a clean tree under Purify, verified fixed.