Definitely needs to happen: * Load N tabs at a time (where N is pref controlled, default of 3-5 probably, < 1 indicates no limit). * When a load finishes, kick off another tab load. * If a tab is switched to, load that one * Some sort of timeout to kick off more loads if no tab completes within a certain time (to combat a bad host or something) * Show page title instead of "Loading..." string Would be nice: * Restore in MRU (needs bug 586067) * Some sort of indicator that a tab isn't actually loaded (will likely just be progress icon/bar) Alex - did I miss anything?

I ended up reorganizing a little bit & moving where we do the setTimeout (from the recursive call the restoreHistory to the first call from restoreHistoryPrecursor. Then everything in restoreHistory just happens in a loop. I didn't see a good reason to keep it how it was and it got a bit confusing to follow and cuts us down to 1 setTimeout (here anyway). Done: * Titles stick * Loads N tabs at a time (mostly, have some XXX comments about that) * when switching to a tab, load it Todo: * Move max concurrent tabs to a pref * Quirkiness with favicons. Since it's something we persist, the loading icon only shows when the load is happening (for pages that had favicons) and before a tab starts loading (for pages that didn't have a favicon). That might be OK for now assuming the tab progress thing happens soon. * Figure out the timeout situation. * Tests Simon, have any thoughts on what I've done here so far?

We'll want bug 586211 to block so that we don't have a bunch of !visible tabs loading & blocking the ones that are visible. There are some other TabCandy-related issues now too... I'll have to look into it (if you change your visible tab set, we need to reorder).

First, I consolidated some logic to make things more simple. Then I probably over-engineered handling tab candy (hidden). I don't think there are any events for changing visibility yet (so that's part of the problem), but once there is then I can simplify a bit. It's actually hard to test because opening tab candy too early is causing me to not have tabs in groups, sort of defeating the purpose...

Comment on attachment 465914 [details] [diff] [review] Patch v0.2 (WIP) generally ok, feedback+. before moving forward, test it with a large enough set of visible tabs that you can tell if there's an improvement for the default case.

This is another way we're attacking the "session makes startup suck" problem. So we'll probably want to block beta 5 since it's a pretty significant behavior change.

(In reply to comment #0) > Definitely needs to happen: > * Load N tabs at a time (where N is pref controlled, default of 3-5 probably, < > 1 indicates no limit). > * When a load finishes, kick off another tab load. > * If a tab is switched to, load that one > * Some sort of timeout to kick off more loads if no tab completes within a > certain time (to combat a bad host or something) > * Show page title instead of "Loading..." string Have we also considered...: * only load tabs that are visible in tabstrip * after first N tabs have loaded, wait until idle timer fires before loading more tabs * obviously if the user switches to tab X, load that tab

Paul, any chance we can get this in before Monday?

Unless Paul has a bunch of work done that's not posted, I think we need to push this to beta 6. There's no conclusion as yet that there's a win here, and any significant changes to session restore should have significant bake-time.

(In reply to comment #6) > * only load tabs that are visible in tabstrip This would be great, at least for my usecase where I usually have lots of tabs open. If we could first load just the visible tabs and then other tabs.

Updated WIP. Cleaned up a bunch and simplified where it could be. Should work a bit better overall. Still todo: * write tests * handle pressing stop Would be nice to do (but should happen in followup) - using this for loading a set of bookmarks.

In the interest of keeping this up to date with status... here's a new WIP. I got the progress listener hooked up so we know when a load finishes or is stopped, so that's good. Next is cleanup, making sure we don't leak everything under stress (ie, quitting before we restore everything) and hopefully figuring out a sane way to test this...

Well, the leak situation hasn't gotten any better, but I only use one progress listener instead of N. That's gotta be better, right? I'm stuck. This doesn't leak if all the tabs load to completion. However, if I quit while any of them are loading, then I leak the world. Somewhere a browser is crying and doesn't want to leave. This isn't in a position to land today (15th) assuming we're still freezing. Perhaps we can say this isn't a "feature" and it can slip past feature freeze?

Just wanted to mention how surprised I am that there's no mention of the BarTab extension in this discussion. It already seems to do everything discussed here. https://addons.mozilla.org/en-US/firefox/addon/67651/ (I've been using it for a long time and with beta 5 too and it works almost perfectly) I feel this would make a good addition to a batch of Officially Suggested Add-Ons... if there were such a thing. At the very least discussing these features with BarTab's author might be helpful to get it fixed and landed.

(In reply to comment #13) > Just wanted to mention how surprised I am that there's no mention of the BarTab > extension in this discussion. He works at Mozilla, so we know about it. ;) However, it has some downsides (you start your browser, about to go on a plane trip, want to queue up all your pages before you go offline), so we'll need to solve it slightly differently. Post-FF4, we're looking into variations of this technique, the main point of this bug is to make FF more responsive on startup.

Leaks are gone. Cleaning up and figuring out how difficult it is going to be to write a reliable (read: not immediately orange) test for this. My guess is fairly difficult. Re: BarTab... So I can (and have locally) changed a few lines and just built that functionality in. You have to change a pref to get it, but that's how it should be I think. Alex is right about the crappy airplane case. We do actually hit the cache when restoring if offline though (or so it happens when I test when offline), so perhaps it's not as bad a situation as it seems. Assuming this makes it through review, I'm inclined to say that's OK since it would only be available via a hidden pref.

(In reply to comment #15) > Alex is right about the crappy airplane case. We do actually hit the cache when > restoring if offline though (or so it happens when I test when offline), so > perhaps it's not as bad a situation as it seems. Yeah, if you already have it in cache, the cache is perfectly aligned with what you're trying to restore, etc, etc. ;) > Assuming this makes it through review, I'm inclined to say that's OK since it > would only be available via a hidden pref. Yes, it's cool that we have it available for the people that want it.

Still testless, but it now passes all of the existing sessionstore tests. Those caught a few issues, so yay tests! I'll push to try to see what happens there. I expect it might have some effect on tests that open a sizable number of tabs (though no local effect on sessionstore tests). There's only 1 XXXzpao comment/question, and I'm pretty sure I know what I should do (my answer is nothing, I can explain if needed). I figured we could get review started without the test - I still haven't worked out exactly how that's going to work.

(In reply to comment #16) > > Assuming this makes it through review, I'm inclined to say that's OK since it > > would only be available via a hidden pref. > > Yes, it's cool that we have it available for the people that want it. Will you fill a new bug for that or address it in this one ?

Fixed one issue uncovered by the new tests: wasn't observing the pref (oops!). Also now has some tests. It's not complete coverage, but does a decent job. Untested: * making sure hiding/showing tabs changes the order they get loaded in * perhaps something else?

Comment on attachment 476079 [details] [diff] [review] Patch v0.8 > >+ // tabs to restore in order >+ _tabsToRestore: { visible: [], hidden: [] }, >+ _tabsRestoringCount: 0, >+ >+ // number of tabs to restore concurrently, pref controlled. >+ _maxConcurrentRestores: null, _maxConcurrentTabRestores, or some shorter way of making it clear that it's tabs. > >+ this._maxConcurrentRestores = >+ this._prefBranch.getIntPref("sessionstore.max_concurrent_tabs"); validate the pref value. handle negative number, or 0. do we want to support some magic number (-1/0) for no restraint on concurrent tabs restored, for people with quantum computers and fiber connections? update: i see that 0 means 1 at a time, should document that in firefox.js and this file. i guess it's makes sense, but best to be explicit. >+ case "sessionstore.max_concurrent_tabs": >+ this._maxConcurrentRestores = >+ this._prefBranch.getIntPref("sessionstore.max_concurrent_tabs"); >+ break; ditto on validation > >+ // If this tab was in the middle of restoring, we want to restore the next >+ // tab. If the tab hasn't been restored, we want to remove it from the array. >+ if (browser.__SS_restoring) { >+ this.restoreNextTab(true); hm, maybe premature, but it feels like we want to yield before doing this. >+ } >+ else if (browser.__SS_needsRestore) { >+ if (aTab.hidden) >+ this._tabsToRestore.hidden.splice(this._tabsToRestore.hidden.indexOf(aTab)); >+ else >+ this._tabsToRestore.visible.splice(this._tabsToRestore.visible.indexOf(aTab)); >+ } what happens for tabs that don't match either above case? is that possible? >+ let tab = aWindow.gBrowser.selectedTab; >+ // If __SS_needsRestore is still on the browser, then we haven't restored >+ // this tab yet. Explicitly call restoreTab to kick off the restore. >+ if (tab.linkedBrowser.__SS_needsRestore) { >+ // In theory, it should only be possible to select visible tabs, but >+ // make sure we look in the right place for the tab. >+ let tabsArr = tab.hidden ? this._tabsToRestore.hidden >+ : this._tabsToRestore.visible; let's just look in the right place, and throw if it's not there. > >+ // Make sure _tabsToRestore is emptied out >+ this._tabsToRestore = { visible: [], hidden: [] }; >+ this._tabsRestoringCount = 0; DRY. make a _initRestoringState() or something like that for the places you do this. >+ return; >+ //XXXzpao maybe we should remove gRestoreTabsProgressListener from each window? yeah should be removed at end of restoration. and re-added as needed. r=me with these changes.

(In reply to comment #20) > >+ // tabs to restore in order > >+ _tabsToRestore: { visible: [], hidden: [] }, > >+ _tabsRestoringCount: 0, > >+ > >+ // number of tabs to restore concurrently, pref controlled. > >+ _maxConcurrentRestores: null, > > _maxConcurrentTabRestores, or some shorter way of making it clear that it's > tabs. Will do. Variable names are hard. > >+ this._maxConcurrentRestores = > >+ this._prefBranch.getIntPref("sessionstore.max_concurrent_tabs"); > > validate the pref value. handle negative number, or 0. do we want to support > some magic number (-1/0) for no restraint on concurrent tabs restored, for > people with quantum computers and fiber connections? > > update: i see that 0 means 1 at a time, should document that in firefox.js and > this file. i guess it's makes sense, but best to be explicit. It's actually better than that :) -1 (well anything < 0) is the magic restore everything number 0 actually means 0 (except that this will restore the selected tab in each window). This gives us a secret bartab behavior :) 1 actually means we'll keep loading 1 at a time That said, I don't think validation is strictly necessary. I can add it though if you really think it necessary. It would really only make sure we were never < -1. > >+ case "sessionstore.max_concurrent_tabs": > >+ this._maxConcurrentRestores = > >+ this._prefBranch.getIntPref("sessionstore.max_concurrent_tabs"); > >+ break; > > ditto on validation Ditto on my no-need comment :) > >+ // If this tab was in the middle of restoring, we want to restore the next > >+ // tab. If the tab hasn't been restored, we want to remove it from the array. > >+ if (browser.__SS_restoring) { > >+ this.restoreNextTab(true); > > hm, maybe premature, but it feels like we want to yield before doing this. (I know we just a quick chat on IRC, but I'll add my thoughts) I had this concern too, but I think we should be OK. We end up hitting webNav.gotoIndex and unwinding, so we shouldn't ever get too deep. I vote we take as is. I'll file a follow though. We probably should be more wary of doing this. > >+ } > >+ else if (browser.__SS_needsRestore) { > >+ if (aTab.hidden) > >+ this._tabsToRestore.hidden.splice(this._tabsToRestore.hidden.indexOf(aTab)); > >+ else > >+ this._tabsToRestore.visible.splice(this._tabsToRestore.visible.indexOf(aTab)); > >+ } > > what happens for tabs that don't match either above case? is that possible? I hope it's not possible. If so, then the world falls apart around us. > >+ let tab = aWindow.gBrowser.selectedTab; > >+ // If __SS_needsRestore is still on the browser, then we haven't restored > >+ // this tab yet. Explicitly call restoreTab to kick off the restore. > >+ if (tab.linkedBrowser.__SS_needsRestore) { > >+ // In theory, it should only be possible to select visible tabs, but > >+ // make sure we look in the right place for the tab. > >+ let tabsArr = tab.hidden ? this._tabsToRestore.hidden > >+ : this._tabsToRestore.visible; > > let's just look in the right place, and throw if it's not there. Ok > >+ // Make sure _tabsToRestore is emptied out > >+ this._tabsToRestore = { visible: [], hidden: [] }; > >+ this._tabsRestoringCount = 0; > > DRY. make a _initRestoringState() or something like that for the places you do > this. fair > >+ return; > >+ //XXXzpao maybe we should remove gRestoreTabsProgressListener from each window? > > yeah should be removed at end of restoration. and re-added as needed. Yeah, that's the right thing. I'll need to make sure that setTabState adds the listener (it doesn't currently) > r=me with these changes. woo!

Pushed http://hg.mozilla.org/mozilla-central/rev/a73c063e52cb

Patch as landed

So now when I start FF, it *looks* like only the front-most tab is loaded. That is because other tabs which are loading are somewhere earlier in the tab list, so they are hidden in the tab strip. (I have ~50 tabs atm)

this path is not working when reopen closed window open 1st window, open 5-6 tabs open 2nd window close 1st window from 2nd window , open 1st from closed windows list result few tabs not loading until you select them

seems to be partially broken. [STR] open 10 tabs. focus/open 1st tab. exit and restart. History > Restore Previous Session 10 tabs open and all tabs are loaded. but open 10 tabs. focus/open 2nd tab (tab besides 1st tab) exit and restart. History > Restore Previous Session 10 tabs open and only 3 tabs are loaded.

Please file new bugs for problems with the functionality implemented here, rather than commenting (or reopening), and mark them as blocking this bug.

(In reply to comment #27) > Please file new bugs for problems with the functionality implemented here, > rather than commenting (or reopening), and mark them as blocking this bug. (In reply to comment #26) filed this, bug 597933

I have a question about the functionality here and it's affect on observer notifications. For example currently when a session has finished loading SessionStore sends out a "sessionstore-browser-state-restored" or ""sessionstore-windows-restored" notification to indicate the load is finished. Looking at the new code, it looks like that notification will get sent out before the cascade load completes. Is that correct? Also how does this affect other notifications like SSTabRestoring and SSTabRestored?

(In reply to comment #29) > I have a question about the functionality here and it's affect on observer > notifications. For example currently when a session has finished loading > SessionStore sends out a "sessionstore-browser-state-restored" or > ""sessionstore-windows-restored" notification to indicate the load is finished. > Looking at the new code, it looks like that notification will get sent out > before the cascade load completes. Is that correct? That's correct. Those topics haven't properly meant "restored" for a while. They only indicate that the windows have been opened and tabs have been set up. That hasn't changed. > Also how does this affect other notifications like SSTabRestoring and > SSTabRestored? These still behave the same. SSTabRestoring is still used to indicate that the chrome for the tab has been restored, but before any page loads take place. SSTabRestored is fired after the tab has been loaded (or "restored")

BTW, session is restored in a new windows, not in a current window. is this intended ? should be in current window ?

(In reply to comment #31) > BTW, > > session is restored in a new windows, not in a current window. > is this intended ? > should be in current window ? If I'm understanding right, I think you're talking about "Restore Previous Session" from the history menu, which is bug 588482.

(In reply to comment #32) > (In reply to comment #31) > > BTW, > > > > session is restored in a new windows, not in a current window. > > is this intended ? > > should be in current window ? > > If I'm understanding right, I think you're talking about "Restore Previous > Session" from the history menu, which is bug 588482. yes, from history menu. sorry for posting here.

is it possible to somehow unload an already loaded tab (without add-ons like bartab)? For example by applying some script through the error console?

Until recently (about 4 nightlies ago) only tabs of the currently active tabgroup were restored when you restarted firefox, this was pretty nice as it saved memory by not loading tabs in groups you rarely touch. Now firefox seems to load all tabs from all groups on startup. Is this intended behavior?

(In reply to comment #36) > Until recently (about 4 nightlies ago) only tabs of the currently active > tabgroup were restored when you restarted firefox, this was pretty nice as it > saved memory by not loading tabs in groups you rarely touch. That should never have been the case. There is a bug to do that though - not sure if it will make Fx4: bug 595601. > Now firefox seems to load all tabs from all groups on startup. Is this intended > behavior? Yes.

(In reply to comment #37) > > Now firefox seems to load all tabs from all groups on startup. Is this intended > > behavior? > > Yes. Can we have a pref to turn on the old behavior?

(In reply to comment #38) > (In reply to comment #37) > > > Now firefox seems to load all tabs from all groups on startup. Is this intended > > > behavior? > > > > Yes. > > Can we have a pref to turn on the old behavior? That was the old behavior. It never should have been (and I don't believe it ever was) the case that only tabs from the active group were restored.

Since 2011-08-15 build i get hangs in GetRootSHEntry. It does not seem to find the parent node/entry/whatever ... Once the tab was loaded once it does not happen again (it happens on first load either from startup or group change). Two of the call stacks (2011-08-16 build): > xul.dll!GetRootSHEntry() Line 10470 C++ xul.dll!nsDocShell::SetHistoryEntry(nsCOMPtr<nsISHEntry> * aPtr=0x04ad2b7c, nsISHEntry * aEntry=0x0b3d5b50) Line 10490 C++ xul.dll!nsDocShell::InternalLoad(nsIURI * aURI=0x017ce52f, nsIURI * aReferrer=0x04ad2a98, nsISupports * aOwner=0x10590d40, unsigned int aFlags=274271728, const wchar_t * aWindowTarget=0x00000000, const char * aTypeHint=0x00000000, nsIInputStream * aPostData=0x00000000, nsIInputStream * aHeadersData=0x01dca5c4, unsigned int aLoadType=0, nsISHEntry * aSHEntry=0x00000000, int aFirstParty=4, nsIDocShell * * aDocShell=0x0b3d5b50, nsIRequest * * aRequest=0x00000001) Line 8684 C++ mozjs.dll!SearchTable(JSDHashTable * table=0x00000000, const void * key=0x0b3d5b50, unsigned int keyHash=274271728, JSDHashOperator op=JS_DHASH_LOOKUP) Line 436 + 0x14 bytes C++ xul.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x0000088b, unsigned int methodIndex=3946857, unsigned int * args=0x00000000, unsigned int * stackBytesToPop=0x0417be08) Line 114 + 0x15 bytes C++ > xul.dll!GetRootSHEntry() Line 10470 C++ xul.dll!nsDocShell::SetHistoryEntry(nsCOMPtr<nsISHEntry> * aPtr=0x0602a178, nsISHEntry * aEntry=0x1b7b8dd0) Line 10490 C++ xul.dll!nsDocShell::Embed(nsIContentViewer * aContentViewer=0x0fe05d10, const char * aCommand=0x019b9733, nsISupports * aExtraInfo=0x00000000) Line 5835 C++ xul.dll!nsDocShell::CreateContentViewer(const char * aContentType=0x0f027a68, nsIRequest * request=0x06037030, nsIStreamListener * * aContentHandler=0x06037030) Line 7584 + 0x15 bytes C++ xul.dll!nsDSURIContentListener::DoContent(const char * aContentType=0x0f027a68, int aIsContentPreferred=0, nsIRequest * request=0x00791100, nsIStreamListener * * aContentHandler=0x23b2c7cc, int * aAbortProcess=0x0012cd20) Line 149 C++ xul.dll!nsDocumentOpenInfo::TryContentListener(nsIURIContentListener * aListener=0x07cc5380, nsIChannel * aChannel=0x06037030) Line 758 C++ xul.dll!nsDocumentOpenInfo::DispatchContent(nsIRequest * request=0x06037030, nsISupports * aCtxt=0x0012cf3c) Line 454 + 0x16 bytes C++ xul.dll!nsDocumentOpenInfo::OnStartRequest(nsIRequest * request=0x000000c8, nsISupports * aCtxt=0x00000000) Line 301 C++ xul.dll!nsHttpChannel::CallOnStartRequest() Line 721 C++ xul.dll!nsHttpChannel::ContinueProcessNormal(unsigned int rv=0) Line 1172 C++ xul.dll!nsHttpChannel::ProcessNormal() Line 1109 C++ xul.dll!nsHttpChannel::ProcessResponse() Line 996 C++ xul.dll!nsHttpChannel::OnStartRequest(nsIRequest * request=0x0a199290, nsISupports * ctxt=0x00000000) Line 4046 C++ xul.dll!nsInputStreamPump::OnStateStart() Line 445 C++ xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream=0x04067b68) Line 397 + 0x8 bytes C++ xul.dll!nsInputStreamReadyEvent::Run() Line 115 C++ xul.dll!nsThread::ProcessNextEvent(int mayWait=16983422, int * result=0x0f00ed80) Line 637 C++ nspr4.dll!_MD_CURRENT_THREAD() Line 310 C xul.dll!MessageLoop::RunHandler() Line 206 C++ xul.dll!MessageLoop::Run() Line 180 C++ xul.dll!nsBaseAppShell::Run() Line 191 C++ xul.dll!nsAppShell::Run() Line 261 + 0x6 bytes C++ xul.dll!nsAppStartup::Run() Line 225 C++ xul.dll!XRE_main(int argc=3, char * * argv=0x00a1a0c0, const nsXREAppData * aAppData=0x00a165c0) Line 3547 C++ firefox.exe!wmain(int argc=-234370879, wchar_t * * argv=0x00401b72) Line 107 + 0x602 bytes C++

Volkmar, your problem has nothing to do with this bug. Could you please file a new bug, and CC me.