933699 - TabView.uninit() leaks the TabView window

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-933699-TabView.uninit-leaks-the-TabView-window.patch

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-933699-TabView.uninit-leaks-the-TabView-window.patch

Small addition.

Comment 3

[:Gijs (out for now; he/him)]

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?

Comment 4

[:Gijs (out for now; he/him)]

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.

Comment 5

[:Gijs (out for now; he/him)]

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.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/a2efbf205a5c

Comment 9

[Jim Mathies [:jimm]]

https://hg.mozilla.org/mozilla-central/rev/a2efbf205a5c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Can we request approval to land this on Aurora/Beta please? :)

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

My hope was to get the leak detector uplifted eventually. Does that change your opinion about uplifting this?

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

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