allow edge to gc chunks

[butonic]

Try uploading > 5GB with edge. It does not matter if chunks are used or not. Edge will kill the tab process once it hits 5GB. This can be monitored quite nicely when keeping the EDGE dev tools memory tab open. When the process is killed the tools get closed. Sadly, memory is not even freed after an upload completes.

This PR allows edge to cleanup chunks. Currently, by only omitting the actual upload data from the options used to initialize the progress. Maybe it makes more sense to always omit it ... my js is not good enough to wrap my head around that.

Comments welcome.

[blueimp]

Thanks for your contribution @butonic
As far as I understand you, you're encountering a memory leak with Edge and file upload progress reporting.
The part that I don't understand is how your code snippet is supposed to fix things with this snippet:

var clone = Object.assign({}, o);
delete clone.data;

You're assigning all option properties to a new object and then remove the data property from that new object.
Since you're not using this new clone object it should be simply dismissed for garbage collection by the browser at the end of the function block.
And since you're not mutating any other objects (o will be unchanged), I fail to see how this can have an impact?

Could you confirm that this change fixes the memory leak on edge?
And if yes, do you have an explanation for this behavior?
Sorry but as of right now, I fail to see how this fix could have any actual impact, despite maybe triggering undocumented behavior in edge.

[butonic]

updated pr incoming ... I forgot to actually use clone as a parameter to _initProgressListener.

That method seems to indirectly reference the object by itself, or rather by an anonymous handler listening on progress events.

This seems to prevent the gc to collect the chunk data.

[butonic]

AFAICT _initProgressListener() is the culprit:

jQuery-File-Upload/js/jquery.fileupload.js

Lines 417 to 435 in a451d60

	_initProgressListener: function (options) {
	var that = this,
	xhr = options.xhr ? options.xhr() : $.ajaxSettings.xhr();
	// Accesss to the native XHR object is required to add event listeners
	// for the upload progress event:
	if (xhr.upload) {
	$(xhr.upload).bind('progress', function (e) {
	var oe = e.originalEvent;
	// Make sure the progress event properties get copied over:
	e.lengthComputable = oe.lengthComputable;
	e.loaded = oe.loaded;
	e.total = oe.total;
	that._onProgress(e, options);
	});
	options.xhr = function () {
	return xhr;
	};
	}
	},

In 419 xhr might be taken from the options.
If xhh.upload exists line 423 will bind an anonymous function to 'progress' events of the xhh.upload object.
Finally, line 431 replaces the options.xhr with another anonymous function, creating another indirection.

Im not too much into garbage collection, but I have a feeling that the edge implementation does not yet handle that good enough.

[butonic]

hm, indeed, making the options.xhr replacement conditional also allows the objects to be garbage collected.

--- apps/files/js/jquery.fileupload.js	(date 1529351608000)
+++ apps/files/js/jquery.fileupload.js	(date 1529351608000)
@@ -428,9 +428,11 @@
                     e.total = oe.total;
                     that._onProgress(e, options);
                 });
-                options.xhr = function () {
-                    return xhr;
-                };
+                if (options.xhr && options.xhr() !== xhr) {
+					options.xhr = function () {
+						return xhr;
+					};
+				}
             }
         },

[butonic]

hm the condition is wrong, let me see if I can polish this.

[butonic]

debugging shows:

1. options.xhr is unset in 419
2. commenting lines 431-433 allows gc to happen, upload still completes successfully

@blueimp Why is the xhr added to the options when initializing the progress listener?
It was added with the rewrite 7 years ago. It seems back then handleGlobalProgess had the xhr as a parameter, but it was never used:

jQuery-File-Upload/jquery.fileupload.js

Lines 211 to 234 in 8ae0b06

	handleGlobalProgress = function (event, files, index, xhr, settings) {
	var progressEvent,
	loaderList,
	globalLoaded = 0,
	globalTotal = 0;
	if (event.lengthComputable && typeof settings.onProgressAll === func) {
	settings.progressLoaded = parseInt(
	event.loaded / event.total * getProgressTotal(files, index, settings),
	10
	);
	loaderList = multiLoader.getList();
	$.each(loaderList, function (index, item) {
	// item is an array with [files, index, xhr, settings]
	globalLoaded += item[3].progressLoaded || 0;
	globalTotal += getProgressTotal(item[0], item[1], item[3]);
	});
	progressEvent = createProgressEvent(
	true,
	globalLoaded,
	globalTotal
	);
	settings.onProgressAll(progressEvent, loaderList);
	}
	},

Also if settings.onProgess wes a function it would be called with the xhr in handleLoadEvent

jQuery-File-Upload/jquery.fileupload.js

Line 246 in 8ae0b06

settings.onProgress(progressEvent, files, index, xhr, settings);

and handleProgressEvent

jQuery-File-Upload/jquery.fileupload.js

Line 284 in 8ae0b06

settings.onProgress(progressEvent, files, index, xhr, settings);

Access to a .xhr property is nowhere to be found in https://github.com/blueimp/jQuery-File-Upload/tree/b0b003c74cfbccf5801588602d147a6256d10e17 , the jQuery UI Widget rewrite.

Today, the only place that might rely on the objects having the xhr property is

jQuery-File-Upload/js/cors/jquery.postmessage-transport.js

Line 66 in a451d60

xhrUpload = options.xhr().upload;

After fiddling around a bit, it seems that just cloning the options already allows edge to gc the chunks.

Currently, this PR should allow code relying on options.xhr() to continue to work. While getting here I was under the impression that options.xhr should just not be set. But to be safe and backwards compatible it seems this is the right workaround.

[butonic]

It seems to be a variant of https://blog.meteor.com/an-interesting-kind-of-javascript-memory-leak-8b47d2e7f156

[butonic]

the options are also referenced by the jquery cache ... so maybe https://makandracards.com/makandra/31325-how-to-create-memory-leaks-in-jquery is related .... too late to dig further. will have another look tomorrow.

[butonic]

it seems I can clean this up on our side:

				fileupload.on('fileuploadchunkdone', function(e, data) {
					$(data.xhr().upload).unbind('progress').remove();
				});

will test tomorrow ... really getting too late

[butonic]

Ok, so actually just unbinding the progress listener is enough to let gc happen:

				fileupload.on('fileuploadchunkdone', function(e, data) {
					$(data.xhr().upload).unbind('progress');
				});

The cleanest way I could come up with was using always() to remove the progress listener. Updated this PR accordingly, Maybe this saves others some time, trying to figure out why uploads fail randomly.

[blueimp]

Thanks a lot for your investigation and fix, I've included a slightly modified version of it in the latest release.
And sorry for the late response. :|