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

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

Summary: Option to not load tabs from inactive groups on initial browser startup → Option to not load tabs from inactive groups on initial browser startup (and until such time as the tab(s) become part of an active group)

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.

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

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.

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!

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

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 *** ?

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