624202 - Make sure to remove subscribers

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P1)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

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.

Comment 1

[(no longer active)]

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

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

(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?

Comment 3

[(no longer active)]

(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.

Comment 4

[(no longer active)]

Attachment: Patch (v1)

Comment 5

[Ian Gilman [:iangilman]]

(In reply to comment #1)
> The first two shouldn't cause leaks, should they?

Agreed; remove() takes care of it.

Comment 6

[Ian Gilman [:iangilman]]

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

Comment 7

[(no longer active)]

(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?

Comment 8

[Ian Gilman [:iangilman]]

(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.

Comment 9

[(no longer active)]

(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?  :-)

Comment 10

[Ian Gilman [:iangilman]]

(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.

Comment 11

[(no longer active)]

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?

Comment 12

[Ian Gilman [:iangilman]]

(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.

Comment 13

[(no longer active)]

Attachment: Patch (v2)

OK then!

Comment 14

[Ian Gilman [:iangilman]]

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

Voila

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9f8e3021db7f