Closed Bug 112973 Opened 23 years ago Closed 23 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.7

People

(Reporter: ilias.argyriou, Assigned: hwaara)

References

(
URL
)

Details

Attachments

(1 file, 6 obsolete files)

Last one, I promise!
1.22 KB, patch
bzbarsky
: review+
attinasi
: superreview+
Details | Diff | Splinter Review
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: 23 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.

Attachment

General

Creator:
Created:
Updated:
Size: