TabView.uninit() leaks the TabView window

RESOLVED FIXED in Firefox 28

Status

RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 28

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Calling TabView.uninit() doesn't properly clean up and we're leaking the TabView window. This is not a big deal in practice as that function is never called explicitly and it's only the browser window that keeps TabView alive, so no leaks after a window is closed. TabView is alive as long as the window is.
Attachment #825830 - Flags: review?(gijskruitbosch+bugs)
Small addition.
Attachment #825830 - Attachment is obsolete: true
Attachment #825830 - Flags: review?(gijskruitbosch+bugs)
Attachment #825832 - Flags: review?(gijskruitbosch+bugs)

Comment 3

6 years ago
Comment on attachment 825830 [details] [diff] [review]
0003-Bug-933699-TabView.uninit-leaks-the-TabView-window.patch

Review of attachment 825830 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below. :-)

::: browser/components/tabview/tabitems.js
@@ +803,5 @@
>      for (let name in this._eventListeners) {
>        AllTabs.unregister(name, this._eventListeners[name]);
>      }
>      this.items.forEach(function(tabItem) {
> +      delete tabItem.tab._tabViewTabItem;

Can I suggest making this an arrow function and doing:

this.unlink(tabItem.tab);

instead? Seems like that was intended for exactly this purpose, and has a try catch with logging. In case either of us is missing something here.

::: browser/components/tabview/ui.js
@@ +227,5 @@
> +      };
> +
> +      gWindow.addEventListener("SSWindowClosing", onWindowClosing);
> +      this._cleanupFunctions.push(function () {
> +        gWindow.removeEventListener("SSWindowClosing", onWindowClosing);

Can probably leave this inside as well?
Attachment #825830 - Attachment is obsolete: false

Comment 4

6 years ago
Comment on attachment 825832 [details] [diff] [review]
0003-Bug-933699-TabView.uninit-leaks-the-TabView-window.patch

Review of attachment 825832 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the stuff in comment #3 addressed.
Attachment #825832 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 5

6 years ago
Comment on attachment 825830 [details] [diff] [review]
0003-Bug-933699-TabView.uninit-leaks-the-TabView-window.patch

Because instead of midairing me, bugzilla unobsoleted this and ugh.
Attachment #825830 - Attachment is obsolete: true
(In reply to :Gijs Kruitbosch from comment #3)
> >      this.items.forEach(function(tabItem) {
> > +      delete tabItem.tab._tabViewTabItem;
> 
> Can I suggest making this an arrow function and doing:
> 
> this.unlink(tabItem.tab);
> 
> instead? Seems like that was intended for exactly this purpose, and has a
> try catch with logging. In case either of us is missing something here.

Yeah, I thought about doing this as well. Will check if it leaks.

> > +      gWindow.addEventListener("SSWindowClosing", onWindowClosing);
> > +      this._cleanupFunctions.push(function () {
> > +        gWindow.removeEventListener("SSWindowClosing", onWindowClosing);
> 
> Can probably leave this inside as well?

If I call uninit() without closing the window the listener won't be removed and we have a leak. In fact only until the window closes but it's important for the leak checker to do that correctly.
(In reply to Tim Taubert [:ttaubert] from comment #6)
> > Can I suggest making this an arrow function and doing:
> > 
> > this.unlink(tabItem.tab);
> > 
> > instead? Seems like that was intended for exactly this purpose, and has a
> > try catch with logging. In case either of us is missing something here.
> 
> Yeah, I thought about doing this as well. Will check if it leaks.

I just remembered that we can't call unlink() here. Tabs would be removed from the storage if we do that and that would destroy tabs in their groups.
https://hg.mozilla.org/mozilla-central/rev/a2efbf205a5c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Can we request approval to land this on Aurora/Beta please? :)
I don't think it makes much sense to uplift this as it's not a leak that we're hitting with our current tests or even in the wild. Panorama lives as long as its parent window, so it's not really important whether the window keeps it alive.

With the new old leak detector however, we call TabView.uninit() explicitly before checking for leaks and that requires correct cleanup.
My hope was to get the leak detector uplifted eventually. Does that change your opinion about uplifting this?
Sure, if we uplift the leak detector then we also need to uplift all the leak fixes, including this one. I'm not opposing the uplift of this patch, all I was saying is that it currently doesn't have any effects :)
Target Milestone: --- → Firefox 28
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.