Make sure to remove subscribers

RESOLVED FIXED in Firefox 4.0b12

Status

Firefox Graveyard
Panorama
P1
major
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: mitcho, Assigned: Ehsan)

Tracking

({memory-leak})

unspecified
Firefox 4.0b12
memory-leak

Details

(Whiteboard: [qa-][cleanup][potential leak])

Attachments

(1 attachment, 1 obsolete attachment)

922 bytes, patch
iangilman
: review+
Dolske
: approval2.0+
Details | Diff | Splinter Review
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.
(Reporter)

Updated

8 years ago
Severity: normal → major
Priority: -- → P1
Whiteboard: [qa-][cleanup]
(Assignee)

Comment 1

8 years ago
The first two shouldn't cause leaks, should they?
(Reporter)

Comment 2

8 years ago
(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?
(Assignee)

Comment 3

8 years ago
(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.
(Reporter)

Updated

8 years ago
Assignee: nobody → ehsan
Whiteboard: [qa-][cleanup] → [qa-][cleanup][potential leak]
Blocks: 627096
(Assignee)

Updated

7 years ago
Keywords: mlk
(Assignee)

Comment 4

7 years ago
Created attachment 508167 [details] [diff] [review]
Patch (v1)
Attachment #508167 - Flags: review?(ian)
(Assignee)

Updated

7 years ago
Attachment #508167 - Attachment is patch: true
Attachment #508167 - Attachment mime type: application/octet-stream → text/plain
(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
Attachment #508167 - Flags: review?(ian) → review+
(Assignee)

Comment 7

7 years ago
(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.
(Assignee)

Comment 9

7 years ago
(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.
(Assignee)

Comment 11

7 years ago
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.
(Assignee)

Comment 13

7 years ago
Created attachment 509638 [details] [diff] [review]
Patch (v2)

OK then!
Attachment #508167 - Attachment is obsolete: true
Attachment #509638 - Flags: review?(ian)
Comment on attachment 509638 [details] [diff] [review]
Patch (v2)

Voila
Attachment #509638 - Flags: review?(ian) → review+
(Assignee)

Updated

7 years ago
Attachment #509638 - Flags: approval2.0?
Attachment #509638 - Flags: approval2.0? → approval2.0+
(Assignee)

Updated

7 years ago
Whiteboard: [qa-][cleanup][potential leak] → [qa-][cleanup][potential leak][needs landing]
(Assignee)

Comment 15

7 years ago
http://hg.mozilla.org/mozilla-central/rev/9f8e3021db7f
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [qa-][cleanup][potential leak][needs landing] → [qa-][cleanup][potential leak]
Target Milestone: --- → Firefox 4.0b12
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.