Closed Bug 604561 Opened 11 years ago Closed 11 years ago

Upload fails without an error (after memory leak?)

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- final+
status1.9.2 --- wontfix

People

(Reporter: karl156, Assigned: bzbarsky)

Details

Attachments

(1 file)

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
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.
Component: Networking: File → Networking: HTTP
QA Contact: networking.file → networking.http
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.
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".
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?
Status: UNCONFIRMED → NEW
Component: Networking: HTTP → DOM
Ever confirmed: true
QA Contact: networking.http → general
(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.
How can it detect if it fails?  Don't we just abort?
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.
> 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);
+    }
> Don't we just abort?

Strings don't use infallible malloc yet.
(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.
An note that getAsBinary can't start using infallible malloc, for obvious reasons....
Yeah, we need to make sure we keep using fallable APIs for all allocations in FileReader related to the loaded data.
It works. After 12 successful uploads the 13th failed with:

Error: Component returned failure code: 0x8007000e (NS_ERROR_OUT_OF_MEMORY) [nsIDOMFile.getAsBinary]
Attached patch FixSplinter Review
Great!  This should fix the three relevant functions to check for OOM as needed.
Assignee: nobody → bzbarsky
Attachment #483685 - Flags: review?(jonas)
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?
Attachment #483685 - Flags: approval2.0?
Priority: -- → P1
Whiteboard: [need approval]
blocking2.0: --- → ?
Blocking.
blocking2.0: ? → final+
Attachment #483685 - Flags: approval2.0?
Whiteboard: [need approval] → [need landing]
http://hg.mozilla.org/mozilla-central/rev/733280bfc136
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla2.0b8
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....
Attachment #483685 - Flags: approval1.9.2.12?
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.
Attachment #483685 - Flags: approval1.9.2.12? → approval1.9.2.12-
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Jonas, do we need similar changes for nsDOMFileReader?  That seems to have some copy/pasted code....
Oh, yeah, I missed that this changed nsDOMFile functions, which are deprecated, rather than the nsDOMFileReader functions :(
> do we need similar changes for nsDOMFileReader? 

Looks like those happened in bug 613472.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.