We have, by my count, three places where we're adding subscribers and they don't remove themselves after being fired: groupitems.js: 681 child.addSubscriber(self, "close", function() { 682 self.remove(child); 683 }); 900 item.addSubscriber(this, "close", function() { 901 self.remove(item); 902 }); ui.js: 369 this._activeTab.addSubscriber(this, "close", function() { 370 self._activeTab = null; 371 }); In addition, I believe I've seen (on my leak hunt) other subscribers which, if they don't get fired, don't actually get removed... but that'll have to be a more systematic search and can be another bug. For starters, let's patch these three potential leaks.

The first two shouldn't cause leaks, should they?

(In reply to comment #1) > The first two shouldn't cause leaks, should they? I'm not entirely sure. Ehsan, can we assign this to you?

(In reply to comment #2) > (In reply to comment #1) > > The first two shouldn't cause leaks, should they? > > I'm not entirely sure. Ehsan, can we assign this to you? Please do.

(In reply to comment #1) > The first two shouldn't cause leaks, should they? Agreed; remove() takes care of it.

Comment on attachment 508167 [details] [diff] [review] Patch (v1) I believe it should be: closedTabItem.removeSubscriber(self, "close"); … just in case. R+ with that change

(In reply to comment #6) > Comment on attachment 508167 [details] [diff] [review] > Patch (v1) > > I believe it should be: > > closedTabItem.removeSubscriber(self, "close"); > > … just in case. > > R+ with that change Hmm, why is that? closedTabItem could be different to self._activeTab, right?

(In reply to comment #7) > > I believe it should be: > > > > closedTabItem.removeSubscriber(self, "close"); > > > > … just in case. > > > > R+ with that change > > Hmm, why is that? closedTabItem could be different to self._activeTab, right? Correct. closedTabItem will always be the object sending the message, and that's who you want to be unsubscribing from.

(In reply to comment #8) > (In reply to comment #7) > > > I believe it should be: > > > > > > closedTabItem.removeSubscriber(self, "close"); > > > > > > … just in case. > > > > > > R+ with that change > > > > Hmm, why is that? closedTabItem could be different to self._activeTab, right? > > Correct. closedTabItem will always be the object sending the message, and > that's who you want to be unsubscribing from. If that's the case, why do we need to explicitly check it on the next line? :-)

(In reply to comment #9) > > > Hmm, why is that? closedTabItem could be different to self._activeTab, right? > > > > Correct. closedTabItem will always be the object sending the message, and > > that's who you want to be unsubscribing from. > > If that's the case, why do we need to explicitly check it on the next line? > :-) Because closedTabItem could be different to self._activeTab. I feel like we're going around in circles. The idea is that there's only one _activeTab, and every time you change it, you unsubscribe from the old one and subscribe to the new one, so I suppose theoretically we don't need these checks at all. We have them because I feel it's good defensive programming. So, the scenario that we're defending against is that we've changed _activeTab but didn't unsubscribe from the old one before its "close" fires. In this case, you want to make sure to unsubscribe closedTabItem (because it's the object sending the event... unsubscribing the new _activeTab would be wrong), and you only want to set _activeTab to null if the object getting closed is in fact _activeTab. Even if we decide that this scenario could never occur, it's still more correct to unsubscribe closedTabItem, because it's the actual thing sending the event, and the only reason we would be getting the event is because we had subscribed to it. Does that make sense? If not, let's discuss in IRC.

OK, reading the description in comment 10, your comment makes perfect sense. Sorry, I didn't fully understand what's happening here. So, here's another question: would it make sense for us to take another patch, namely one which replaces the |self._activeTab = null;| with a |self.setActiveTab(null);| call? This way, removeSubscriber is taken care of, and MakeDeactive also gets called. Does that make more sense?

(In reply to comment #11) > So, here's another question: would it make sense for us to take another patch, > namely one which replaces the |self._activeTab = null;| with a > |self.setActiveTab(null);| call? This way, removeSubscriber is taken care of, > and MakeDeactive also gets called. Does that make more sense? That would certainly work, too. makeDeactive isn't really necessary in this case, as the tab is closing, but it doesn't hurt. It's probably better design to call self.setActiveTab(null) so we're consolidating code paths.

OK then!

Comment on attachment 509638 [details] [diff] [review] Patch (v2) Voila