Last Comment Bug 680686 - Remove AllTabs.jsm until the all-windows feature gets properly implemented
: Remove AllTabs.jsm until the all-windows feature gets properly implemented
Status: RESOLVED FIXED
: mlk
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 9
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on:
Blocks: 626455 bc-leaks 662812
  Show dependency treegraph
 
Reported: 2011-08-20 10:37 PDT by Tim Taubert [:ttaubert]
Modified: 2016-04-12 14:00 PDT (History)
1 user (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (8.66 KB, patch)
2011-08-20 10:40 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v2 (8.63 KB, patch)
2011-08-20 11:16 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v3 (1.02 KB, patch)
2011-08-20 11:46 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v4 (19.48 KB, patch)
2011-08-20 12:42 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v2b (16.61 KB, patch)
2011-08-20 13:27 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v2c (18.26 KB, patch)
2011-08-20 16:09 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Review

Description Tim Taubert [:ttaubert] 2011-08-20 10:37:42 PDT
[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] 2011-08-20 10:40:04 PDT
Created attachment 554646 [details] [diff] [review]
patch v1
Comment 2 Dão Gottwald [:dao] 2011-08-20 11:00:43 PDT
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] 2011-08-20 11:04:25 PDT
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] 2011-08-20 11:16:45 PDT
Created attachment 554648 [details] [diff] [review]
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] 2011-08-20 11:17:54 PDT
(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] 2011-08-20 11:29:37 PDT
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] 2011-08-20 11:33:03 PDT
(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] 2011-08-20 11:45:02 PDT
(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] 2011-08-20 11:46:16 PDT
Created attachment 554654 [details] [diff] [review]
patch v3
Comment 10 Dão Gottwald [:dao] 2011-08-20 11:52:55 PDT
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] 2011-08-20 12:42:15 PDT
Created attachment 554657 [details] [diff] [review]
patch v4
Comment 12 Dão Gottwald [:dao] 2011-08-20 12:49:12 PDT
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] 2011-08-20 12:51:20 PDT
(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] 2011-08-20 12:56:38 PDT
For patch v2 I'd still like a tab -> event signature change for the callbacks.
Comment 15 Tim Taubert [:ttaubert] 2011-08-20 13:27:34 PDT
Created attachment 554663 [details] [diff] [review]
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] 2011-08-20 16:09:06 PDT
Created attachment 554675 [details] [diff] [review]
patch v2c

Fixed browser_tabview_alltabs.js.
Comment 17 Dão Gottwald [:dao] 2011-08-21 00:16:15 PDT
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] 2011-08-21 01:43:05 PDT
http://hg.mozilla.org/integration/fx-team/rev/399aba688a19
Comment 19 Tim Taubert [:ttaubert] 2011-08-21 23:53:09 PDT
http://hg.mozilla.org/mozilla-central/rev/399aba688a19

Note You need to log in before you can comment on or make changes to this bug.