115815 - nsFormSubmitter leaks like a sieve

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

[Håkan Waara]

nsFormSubmitter leaks a lot of strings and stuff.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Isn't this just a dup of the "clean up this function" bug?  Or are there issues 
throughout the file?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Nevermind.  I got it... :)

Comment 3

[Håkan Waara]

I filed this before commenting in the other bug, and you read your bugmail
sorted by date. Right?  :-)

Comment 4

[Boris Zbarsky [:bzbarsky]]

yup

Comment 5

[Nivedita (gone)]

Håkan Waara
Can you please elaborate the bug description. And can you please enclose the 
testcases if any and observations supporting the bug, so as to help me in 
cornering the bug.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Nivedita, see bug 112973, comment #7 and bug 112973 comment #15 (this bug is 
meant to address the latter, really).

Comment 7

[Nivedita (gone)]

Attachment: patch file fixing the memory leaks in nsFormSubmitter.cpp

Added a class nsStringAutoArrayPtr which handles the deletion of nsString
array. Used nsCAutoString instead of char* so as to create local variables on
stack.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Do you really need the |char *| versions of the strings _and_ the 
|nsCAutoString| versions?  Why are the char* versions needed at all?

Comment 9

[Nivedita (gone)]

Attachment: patch file incorporating the comments given by Boris Zbarsky

Eliminated the redundant usage of char*.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> +          nsCAutoString fname;

Transfer the comment about "path removed" as well?

> +          if (!name.get()) {

Wouldn't this be better as |if (name.IsEmpty()) {| ?  Same thing for 
!value.get()

> +            name.Adopt(ToNewCString(names[valueX]));

Is there a reason to allocate this on the stack?  Wouldn't it make more sense to 
do  |name = NS_ConvertUCS2toUTF8(names[valueX]);| ?

> +          value.Adopt(nsLinebreakConverter::ConvertLineBreaks(value.get(),

Is this safe?  I know strings have some issues with things like | foo = bar + 
foo | at times due to lazy copying (normally a good thing).  This is not exactly 
the same, but please test this well....

> PL_strlen(name.get())

Just use name.Length() ?  Same thing for fname.

> +              PL_strlen(value.get())) { // Don't bother if no file specified

Use !value.IsEmpty() instead -- it's much faster, for one thing.

One problem I see with using .get() and autostrings is embedded nulls in the 
strings...  Are those a possible issue here?  If so, how does Adopt() handle 
those?

Comment 11

[Nivedita (gone)]

Attachment: Incorporated the comments given by Boris Zbarsky

Using the nsCAutoString methods for getting the length and for check if empty. 


if we use name = NS_ConvertUCS2toUTF8(names[valueX]), this also causes realloc
when the Assign method is called when the value is getting assigned to name,
hence the essence of avoiding the alloc using ToNewCString is lost. Please
convey if I am missing something here. 

And the problem of using nsCAutoStrings was already existing, as char* were
used. Hence we could address this issue as an another bug

Comment 12

[Håkan Waara]

Reassign to nivedita who's working on this now.

Isn't this new |nsStringAutoArrayPtr| class something we'd want globally (i.e.
as a public string class in the strings module)? Jag?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> this also causes realloc when the Assign method is called when the value is
> getting assigned to name

Only if the value is more than 80 characters long.  Auto strings have a 
stack-allocated internal buffer that holds 80 characters.  They don't allocate 
unless asked to hold more than that.  In this case, this is the name of a form 
control, so 80 characters is not likely to be exceeded.

The last patch looks good, though.  r=bzbarsky

Comment 14

[jag (Peter Annema)]

We already have an nsAutoArrayPtr which you could use here:

xpcom/base/nsAutoPtr.h

TestAutoPtr.cpp in the same directory shows how to use it.

For now the Adopt(ToNewCString(...)) method is not encouraged. Soon I'll add
generating converters which will enable us to skip the second allocation on
Append (string sharing should take care of this on Assign).

Comment 15

[Vinay Badami]

Had spoken to dbaron about nsAutoPtr and he had said that this was work in
progress. We can switch to this once we this header file is gets published to
dist/include/xpcom/. Until then, we would need our local AutoPtr class. This was
my suggestion to Nivedita.

How would u want us to handle the Adopt(ToNewCString(...)) ? Is there an
alternative that we are missing ?

Comment 16

[jag (Peter Annema)]

No, Adopt(ToNewCString()) is probably what you should use for now.

Comment 17

[Vinay Badami]

Can someone do a super review for this and check this in please ? None of us
have checkin access at this point.

Comment 18

[jag (Peter Annema)]

Comment on attachment 63487 [details] [diff] [review]
Incorporated the comments given by Boris Zbarsky

Could you add a comment pointing to dbaron's nsAutoArray and a suggestion that
the code be switched to that once it's been made public?

sr=jag with that.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comments added and checked in.