Page MenuHomePhabricator

In uw.controller.Details, `upload.off( 'remove-upload', … )` does not actually remove the event handlers
Closed, ResolvedPublic

Description

In uw.controller.Details, upload.off( 'remove-upload', … ) does not actually remove the event handlers. This is a bug in ff53d993ce9a456718b1528048e319c9ed69b22f, that I didn't notice when reviewing it.

We set up some event handlers when entering the Details step:

	uw.controller.Details.prototype.moveTo = function ( uploads ) {
		var details = this;

		...

		$.each( uploads, function ( i, upload ) {
			upload.on( 'remove-upload', details.removeUpload.bind( details, upload ) );
		} );
	};

We try to clear them when leaving it:

	uw.controller.Details.prototype.moveNext = function () {
		var controller = this;

		$.each( this.uploads, function ( i, upload ) {
			upload.off( 'remove-upload', controller.removeUpload.bind( controller, upload ) );
		} );

		this.removeErrorUploads();

		uw.controller.Step.prototype.moveNext.call( this );
	};

	uw.controller.Details.prototype.movePrevious = function () {
		var controller = this;

		$.each( this.uploads, function ( i, upload ) {
			// get rid of remove handler, this handler only makes sense in this
			// exact step - having it bound while in other steps could cause
			// unexpected issues
			upload.off( 'remove-upload', controller.removeUpload.bind( controller, upload ) );
		} );

		uw.controller.Step.prototype.movePrevious.call( this );
	};

But this doesn't actually work, as the bind() calls return completely new function objects, not identical to the ones we used earlier in on(). The actual handler function would have to be stored somewhere and passed to off().

Note that as far as I can tell, this can't cause any problems in practice right now, except perhaps performance issues for very long-lived upload sessions (if the user goes through this step dozens of times, and the event handlers are added each time).

Event Timeline

matmarex created this task.Nov 16 2016, 6:13 PM
Restricted Application added a project: Multimedia. · View Herald TranscriptNov 16 2016, 6:13 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

I forgot to say why I'm filing a task and not just fixing it: I am wondering if we should fix these, or just remove them? As far as I can tell, everything works just fine with this code being dead. Am I missing something?

(Also, uploads can generally be removed only once, so 'remove-upload' is only fired once per upload, so ignore what I said about performance issues.)

Restricted Application added a subscriber: Matanya. · View Herald TranscriptNov 16 2016, 6:18 PM
MarkTraceur triaged this task as Low priority.Nov 16 2016, 6:25 PM
MarkTraceur moved this task from Untriaged to Next up on the Multimedia board.
MarkTraceur added a subscriber: MarkTraceur.

If the code is dead, maybe just remove it and if there are major changes to behaviour, we can catch it in review?

Change 322054 had a related patch set uploaded (by Matthias Mullie):
Fix remove-upload callback removal

https://gerrit.wikimedia.org/r/322054

The reason I wanted to unbind is that this particular callback is bound only in details step (it cleans up some things in that step, like getting rid of the "copy details" widget when there are no longer >1 uploads)
Now that we can go back & forth, we can reach this step more than once, and this callback would be bound & executed more than once when removing only 1 file.
All of the things that happen in this callback are safe enough that not unbinding it doesn't really cause a problem (apart from just piling on more and more callback), but I'd rather fix it :)
Fix is on gerrit - pretty trivial :)

matmarex closed this task as Resolved.Nov 17 2016, 3:24 PM
matmarex removed a project: Patch-For-Review.

Thanks!

Change 322054 merged by jenkins-bot:
Fix remove-upload callback removal

https://gerrit.wikimedia.org/r/322054