680686 - Remove AllTabs.jsm until the all-windows feature gets properly implemented

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

[browser/components/sessionstore/test/browser/browser_354894.js]
  13x [about:blank]
  12x [chrome://browser/content/browser.xul]

I tried to fix AllTabs.jsm to not leak DOMWindows but wasn't successful. AllTabs was created with multi-windows support in mind which Panorama isn't ready for. In fact it's going to need some time for that. So I'll suggest removing AllTabs.jsm for now and replace it with a per-window solution. Running the test from above doesn't leak anymore with the patch applied.

This leak is very likely responsible for some (if not all) of the leaks introduced with bug 626455 and bug 662812.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 554646 [details] [diff] [review]
patch v1

>+  handleEvent: function AllTabs_handleEvent(event) {
>+    let tab = event.target;
>+  },

Dead code?

>+  register: function AllTabs_register(eventName, callback) {
>+    function handleEvent(event) {
>+      callback(event.target, event);
>+    }
>+
>+    this._callbacks.set(callback, handleEvent);
>+    gBrowser.tabContainer.addEventListener(this._events[eventName], handleEvent, false);
>+  },
>+
>+  unregister: function AllTabs_unregister(eventName, callback) {
>+    if (!this._callbacks.has(callback))
>+      return;
>+
>+    let handleEvent = this._callbacks.get(callback);
>+    this._callbacks.delete(callback);
>+    gBrowser.tabContainer.removeEventListener(this._events[eventName], handleEvent, false);
>+  }

What happens when calling register twice with the same callback?

Comment 3

[Dão Gottwald [:dao]]

It really would be interesting to know what AllTabs.jsm is doing wrong. It looks like it shouldn't leak...

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(In reply to Dão Gottwald [:dao] from comment #2)
> Dead code?

Oops, indeed. Removed.

> What happens when calling register twice with the same callback?

Checking now if the callback is already registered and returning if so.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #3)
> It really would be interesting to know what AllTabs.jsm is doing wrong. It
> looks like it shouldn't leak...

Yeah, I tried lots of different things and made sure that unregisterBrowserWindow() and similar functions are called. I even added a domwindowclosed observer but that didn't help...

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 554648 [details] [diff] [review]
patch v2

>+  register: function AllTabs_register(eventName, callback) {
>+    if (this._callbacks.has(callback))
>+      return;

This means that you can't use one function to react to two events...
Can we just make register/unregister call add/removeEventListener directly and let the callbacks take an event instead of a tab?

Can we also remove the tab.ownerDocument.defaultView != gWindow checks?
Could the reason for the leaks be that groupitems.js currently does *not* have this check?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #6)
> This means that you can't use one function to react to two events...
> Can we just make register/unregister call add/removeEventListener directly
> and let the callbacks take an event instead of a tab?

I tried to be minimal invasive but I think we should do that. That's much better than fiddling around with those callbacks.

> Can we also remove the tab.ownerDocument.defaultView != gWindow checks?
> Could the reason for the leaks be that groupitems.js currently does *not*
> have this check?

Interesting discovery... let me try that!

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #7)
> > Can we also remove the tab.ownerDocument.defaultView != gWindow checks?
> > Could the reason for the leaks be that groupitems.js currently does *not*
> > have this check?
> 
> Interesting discovery... let me try that!

Bingo, the leak is gone. Great catch, thanks!

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

Comment 10

[Dão Gottwald [:dao]]

I think we should probably still get rid of AllTabs.jsm. It's showing that it's overengineered. There's only a handful of consumers, so we shouldn't be afraid of being invasive.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 554657 [details] [diff] [review]
patch v4

>-      AllTabs.tabs.forEach(function(tab) {
>-        if (tab.ownerDocument.defaultView != gWindow)
>-          return;
>-
>+      Array.forEach(gBrowser.tabs, function (tab) {
>         self.saveTab(tab, null);
>       });

This is a behavior change, since AllTabs.tabs filtered closing tabs, right? I wouldn't have minded maintaining AllTabs.tabs, AllTabs.register and AllTabs.unregister as thin wrappers like in your previous patches...

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #12)
> This is a behavior change, since AllTabs.tabs filtered closing tabs, right?
> I wouldn't have minded maintaining AllTabs.tabs, AllTabs.register and
> AllTabs.unregister as thin wrappers like in your previous patches...

Oh! We shouldn't touch closing tabs, good point. Ok... the patch is still there :)

Comment 14

[Dão Gottwald [:dao]]

For patch v2 I'd still like a tab -> event signature change for the callbacks.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2b

(In reply to Dão Gottwald [:dao] from comment #14)
> For patch v2 I'd still like a tab -> event signature change for the
> callbacks.

Done.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2c

Fixed browser_tabview_alltabs.js.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 554675 [details] [diff] [review]
patch v2c

>--- a/browser/base/content/tabview/tabitems.js
>+++ b/browser/base/content/tabview/tabitems.js

>-    this._eventListeners["open"] = function(tab) {
>-      if (tab.ownerDocument.defaultView != gWindow || tab.pinned)
>-        return;
>+    this._eventListeners["open"] = function (event) {
>+      let tab = event.target;

nit: this._eventListeners.open

>-    this._eventListeners["attrModified"] = function(tab) {
>-      if (tab.ownerDocument.defaultView != gWindow || tab.pinned)
>-        return;
>+    this._eventListeners["attrModified"] = function (event) {
>+      let tab = event.target;

ditto

>--- a/browser/base/content/test/tabview/browser_tabview_alltabs.js
>+++ b/browser/base/content/test/tabview/browser_tabview_alltabs.js

> Cu.import("resource:///modules/tabview/AllTabs.jsm");

still need to remove this

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/fx-team/rev/399aba688a19

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/399aba688a19