I have seen a large number of bugs where chrome script fails to clean up when the browser.xul window is closed. Often these scripts are registering for TabClose to clean up. I think there is some expectation that closing the window will fire TabClose for each tab in that window. This is not what happens, though. When the browser.xul window is closed we don't fire TabClose for each tab. Instead the code must explicitly listen for the "unload" event on the browser window. It would be nice to make browser.xul window close fire TabClose, but that would probably break a lot of code and addons that don't expect to get two notifications today. Instead, I think we should add a new "TabOrWindowClosed" that is fired in either case. New code can stat using that and we can migrate old code to use it over time. Thoughts?

Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 I tested all my daily-use extensions and 4 out of 24 are suffering from browser.xul leak: DownThemAll! *nightly*: https://bugs.downthemall.net/ticket/2985 Stylish: https://github.com/JasonBarnabe/stylish/issues/286/ NetVideoHunter: (No public issue tracker. I have sent a report via http://netvideohunter.com/contact) Menu Wizard: http://forums.mozillazine.org/viewtopic.php?p=14690963#p14690963 2 others extensions has fixed similar issues recently (that I know of): Greasemonkey: https://github.com/greasemonkey/greasemonkey/commit/32467a77bc2c4dfed86b6783186d44170f1260ec/ Tree Style Tabs: https://github.com/piroor/treestyletab/issues/886/ All of the above except DownThemAll are traditional extensions (non-jetpack/WebExtensions).

(In reply to Ben Kelly [:bkelly] from comment #0) > Instead, I think we should add a new "TabOrWindowClosed" that is fired in > either case. New code can stat using that and we can migrate old code to > use it over time. > > Thoughts? What does this cover that "unload" doesn't?

(In reply to :Gijs Kruitbosch from comment #2) > What does this cover that "unload" doesn't? Well, "unload" is not fired on every tab contained by a window, is it? So code has to listen for both tab close and window unload separately.

(In reply to Ben Kelly [:bkelly] from comment #3) > (In reply to :Gijs Kruitbosch from comment #2) > > What does this cover that "unload" doesn't? > > Well, "unload" is not fired on every tab contained by a window, is it? So > code has to listen for both tab close and window unload separately. I mean, if you're interested in "the entire window went away", listen for "unload". If you're interested in "a single tab was closed", listen to tabclose. If we don't fire the latter if the last tab in a window goes away that feels to me like just a bug in how we fire tabclose. Dão?

(In reply to Ben Kelly [:bkelly] from comment #3) > (In reply to :Gijs Kruitbosch from comment #2) > > What does this cover that "unload" doesn't? > > Well, "unload" is not fired on every tab contained by a window, is it? So > code has to listen for both tab close and window unload separately. The point I was trying to make is that if we're not changing tabclose then add-ons need to take action to make things work anyway, and adding a new redundant event doesn't seem like it'd be materially better for add-ons - they'd still need to make changes.

(In reply to :Gijs Kruitbosch from comment #5) > The point I was trying to make is that if we're not changing tabclose then > add-ons need to take action to make things work anyway, and adding a new > redundant event doesn't seem like it'd be materially better for add-ons - > they'd still need to make changes. We have an event TabClose that does not in fact fire for every case a tab is closed. If you close the tab by closing the window its not fired. This is terrible and we have plenty of evidence people don't understand it. Code should not have to care or be aware of *how* their window is closed. It seems to m we need to create a new event that works sanely and recommend new code use that. We should then deprecate TabClose. Open to other suggestions.

WebExtensions abstract this away anyway, don't they?

(In reply to Ben Kelly [:bkelly] from comment #6) > (In reply to :Gijs Kruitbosch from comment #5) > > The point I was trying to make is that if we're not changing tabclose then > > add-ons need to take action to make things work anyway, and adding a new > > redundant event doesn't seem like it'd be materially better for add-ons - > > they'd still need to make changes. > > We have an event TabClose that does not in fact fire for every case a tab is > closed. If you close the tab by closing the window its not fired. This is > terrible and we have plenty of evidence people don't understand it. Code > should not have to care or be aware of *how* their window is closed. > > It seems to m we need to create a new event that works sanely and recommend > new code use that. We should then deprecate TabClose. Open to other > suggestions. I have two: - change the meaning of tabclose - do nothing There are basically 3 categories of add-ons a) add-ons that already listen to both tabclose and unload b) add-ons that listen to only tabclose and should (also) listen to unload or (instead) to this new thing you're proposing c) add-ons that listen to only tabclose and are fine like that your change addresses (b), but you'd need to tell all the add-ons to update, too, in addition to us writing and maintaining that code. Changing the meaning of tabclose means some add-ons in (b) might not need to do anything, but we might break add-ons in (c) and/or (a) - I really don't know. I expect Dão will have a better idea. Doing nothing means only the add-ons in (b) need updating, just the same as if we add a new event. And of course, all of these add-ons are going to be deprecated sometime in 2017. So I would prefer to do nothing over adding a new event. If half the add-ons ever are leaking because of this, I would prefer to change tabclose in some way. But I don't see the point in adding a new event. Anyway, all this code is more to do with tabbrowser than core, so moving the bug.

(In reply to :Gijs Kruitbosch from comment #8) > Doing nothing means only the add-ons in (b) need updating, just the same as > if we add a new event. I think you're ignoring all the future chrome code that will be written that has to use this API. What are you going to do to make sure people get this right in the future? I think we have lots of evidence the current API is extremely error prone. Sticking with the status quo is just asking for more memory leaks in the future. Yes it would be nice to fix all the existing code, but we have to do work for that either way (as you noted). What I am asking for is a way to prevent creating more broken code in the future because the API is confusing. > And of course, all of these add-ons are going to be deprecated sometime in > 2017. This affects firefox chrome script as well. Not just add-ons. For example, stuff like bug 1276271.

(In reply to Ben Kelly [:bkelly] from comment #9) > (In reply to :Gijs Kruitbosch from comment #8) > > Doing nothing means only the add-ons in (b) need updating, just the same as > > if we add a new event. > > I think you're ignoring all the future chrome code that will be written that > has to use this API. What are you going to do to make sure people get this > right in the future? We should probably update MDN to clarify when we don't fire the event. If this is very widespread among add-ons (is there evidence of that? Lots of add-ons use TabClose listeners, but are they all leaking the world all the time? That seems unlikely...) then we could do a hacks or add-ons blogpost. > I think we have lots of evidence the current API is > extremely error prone. Can you elaborate? Just the bug you referenced? Other bugs? > Sticking with the status quo is just asking for more memory leaks in the > future. > > Yes it would be nice to fix all the existing code, but we have to do work > for that either way (as you noted). What I am asking for is a way to > prevent creating more broken code in the future because the API is confusing. I personally do not believe that adding a second API would prevent these issues. There is already a second API people could use, and don't. Why would someone who uses "TabClose" today use "TabOrWindowClosed" instead? The event should "do what it says on the tin". It's just that there's 2 ways to read what the tin says. :-) Especially considering how many people in the add-ons community treat our codebase as a black box that they have to negotiate with the same way as with closed-source software, and how many people copy-paste snippets from forums, IRC, stackoverflow, etc., trying to evangelize people doing something differently from how it's been done for 10+ years is a difficult problem. So if we do anything I'd prefer to outright change the existing event to also fire when the window goes away. comment #0 suggests that would break existing code. I don't really understand why it talks about "two notifications" - you mean for both TabClose and "unload", or am I missing something ? The unload one won't refer to a specific tab, so they would be distinguishable. On the other hand, some code really specifically means "tab close" not "tab close as a result of a window closing", which it treats differently (e.g. session restore code), and such code would break if we changed the meaning of the TabClose event. > > And of course, all of these add-ons are going to be deprecated sometime in > > 2017. > > This affects firefox chrome script as well. Not just add-ons. For example, > stuff like bug 1276271. Again, I don't think adding an API will prevent misuse of an existing API. AFAICT none of the code under browser/ has issues like you describe. I can't speak for devtools, it's effectively a separate project.

(In reply to :Gijs Kruitbosch from comment #10) > > I think we have lots of evidence the current API is > > extremely error prone. > > Can you elaborate? Just the bug you referenced? Other bugs? Bug 1291709 is the other recent one that comes to mind. You can see more discussion in the github thread: https://github.com/mozilla/pdf.js/pull/7521#issuecomment-237903003 > > Sticking with the status quo is just asking for more memory leaks in the > > future. > > > > Yes it would be nice to fix all the existing code, but we have to do work > > for that either way (as you noted). What I am asking for is a way to > > prevent creating more broken code in the future because the API is confusing. > > I personally do not believe that adding a second API would prevent these > issues. There is already a second API people could use, and don't. Why would > someone who uses "TabClose" today use "TabOrWindowClosed" instead? The event > should "do what it says on the tin". It's just that there's 2 ways to read > what the tin says. :-) I guess you guys can do what you want, but I think its bad API design. Instead of a single, simple "your closing, clean up" hook we require all tab oriented code to register for two hooks instead.

(In reply to Ben Kelly [:bkelly] from comment #11) > I guess you guys can do what you want, but I think its bad API design. I don't think anybody thinks the current situation is optimal or "awesome API design". > Instead of a single, simple "your closing, clean up" hook we require all tab > oriented code to register for two hooks instead. Not everyone requires both hooks. Having several events here with clearer naming is now difficult to accomplish (because if we keep the existing "TabClose" then that will stay ambiguous, and we can't get rid of it without breaking lots of consumers - they're events, so it's not even like we can shim things or apply magic and do different things for different consumers). There are no easy fixes, AFAICT. Dão/Mike, got opinions about what to do here?

We can change semantics once XUL extensions have been phased out. Whether we should do that, I'm not sure. It would likely be handy for consumers living outside of the browser window, not so much for those living inside the window.

Just to clarify, once XUL extensions are phased out we might be able to make the window `unload` close all of its tabs explicitly? So each tab would get a graceful close and a `TabClose` event in these cases? That would be ideal.

No, we wouldn't actually close the tabs one by one, just fire the events before closing the window. Thinking about it though, that might confuse consumers that keep track of the number of tabs and stuff like that, so we might still need a separate event for that.

Can somebody lay out the case for not firing the TabClose event for each tab (without actually removing the nodes) in a window when the window is closed? That seems like it'd be the most sensible thing to do in the long run, imo. Is there a known case where doing this will break an extension?

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #16) > Can somebody lay out the case for not firing the TabClose event for each tab > (without actually removing the nodes) in a window when the window is closed? > That seems like it'd be the most sensible thing to do in the long run, imo. > Is there a known case where doing this will break an extension? I expect it will break session restore, to name one. "restore closed window" would restore empty windows if we removed all the tabs from them before they closed. I expect there'll be other (tab or session management) add-ons with similar problems.