604561 - Upload fails without an error (after memory leak?)

Status: RESOLVED FIXED

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

Description

[karl155]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10

After the memory usage has grown high enough there is a problem when you try to upload large files.

With a fresh started Firefox everything works ok.
After some time it only uploads the first 32 MiB of a file.
After some more time it only uploads the first 16 MiB of a file.
After some more time it crashes.

The primary error is a huge memory leak (maybe in Firebug). But the secondary error (this bug report) is that Firefox gives no indication that something is wrong.
Even the error log gives no information.
Closing the tab with the upload form does not help.

Here the code that was used for the upload form:
var xhr = new XMLHttpRequest();
xhr.open('POST', '?f='+encodeURIComponent(input.files.item(0).fileName)+'&s='+encodeURIComponent(input.files.item(0).fileSize));
xhr.setRequestHeader('Content-Type','application/octet-stream');
xhr.sendAsBinary(input.files.item(0).getAsBinary());

Reproducible: Always

Steps to Reproduce:
1. Upload a huge file
2. Go to Step 1



Maybe it is necessary to install an extension with a memory leak (like Firebug?)
Tested with Firebug 1.5.4, the latest release

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  John, does firebug always hook XHR, or only if the network panel is enabled?  I'm not aware of anything in necko per se that would cause upload failures here, short of actaul OOM.

Comment 2

[John J. Barton]

This sounds like 
http://code.google.com/p/fbug/issues/detail?id=3123
which was fixed at 1.6a20.  

Please install firebug 1.6X.0b1 from http://getfirebug.com/releases/firebug/1.6X and let us know.

Comment 3

[karl155]

This bug report is not about the memory leak, but about the missing filesize error detection in the upload.

I think this is what happens:
- After a while the memory is fragmented and there are no more contiguous 64 MiB blocks of memory available.
- Firefox asks for a 37 MiB but only gets a 32 MiB block.
- The code does not check that return value

The XHR progress indicator says that the upload completes and "total" is 32 MiB. So I think the function which does not check the error belongs to "getAsBinary".

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  GetAsBinary will fail to detect if AppendASCIItoUTF16 fails, yes...  I suppose it's conceivable that we're running out of address space here and that's happening.

karl155, if I created a test build that throws an exception in that situation, would you be able to give it a try?

Comment 5

[karl155]

(In reply to comment #4)
> karl155, if I created a test build that throws an exception in that situation,
> would you be able to give it a try?

Sounds good. Let's try that.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

How can it detect if it fails?  Don't we just abort?

Comment 7

[karl155]

I'm not sure if I got your point. But comparing the size of the string with the filesize would show if something went wrong with the string allocation.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> How can it detect if it fails?

+    PRUint32 oldLength = aResult.Length();
     AppendASCIItoUTF16(Substring(readBuf, readBuf + numRead), aResult);
+    if (aResult.Length() - oldLength != numRead) {
+      return DOMFileResult(NS_ERROR_OUT_OF_MEMORY);
+    }

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Don't we just abort?

Strings don't use infallible malloc yet.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #9)
> > Don't we just abort?
> 
> Strings don't use infallible malloc yet.

Ah, ok.  This was what I was getting at.

Comment 11

[Boris Zbarsky [:bzbarsky]]

An note that getAsBinary can't start using infallible malloc, for obvious reasons....

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, we need to make sure we keep using fallable APIs for all allocations in FileReader related to the loaded data.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Karl, try http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4b7861fafd19/tryserver-win32/firefox-3.6.12pre.en-US.win32.zip ?

Comment 14

[karl155]

It works. After 12 successful uploads the 13th failed with:

Error: Component returned failure code: 0x8007000e (NS_ERROR_OUT_OF_MEMORY) [nsIDOMFile.getAsBinary]

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Great!  This should fix the three relevant functions to check for OOM as needed.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 483685 [details] [diff] [review]
Fix

Let's get this in on trunk.

Jonas, do you think this is worth landing on 1.9.2?

Comment 17

[Johnny Stenback (:jst)]

Blocking.

Comment 18

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/mozilla-central/rev/733280bfc136

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 483685 [details] [diff] [review]
Fix

I'm a little worried about 1.9.2 because there this could cause scripts that currently don't throw to throw.  But that might be better than silently doing the wrong thing....

Comment 20

[LegNeato]

Comment on attachment 483685 [details] [diff] [review]
Fix

Denied for branch. This doesn't really look to be worth it. The problem seems very edge-case, has no dupes or votes, and wasn't found for the whole life of 3.6 so far.

Let's just fix this on trunk unless there is a compelling reason why this needs to come back.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Jonas, do we need similar changes for nsDOMFileReader?  That seems to have some copy/pasted code....

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, yeah, I missed that this changed nsDOMFile functions, which are deprecated, rather than the nsDOMFileReader functions :(

Comment 23

[Boris Zbarsky [:bzbarsky]]

> do we need similar changes for nsDOMFileReader? 

Looks like those happened in bug 613472.