939510 - revokeObjectURL() does not free up memory if the BLOB URL was served as a download

Status: RESOLVED FIXED

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

Description

[Bram van der Kolk]

Attachment: firefoxleak.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.14 Safari/537.36

Steps to reproduce:

1. Create a BLOB with binary data.
2. Create a BLOB URL with window.URL.createObjectURL()
3. Serve the binary data as a file download (by using the "download" attribute in the <a> element to define the filename).
4. Use window.URL.revokeObjectURL() to clear the BLOB from memory.


Actual results:

Memory does not get cleared. Suspect that the running download creates another reference to the BLOB that never gets cleared so that the BLOB never gets garage collected. This is a relatively new feature, as support for the "download" attribute in the <a> element was only added to Firefox 20.

For a proof of concept see the attached file: firefoxleak.html

The bug also occurs when calling window.URL.revokeObjectURL() _after_ the download has completed, so it cannot be worked around.


Expected results:

Upon download completion there should not be any reference to the BLOB any longer, allowing the garbage collector to free up the memory.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Does this only happen when downloading?  So if you load a blob in an iframe, for example, the problem doesn't occur?

Comment 2

[Bram van der Kolk]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Does this only happen when downloading?  So if you load a blob in an iframe,
> for example, the problem doesn't occur?

What exactly do you mean with "load a BLOB in an iframe"? Our goal is to serve binary data as file download to the user (the files are not pre-viewable in the browser).

To find out whether this memory leak also applies in other situations (e.g. with rendering content inside the browser), I would suggest the following approach:
1. Create BLOB URL's for various images.
2. Render them to the DOM
3. Remove them from the DOM.
4. Use window.URL.revokeObjectURL() and see if the data stays in memory.

This would however be different from the issue described above, so it would be a separate (related) issue.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The question is whether this is an issue in blob code or in download code.

Comment 4

[Bram van der Kolk]

(In reply to Boris Zbarsky [:bz] from comment #3)
> The question is whether this is an issue in blob code or in download code.

Probably in the download code? When the BLOB URL is passed as a download does it create another reference to the BLOB? It should probably free that reference when: 
a) the user cancels the download
b) the download completes

Comment 5

[Boris Zbarsky [:bzbarsky]]

There is nothing blob-specific in the download code.  Which is why I think we should check whether this in fact only happens with downloads.

Comment 6

[Bram van der Kolk]

I think the fact that there is nothing blob-specific in the download code is exactly the problem. Shouldn't a blob-download free the reference upon completion? (note that a blob download can take several minutes when it's a large file)

It only seems to happen with downloads. I tested the loading of blob url's in the browser (by passing them to a img.src) and in this case everything works as expected (e.g. no memory leak):

var url = function()
{
	var a = URL.createObjectURL(new Blob([Array(1000001).join('x')]));		
	(new Image()).src = a;
	setTimeout(function(a)
	{	
		console.log(a);
		URL.revokeObjectURL(a);
		url();
	},500,a);
}
url();

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Shouldn't a blob-download free the reference upon completion?

Not any more so than any other load of the data, I would think.

Comment 8

[Bram van der Kolk]

Here is another example that does work correctly. According to my tests blob url downloads cause the handle leak, while other uses of the blob url don't.

<iframe id="fr"></iframe>
<script type="text/javascript">
var url = function()
{
	var a = URL.createObjectURL(new Blob([Array(1000001).join('x')]));	
	document.getElementById('fr').src=a;
	setTimeout(function(a)
	{	
		console.log(a);
		URL.revokeObjectURL(a);		
		url();
	},500,a);
}
url();
</script>

Comment 9

[Boris Zbarsky [:bzbarsky]]

So there are two things going on here:

1)  Something in the Firefox UI (the download manager?) is keeping the nsIChannel alive for
    the download.
2)  An nsInputStreamChannel never nulls out its mContentStream, even after OnStopRequest.

I think we should fix #2, which would drop the data in this case.  Jason?

I also think we should file a separate bug on #1, because that just seems fishy....

Comment 10

[:Paolo Amadini]

(In reply to Boris Zbarsky [:bz] from comment #9)
> I also think we should file a separate bug on #1, because that just seems
> fishy....

Filed bug 941726 with a patch that indirectly fixes this leak. We should probably fix cause #2 as well.

Comment 11

[Jason Duell]

> 2)  An nsInputStreamChannel never nulls out its mContentStream, even after OnStopRequest.
> 
> I think we should fix #2, which would drop the data in this case.  Jason?

That looks like a sensible approach to me.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: nsInputStreamChannel should null out mContentStream when it's done loading it, so that long streams can die as needed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8351e6c6b6

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ea8351e6c6b6