User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6pre) Gecko/20100911 Firefox/4.0b6pre Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6pre) Gecko/20100911 Firefox/4.0b6pre A problem with current TabCandy implementation is that it loads all tabs -- including those in inactive groups -- into memory, on browser startup. This, to an extent, defeats a perceived benefit of TabCandy and creates a problematic disconnect between what a user may expect and what is actually happening. I ran into this issue when I noticed that Firefox was consuming over 400 MB of RAM, but I had only a handful of relatively 'light' tabs loaded. I thought something was leaking, but when I was able to reproduce the issue in safe-mode, with a single tab, I finally opened "Group Your Tabs" and, lo and behold, I had hidden tab groups that Firefox was all-to-happy to load into memory -- even though they were not being used. This is quite problematic, because some users, as happened with me, will be under the impression that they can create a whole bunch of tab groups -- that will be accessible to them sometime later when they want -- and not have to clutter and bog down the browser with many tabs, not realizing that the latter will continue to occur. It would be nice if the default behavior is to load only the tabs in the active group(s), initially, and to have an option so that those who want to always load the whole mess can so choose. Reproducible: Always

please search before you file a bug

Not even close to a dupe. Please carefully before duping.

(In reply to comment #2) > Not even close to a dupe. Please carefully before duping. Meant to say: please read carefully before duping.

I didn't state it explicitly, but the implication is that those hidden tabs SHOULD NOT BE LOADED AT ALL -- until such time as they first become active (i.e. by being part of an active group).

Note that we need to make sure Panorama's thumbnail cache is nice and solid for this, so you can still see the thumbnails for the tabs that haven't been loaded.

One of our core strategic initiatives was to decrease startup time and increase real and perceived speed. Panorama allows you to handle many more tabs that was previously sane, but this also hurts us on restoring the session startup experience. The cascaded loading of bug 586068 helps a lot, but we can go further and get truly zippy'ness by not loaded tabs from hidden groups until we need to. In zpao's words "It *should* actually be a tremendously trivial change after bug 586068". I think of this as the ultimate in polish and would like to mark this blocking for beta n, where n is when our dependency on the cascading loading is done.

(In reply to comment #5) > Note that we need to make sure Panorama's thumbnail cache is nice and solid for > this, so you can still see the thumbnails for the tabs that haven't been > loaded. Filed bug 597248 for this.

We'll take this if it's ready in time, wouldn't hold release on it.

Load Tabs Progressively (https://addons.mozilla.org/en-US/firefox/addon/91919/) behaves like the request.

(In reply to comment #9) > Load Tabs Progressively (https://addons.mozilla.org/en-US/firefox/addon/91919/) > behaves like the request. Panorama is out-of-the-box functionality. Only the geekiest of users will connect the dots, understand the problem, and have the inkling to search for an extension that may possibly alleviate the issue.

(In reply to comment #6) > One of our core strategic initiatives was to decrease startup time and increase > real and perceived speed. This isn't just about speed. Like the reporter mentioned it can significantly affect the memory footprint of firefox. Assuming some usage pattern where some groups are used more like bookmarks/"will read this later" or people simply being lazy about closing tabs it will become more common that a large amount of tabs is open. To avoid the impression of bloat those tabs shouldn't take up memory (which can easily range in the gigabytes, as it does for me).

Moving to b9

bugspam (moving b9 to b10)

bugspam (removing b9)

Moving goalpost to beta 11. Paul, do you think you'll get to this at all for fx4? We most likely will punt on this...

Not a blocker, so it's not likely I'll get to work on this (though I'd like to)

(In reply to comment #16) > Not a blocker, so it's not likely I'll get to work on this (though I'd like to) Feel free to unassign yourself if you'd like someone else to take a look at this.

just to note, setting browser.sessionstore.max_concurrent_tabs to 0 does almost what is requested in this bug, except that it also affects the current group instead of just the background ones.

Of course I desperately want this, but it's way too late for Fx4

This introduces a new pref, browser.sessionstore.restore_hidden_tabs, with default true. Test included. Pushed to try.

(In reply to comment #20) > Created attachment 506136 [details] [diff] [review] > Patch v1 > > This introduces a new pref, browser.sessionstore.restore_hidden_tabs, with > default true. Test included. Pushed to try. Works very well! Thanks, Michael. Tested with three tab groups, and the patch does exactly what this bug requested: Loads only the tabs from the active group. Used the following try server build: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mmitcho@mozilla.com-d2c31ca776bd/try-w32/ I didn't encounter any problems, but others will have to double-check.

Passed try.

Comment on attachment 506136 [details] [diff] [review] Patch v1 I'm glad this was as easy as I'd hoped it would be :) >+// Whether to automatically restore hidden tabs (i.e., tabs in other tab groups) or not >+pref("browser.sessionstore.restore_hidden_tabs", true); That pref name could be bit confusing, but it's a hidden pref so oh well. If we can get this in for beta 11, it might be good to just set this to false and see how it goes. > // Look in visible, then hidden > let nextTabArray; > if (this._tabsToRestore.visible.length) { > nextTabArray = this._tabsToRestore.visible; > } >- else if (this._tabsToRestore.hidden.length) { >+ else if (this._restoreHiddenTabs && this._tabsToRestore.hidden.length) { > nextTabArray = this._tabsToRestore.hidden; > } This made me realize that returning from PB mode could get a bit weird. Tabs that were loaded would need to be loaded again if they weren't in the active group. Not a big deal though. >+ * The Initial Developer of the Original Code is >+ * Mozilla Foundation. >+ * Portions created by the Initial Developer are Copyright (C) 2010 It's 2011 now! >+function test_loadTabs(expectHiddenTabs, callback) { >+ // Set the restore_hidden_tabs pref. >+ Services.prefs.setBoolPref("browser.sessionstore.restore_hidden_tabs", expectHiddenTabs); >+ >+ let state = { windows: [{ tabs: [ >+ { entries: [{ url: "http://example.com#1" }], extData: { "uniq": r() } }, >+ { entries: [{ url: "http://example.com#2" }], extData: { "uniq": r() }, hidden: true }, >+ { entries: [{ url: "http://example.com#3" }], extData: { "uniq": r() } }, >+ { entries: [{ url: "http://example.com#4" }], extData: { "uniq": r() }, hidden: true } >+ ] }] }; You're not actually using any extData checks, so you can get rid of assigning uniq & the definition of r() (which is defined in head.js )or future reference) >+ >+ let expectedTabs = expectHiddenTabs ? 4 : 2; >+ >+ let check = function(unrestored) { >+ gBrowser.tabContainer.removeEventListener("SSTabRestored", onSSTabRestored, true); >+ >+ let testWindow = Services.wm.getEnumerator("navigator:browser").getNext(); You're not using testWindow. >+ is(gBrowser.visibleTabs.length, 2, "only 2 visible tabs"); >+ let tabs = gBrowser.tabs; >+ ok(!tabs[0].hidden, "first is visible"); >+ ok(tabs[1].hidden, "second tab is hidden"); >+ ok(!tabs[2].hidden, "third tab is now visible"); >+ ok(tabs[3].hidden, "third tab is now hidden"); >+ >+ is(restoredTabs, expectedTabs, expectHiddenTabs ? >+ "We restored all four tabs, including the hidden" : >+ "We restored only the two visible tabs" >+ ); >+ >+ is(unrestored, 4 - expectedTabs, expectHiddenTabs ? >+ "There are no unrestored tabs left" : >+ "Two unrestored tabs remain" >+ ); This is a bit gross. Instead of trying to force the ternary in there, I would make it cleaner and just use if/else & repeat the is >+ >+ callback(); >+ }; >+ >+ let restoredTabs = 0; >+ function onSSTabRestored(aEvent) { >+ if (++restoredTabs == expectedTabs) >+ check(needsRestore()); >+ } >+ gBrowser.tabContainer.addEventListener("SSTabRestored", onSSTabRestored, true); >+ >+ ss.setBrowserState(JSON.stringify(state)); >+} You have a few lines that are just spaces, please trim those. I'd really like to see a test with more than max_concurrent_tabs # of tabs that are hidden. Then simulate switching groups by hiding & showing and ensure that all of the tabs load. Ideally all of the visible tabs would finish loading before you switched visiblity. I'd want to check that we actually get max_concurrent_tabs restoring after the change (might be a bit difficult to actually check, but I think it's important to do so). We should be fine with the new pref == true, but false is where I'm concerned. r- until that test exists & is passing. I'm not sure it will pass without more changes to sessionstore itself.

To be clearer, what I expect is going to happen is that we'll restore 1 tab (since we should get TabSelect which should call restoreTab directly) and then we'll only restore 1 at a time after that. So we might need to add a call to restoreNextTab in onTabShow(). It would cause restoreNextTab to get called excessively but it was designed to handle that.

(In reply to comment #23) > If we can get this in for beta 11, it might be good to just set this to false > and see how it goes. I would advise against that. Setting browser.sessionstore.max_concurrent_tabs = 0 currently breaks switch to tab behavior (see bug 599909), so i assume browser.sessionstore.restore_hidden_tabs = false will do the same.

(In reply to comment #25) > I would advise against that. Setting browser.sessionstore.max_concurrent_tabs = > 0 currently breaks switch to tab behavior (see bug 599909), so i assume > browser.sessionstore.restore_hidden_tabs = false will do the same. Why would you assume that? They don't do the same thing. Besides, it's a beta and the point is to enable it to see if causes any problems. If it doesn't get proven in a beta, when do you expect it to be?

(In reply to comment #25) > (In reply to comment #23) > > If we can get this in for beta 11, it might be good to just set this to false > > and see how it goes. > > I would advise against that. Setting browser.sessionstore.max_concurrent_tabs = > 0 currently breaks switch to tab behavior (see bug 599909), so i assume > browser.sessionstore.restore_hidden_tabs = false will do the same. It would do the same. And yes, I know about that brokenness. I filed & wrote the patch that will fix it :) (In reply to comment #26) > They don't do the same thing. They actually end up doing the exact same thing.

Updated test based on your comments. The restore of all hidden tabs when they become unhidden actually seems to be working. I edited onTabShow first, as you suggested, but when I removed that later to check, it actually passed that part. I'm using your countTabs function, now, and check the isRestoring value after every SSTabRestored. I'm not sure if it's because of this timing, but there's one test failure for me, at least locally. It's in the first test (where we start restoring all four tabs). The first isRestoring check fails, as it says there are two tabs being restored at that point... max_concurrent_tabs of course is set to 1. Am I misunderstanding something about this metric? I'll post log output as well.

(In reply to comment #28) > Created attachment 506661 [details] [diff] [review] > Patch v2 > > Updated test based on your comments. > > The restore of all hidden tabs when they become unhidden actually seems to be > working. I edited onTabShow first, as you suggested, but when I removed that > later to check, it actually passed that part. Sorry I should have been clearer. We need a test for what would be the default: max_concurrent_tabs == 3 (or really anything greater than 1). When the max is 1, that's not going to actually test the potential problem I tried to describe above. With max_concurrent_tabs == 1, I would expect your test to always pass. > I'm using your countTabs function, now, and check the isRestoring value after > every SSTabRestored. I'm not sure if it's because of this timing, but there's > one test failure for me, at least locally. It's in the first test (where we > start restoring all four tabs). The first isRestoring check fails, as it says > there are two tabs being restored at that point... Probably a timing issue. countTabs was setup to count correctly based on the order the tabsProgressListener was added in relation to when sessionstore's was added and the order they would get notified of events. You might need to do what I did in browser_586068-cascaded_restore.js with the progress listeners to get a more accurate count.

Comment on attachment 506661 [details] [diff] [review] Patch v2 r- based on comment 30

Based on our limited time and capacity for fx4, as well as the slight risk that this patch poses, we're going to punt on this for fx4.

I'm moving on from Panorama work now, so I'm going to leave this bug open now. The core of the patch is working, though, so if someone can take this the extra mile based on zpao's comments, that would be great and a welcome addition to fx5!

bugspam

Passed try: http://dev.philringnalda.com/tbpl/?tree=Try&pusher=tim.taubert@gmx.de&rev=72ca7ab9cca0 Explanation for changes to browser_tabview_bug595560.js: This test (right before the actual test belonging to this bug) created a group but didn't delete it afterwards. So that was the permanent Win XP opt only orange.

Comment on attachment 527960 [details] [diff] [review] patch v3 I suck at reviewing in a reasonable amount of time... I'm sorry. But I'll be much more on top of this from here out. Now correct me if I'm wrong (please, I would love to be wrong here), but doesn't this suffer from the same problem that I pointed out when reviewing Raymond's patch? I didn't see anything explicitly checking that we were restoring 3 tabs after switching groups, only < 4. The changes to nsSessionStore don't include a call to restoreNextTab in onTabShow (which adds to my suspicions). So again, here's the situation I'm concerned about: * 4 visible tabs, 4 hidden tabs (you have that) * we get to < max_concurrent_tabs left restoring/need restore. I think even 0 is going to break. * we switch groups * we're not restoring max_concurrent_tabs at a time because we don't call restoreNextTab enough. Am I crazy or is that still not properly tested?

(In reply to comment #23) > >+// Whether to automatically restore hidden tabs (i.e., tabs in other tab groups) or not > >+pref("browser.sessionstore.restore_hidden_tabs", true); > > That pref name could be bit confusing, but it's a hidden pref so oh well. I second this. Maybe it would be better to trim the "_tabs"?

(In reply to comment #36) > So again, here's the situation I'm concerned about: > * 4 visible tabs, 4 hidden tabs (you have that) > * we get to < max_concurrent_tabs left restoring/need restore. I think even > 0 is going to break. > * we switch groups > * we're not restoring max_concurrent_tabs at a time because we don't call > restoreNextTab enough. > > Am I crazy or is that still not properly tested? Oh snap, thanks for catching that. When I split the test case into two tests I forgot to include it in the tabview test. It of course does fail (and now it totally makes sense to me, too) without the restoreNextTab() call. I removed it while writing the patch because the test passed without it (surprise :). So browser_tabview_bug595601.js does now check if three tabs are restored concurrently. A restoreNextTab() call was added to onTabShow(aTab) when <aTab> switches visibility.

Comment on attachment 531667 [details] [diff] [review] patch v4 Good to know I wasn't crazy :) Looks good to me now. >diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js >+// Whether to automatically restore hidden tabs (i.e., tabs in other tab groups) or not >+pref("browser.sessionstore.restore_hidden_tabs", true); Let's just go ahead and set this to false and get it landed. We can always set it back to true later if we have to.

Just in case anybody wanted to jump the gun and land this, don't. Tim pointed out there's orange on his try push with this patch. I'm inclined to say it's the test, but it needs to be looked at more (and running solo passes). I'll take a closer look tomorrow. http://tbpl.mozilla.org/?tree=Try&rev=5a372382dd16

Finally passes try (yay!). I had to modify some tests and some head.js functions to deal with hidden tabs not getting restored by default. I modified some tests to execute with restore_hidden_tabs on and off. Therefore asking for review again. http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=134834b69989

Oh and there was some legacy lines of code that counted restored tabs as currently restoring. That seemed to happen only on try and that was the error we saw (4 tabs not 3 tabs restoring).

Comment on attachment 532931 [details] [diff] [review] patch v5 Review of attachment 532931 [details] [diff] [review]: -----------------------------------------------------------------

(In reply to comment #44) > patch for checkin (push with bug 636279 and bug 650573) (volkmar pushed those other bugs' patches to cedar already) pushed to cedar: http://hg.mozilla.org/projects/cedar/rev/75fc2600eea8

Pushed: http://hg.mozilla.org/mozilla-central/rev/75fc2600eea8

Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110704 Firefox/7.0a1 Verified issue on Ubuntu 11.04 x86 and WinXP using the following steps to reproduce: 1. Open Firefox with clean profile 2. In options(preferences) menu set When Firefox starts: "Show my windows and tabs from last time" 3. Go into Panorama and create another group and populate it with all the tabs from BBC's latest headlines 4. Open Task manager and observe the memory consumption 5. Switch to initial group (that has only a few tabs) 6. Close Firefox and watch how memory consumption goes down 7. Start Firefox - memory consumption is low considering that only the previous group is loaded 8. Switch to group with BBC headlines and observe that memory starts going up. Setting status to Verified Fixed.

is it me or the implementation of the fix is buggy ? running firefox 10, browser.sessionstore.restore_hidden_tabs was set to false by default and pinned tabs were automatically loaded anyway ? so *** ?

That's a feature. A setting to change that (restore_pinned_tabs_on_demand) will be available in Firefox 12, apparently (bug 708585)

(In reply to zebul666 from comment #48) > running firefox 10, browser.sessionstore.restore_hidden_tabs was set to > false by default and pinned tabs were automatically loaded anyway ? so *** ? Pinned tabs are not in any group and are always visible. That pref doesn't apply to them. As Roman mentioned, there was a different bug, now fixed, that will add an additional switch for pinned tabs.

+1 more pain again for something working fine, now we have to wait until firefox 12.... hoping it will work then...