Brackets: leaks in src/command/Menus.js

[zaggino]

Menu.prototype.addMenuItem(CMD_ID) (1) calls constructor MenuItem(id, command) (2) which attaches event handlers to command (3). Calling Menu.prototype.removeMenuItem(CMD_ID) (4) doesn't remove these handlers leading to infinitely increasing numbers of event handlers on command.

1) https://github.com/adobe/brackets/blob/master/src/command/Menus.js#L546
2) https://github.com/adobe/brackets/blob/master/src/command/Menus.js#L586
3) https://github.com/adobe/brackets/blob/master/src/command/Menus.js#L295-L316
4) https://github.com/adobe/brackets/blob/master/src/command/Menus.js#L432-L466

One example of this would be https://github.com/zaggino/brackets-git/blob/master/src/Main.ts#L173-L203

[petetnt]

Nice catch and should be pretty trivial to fix. Any takers?

[daytonjallen]

I could have a go at this one. If I'm understanding the problem correctly all I'd need to do is fetch the menu item and call removeEventListener for each of the attached events on it right?

[zaggino]

@daytonallen go ahead
bit of tips: command comes from here: https://github.com/adobe/brackets/blob/master/src/command/CommandManager.js#L66-L73 so it's event methods are provided by EventDispatcher

you need to get a quick look at the code in https://github.com/adobe/brackets/blob/master/src/utils/EventDispatcher.js and it's off method that it assigns to objects

when calling off, you need to make sure you're providing also the second argument, pointer to the handler that was added here: https://github.com/adobe/brackets/blob/master/src/command/Menus.js#L295-L316

you can check contents of command._eventHandlers to see if you have correctly unsubscribed the event handler (number of handlers should decrease by 1 for each event)

[daytonjallen]

Submitted a PR.