Closed Bug 112973 Opened 22 years ago Closed 22 years ago

Memory leak in file upload functionality (INPUT TYPE="file")

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla0.9.7

People

(Reporter: ilias.argyriou, Assigned: hwaara)

References

()

Details

Attachments

(1 file, 6 obsolete files)

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.6) Gecko/20011120
BuildID:    2001112009

I've been uploading several mp3 files to my myplay.com account (an online
storage service for music files). myplay.com allows you to upload one file at a
time. 

Every time I upload a new file, the memory usage of mozilla jumps by an amount
approximately equal to the size of the previous file uploaded.

When I started uploading the first mp3 file memory usage was at 34 MB. After I
finished uploading the 11th file, memory usage was at 81 MB. The total size of
the 11 files uploaded is 57.5 MB.

See below for detailed log and instructions on how to reproduce.

Same thing happened to me last night and at some point my computer started doing
a great deal of swapping. I had to exit mozilla to release all the memory used.
I tried to clear the memory cache via the preferences dialog before exiting but
it made no difference.

Reproducible: Always
Steps to Reproduce:
Here's what I've been doing along with the memory usage numbers for each operation:

Mem Usg  Operation
=======  =========
24.8 MB  Rendering of personal myplay.com page completed
25.7 MB  Upload popup window completes rendering
32.2 MB  File selection native windows dialog pops up
34.4 MB  Selected file #1 (6.2 MB) for upload
34.4 MB  Upload started
34.4 MB  Completed
34.4 MB  Selected file #2 (6.1 MB) for upload
40.5 MB  Upload started
40.5 MB  Completed
40.6 MB  Selected file #3 (4.7 MB) for upload
45.6 MB  Upload started
45.6 MB  Completed
45.6 MB  Selected file #4 (4.2 MB) for upload
50.0 MB  Upload started
50.0 MB  Completed
50.0 MB  Selected file #5 (5.6 MB) for upload
55.9 MB  Upload started
55.9 MB  Completed
55.9 MB  Selected file #6 (4.4 MB) for upload
59.7 MB  Upload started
60.0 MB  Completed
60.0 MB  Selected file #7 (7.3 MB) for upload
63.0 MB  Upload started
63.3 MB  Completed
63.3 MB  Selected file #8 (5.0 MB) for upload
66.3 MB  Upload started
66.5 MB  Completed
66.6 MB  Selected file #9 (3.0 MB) for upload
69.5 MB  Upload started
69.7 MB  Completed
69.7 MB  Selected file #10 (7.5 MB) for upload
76.3 MB  Upload started
76.8 MB  Completed
76.8 MB  Selected file #11 (3.5 MB) for upload
78.0 MB  Upload started
80.0 MB  Completed
81.0 MB  Edit -> Preferences -> Advanced -> Cache -> Clear Memory Cache
22.0 MB  File -> Exit

Actual Results:  Every time a *new* file upload operation starts, memory jumps
by an amount equal to the size of the file *previously* uploaded. During the
upload memory usage stays flat.

Expected Results:  Memory usage should stay constant when a new upload starts.

My memory cache setting is 4096 KB.

I searched for bugs related to 'file dialog', 'upload', 'memory' and 'input
type="file"'. The following results may be of interest:

15320 - not related?
26719 - related + fixed for M18
56676 - related + fixed - 0.9.6 branch 09-Nov-2001
69027 - not related?

I'll try to replicate this with a different web site, briefcase.yahoo.com
perhaps. I'll also try to replicate this on a linux box, time permitting.
The myplay.com upload popup window uses the <input type="file"> tag.

Here's the complete HTML code:

<html>
<head>
<title>MyPlay Browse My Computer</title>
<link rel="stylesheet" type="text/css" href="/style.css">
</head>
<body bgcolor=#ffffcc text=#000000 link=#003399 vlink=#003399 alink=#ff0000
marginheight=10 

marginwidth=10 topmargin=10 leftmargin=10>


<form ENCTYPE="multipart/form-data"
ACTION="http://abv-bc6.myplay.com/c/u/mp?1007163957070" 

METHOD="post">
<input type="hidden" name="wl" value="2">
<span class=vdgn10>
<b>1.</b> &nbsp;Click the "Browse" button to locate music file.
</span>
<table cellpadding=0 cellspacing=0 border=0>
<tr>
<td height=5><spacer TYPE=BLOCK WIDTH=1 HEIGHT=5></td>
</tr>
</table>
<input type=file name="file-to-upload-01" size=21><br>
<span class=vdgn10>
<a href="#" 

onClick="parent.opener.location='/mp/help/help.jsp?pname=top_gmhd';parent.opener.window.focu

s();return false;">Problems copying to your Locker?</a><br>
<span class=vdgn12><br></span>
<b>2.</b> &nbsp;Click the "Copy" button to put the music file into your Locker.<br>
<a 

href="javascript:parent.frames[2].location='http://abv-bc6.myplay.com/c/progress?10071639570

70';document.forms[0].submit();"><img src="/gifs/but_copy.gif" width=54
height=21 vspace=5 

alt="Copy" border=0></a>
</font>
</form>


</body>
</html>
we should dealocate the arena memory reserved for the uploaded file.
ok, i'll investigate if is form submission, doc shell or necko...
Status: UNCONFIRMED → NEW
Ever confirmed: true
ok is not necessarily allocated in the arena (was just a suspicion based on the
described behaviour)
Boris:

could you please investigate this? you're familiar with the ProcessAsMultiPart
stuff. if it's necko, just pass it to them :-)
Might be an idea to CC Boris too, if you need his help. :-)
Um..  I won't be able to take a look for a few weeks... :( I'm about to go on
vacation and I won't have a runnable mozilla.  Once I get back I could look, sure.
Blocks: 114187
Nevermind.  I did a quick skim through the function... This code is a
piece of shit.

  nsString* names  = new nsString[maxNumValues];
  nsString* values = new nsString[maxNumValues];
  if (NS_FAILED(controlNode->GetNamesValues(maxNumValues, numValues,
                                            values, names))) {
     continue;
  }

will leak |names| and |values| in the continue case.

Later there is:

      if (names[valueX].IsEmpty()) {
        continue;
      }

which will leak |name|, |value|, and |fname| (all char*s allocated
earlier in the loop)

These two patterns are repeated twice.  Once when calculating content
length and once when actually creating the temporary post file.

Now the real problem (this bug) comes at:

  // Print file contents
  PRInt32 size = 1;
  while (1) {
    char* readbuffer = nsnull;
    rv = contentFile->Read(&readbuffer, BUFSIZE, &size);
    if (NS_FAILED(rv) || 0 >= size) break;
    wantbytes = size;
    rv = outStream->Write(readbuffer, wantbytes, &gotbytes);
    if (NS_FAILED(rv) || (wantbytes != gotbytes)) break;
  }

The nsFileSpecImpl::Read() function does the following:

  if (!*buffer)
    *buffer = (char*)PR_Malloc(requestedCount + 1);

(http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsFileSpecImpl.cpp#615)   

we pass in a pointer to nsnull as buffer, so it mallocs.  Then we
never free the pointer.  So yes, we leak an amount of memory equal to the size
of the file rounded up to the nearest (BUFSIZE+1) (1025 bytes in our case).

Why are we monkeying with |readbuffer| anyway? We have a perfectly good
stack-allocated |char buffer[BUFSIZE];| lying around.  We could use the pointer
to that (since we get told how many bytes actually got read in), and avoid the
allocations....

In any case, this one should be easy to fix by just adding a PR_Free on
readbuffer or using the stack buffer.

Bonus points would be actually fixing the other memory leaks in this function.  :)
Severity: normal → major
Keywords: mlk
OS: Windows NT → All
Hardware: PC → All
No longer blocks: 114187
*** Bug 114187 has been marked as a duplicate of this bug. ***
Attached patch Patch (obsolete) — Splinter Review
No more leaking. Here are some results from bz's testing:

Before we leaked the whole file (e.g. 9MB for a 9MB file), if the file was
bigger than ~1K. Now the memory usage will go up to 3MB, before it gets freed.
Comment on attachment 60970 [details] [diff] [review]
Patch

r=bzbarsky over IRC
Attachment #60970 - Flags: review+
reassign
Assignee: alexsavulov → hwaara
I should clarify that.  Before we leaked 9mb per upload (I was using a 9mb
file).  Now we go up 3mb the first upload and do not go up any more at all on
subsequent uploads.  So the 3mb is us putting things in cache or loading modules
or something... no idea what.  But it's not this leak. :)
Here's a little something for you to sr=, shaver. :-)
Comment on attachment 60970 [details] [diff] [review]
Patch


>@@ -1057,14 +1074,15 @@
>               }
>               // Print file contents
>               PRInt32 size = 1;
>+              char *readbuffer = nsnull;
>               while (1) {
>-                char* readbuffer = nsnull;
>                 rv = contentFile->Read(&readbuffer, BUFSIZE, &size);
>                 if (NS_FAILED(rv) || 0 >= size) break;
>                 wantbytes = size;
>                 rv = outStream->Write(readbuffer, wantbytes, &gotbytes);
>                 if (NS_FAILED(rv) || (wantbytes != gotbytes)) break;
>               }
>+              PR_FREEIF(readbuffer);
>               NS_RELEASE(contentFile);
>               // Print CRLF after file
>               wantbytes = PL_strlen(CRLF);

This part really doesn't look right.  Do you need to free it each
iteration, or do you only need to free if you hit one break but
not the other?	Also, use the same pattern with nsMemory::Free rather
than pulling PR_FREEIF into this.

Also, it really seems like you need a better solution here -- half the
function shouldn't be freeing memory.  It seems like you either need
objects that will free memory for you or (eek!) gotos.
Basically, the first call to Read() will malloc and assign to readbuffer (see
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsFileSpecImpl.cpp#615).  After
that the newly malloc-ed buffer is just reused.

We could make this clearer by malloc-ing the buffer ourselves before ever going
into the loop, perhaps.  Write() does nothing to the buffer; it just dumps it to
a file.

This function in general is guilty of ugly code, use of nsFileSpec, and other
atrocities.  It all needs to be cleaned up.  I'm willing to take that (just not
in the 0.9.7 timeframe), but I feel that the really big leak of the file being
uploaded should be fixed asap.
Reassign to bzbarsky for a better fix.
Assignee: hwaara → bzbarsky
There is no way I can get to this before early January.  The leak should be
fixed sometime soonish, however.  Punting to Alexandru for getting at least the
leak fixed.  After that, feel free to reassign to me for cleanup work....
Assignee: bzbarsky → alexsavulov
Keywords: mlk
Target Milestone: --- → mozilla0.9.8
Attached patch Fix + cleanup (obsolete) — Splinter Review
This includes string cleanup in that function too, but if you demand even more
cleanup we can keep the bug open.

The high priority thing right now is to fix the huge leak. Boris, wanna r=
this?
Attachment #60970 - Attachment is obsolete: true
Comment on attachment 61025 [details] [diff] [review]
Fix + cleanup

>+              name.Assign(UnicodeToNewBytes(names[valueX].get(),

You want to be using Adopt, not Assign, methinks... this applies throughout.

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

This should be IsEmpty()

>+              nsXPIDLCString readbuffer;
>               while (1) {
>-                char* readbuffer = nsnull;
>-                rv = contentFile->Read(&readbuffer, BUFSIZE, &size);
>+                rv = contentFile->Read(getter_Copies(readbuffer), BUFSIZE, &size);

I don't know enough avout nsXPIDLCString to know whether this does the right
thing.	For example, what happens here if the string's internal buffer is
smaller than BUFSIZE?

>                 if (NS_FAILED(rv) || 0 >= size) break;
>                 wantbytes = size;
>-                rv = outStream->Write(readbuffer, wantbytes, &gotbytes);
>+                rv = outStream->Write(readbuffer.get(), wantbytes, &gotbytes);

How is nsXPDILCString about handling embedded nulls?  (this also applies 
throughout)
CC jag for string answers to bz's questions.
> We could make this clearer by malloc-ing the buffer ourselves before ever going
> into the loop, perhaps.  Write() does nothing to the buffer; it just dumps it to
> a file.

Why not just use a buffer on the stack, then?

char foo[BUFFER_SIZE];
Read(&foo, ....)

does not compile with gcc (because (char[])* is not the same as char**).

If casting &foo to char** is safe in this case, then I'm all for using the
existing stack buffer.  :)
I don't know anything about nsXPIDLCString implementation or whether casting
&foo to char** is safe in this case...

Should I just make a patch for the .IsEmpty() and .Adopt() nits and then we can
get it in?
It's a const-cast from |char * const *| to |char **|.  But why would you need a
|char**| at all, rather than a |char*|?
If I have time tomorrow (no promises), I'll create a patch that fixes the big
leak (the buffer) so we can fix it for 0.9.7, then boris can handle the rest of
the leaks + cleanup for 0.9.8.
Attached patch Fix for the big leak (obsolete) — Splinter Review
This is a patch for only the big leak. I really think we should this for now,
and definitely for 0.9.7.   Boris promised to take care of the rest of this bug
(more leak-fixing, string usage cleanup etc.) for 0.9.8.

Reviews?
Attachment #61025 - Attachment is obsolete: true
Um... Like I said, I don't think using nsXPIDLCString here is correct.  The Read
function will just write to the buffer (not assign to the pointer, and the
buffer is almost certainly not big enough.   Have you actually uploaded a large
file using this fix?  I'm almost certain it will just crash.
Attached patch Better fix for the big leak (obsolete) — Splinter Review
Addresses bz's and dbaron's comments.

Sorry I don't have time to test this, because I'm dead-tired and should've been
in bed a long time ago.
Attachment #61355 - Attachment is obsolete: true
Comment on attachment 61378 [details] [diff] [review]
Better fix for the big leak

r=bzbarsky

Looks good.  I tested it and it compiles, runs, uploads files, and does not
leak them.
Attachment #61378 - Flags: review+
attinasi, please sr= this ASAP.   Thanks!
Attached patch Better fix for the big leak (v2) (obsolete) — Splinter Review
Added a missing error-check that brendan pointed out.
Attachment #61378 - Attachment is obsolete: true
That leaks contentFile...
Too...many...patches
Attachment #61495 - Attachment is obsolete: true
Comment on attachment 61508 [details] [diff] [review]
Last one, I promise!

r=bzbarsky
Attachment #61508 - Flags: review+
I want the fix for the big leak to go into 0.9.7, and I need an sr= and a= in
order to do that. I've emailed Attinasi this week for sr=... but no response (yet).

Brendan, can you sr=?
Target Milestone: mozilla0.9.8 → mozilla0.9.7
Comment on attachment 61508 [details] [diff] [review]
Last one, I promise!

sr=attinasi
Attachment #61508 - Flags: superreview+
The patch for the big leak is checked into the trunk. 

I have emailed drivers for approval to check it into 0.9.7 (although someone 
else will have to do it for me).

-> Bzbarsky, who'll fix all the remaining leaks and mess in this function.
Assignee: alexsavulov → bzbarsky
Target Milestone: mozilla0.9.7 → ---
Ok, approving for checkin to the 0.9.7 branch.

/be
Blocks: 114455
Keywords: mozilla0.9.7+
BTW, it's probably best to spin up a new bug on the other issues with this
class.  This bug was filed for the major mem leak.  Filed bug 115815 on bz to
track the other problems.

Now, who can check my patch into 0.9.7?  Brendan?
Assignee: bzbarsky → hwaara
fixed on branch too.
Status: NEW → RESOLVED
Closed: 22 years ago
Keywords: fixed0.9.7
Resolution: --- → FIXED
Target Milestone: --- → mozilla0.9.7
No longer blocks: 114455
I dont know how to verify this, one of the developers please do it?
Keywords: verifyme
verified we no longer leak gobs of memory here....
Status: RESOLVED → VERIFIED
Keywords: verifyme
Comment on attachment 8560236 [details] [diff] [review]
Part1. Dynamically adjust calculations using timestampoffset

oops.

Forgot a 2 in the bug number (1129732)
Flags: needinfo?(jyavenard)
Attachment #8560236 - Attachment is obsolete: true
Attachment #8560236 - Flags: review?(matt.woodrow)
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.