Last Comment Bug 586068 - Cascade page loads when restoring
: Cascade page loads when restoring
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
-- normal with 6 votes (vote)
: Firefox 4.0b7
Assigned To: Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
:
: Mike de Boer [:mikedeboer]
Mentors:
: 521964 (view as bug list)
Depends on: 598267 629232 586211 596806 597716 597757 597807 597901 597933 598011 598020 598221 599909 600007 604551 606647 607016 611514 614945 617062 618151 623893
Blocks: 582005 597848 675539 496458 561149 595601 597584 675541
  Show dependency treegraph
 
Reported: 2010-08-10 13:36 PDT by Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
Modified: 2014-04-26 02:23 PDT (History)
41 users (show)
paul: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta7+


Attachments
Patch v0.1 WIP) (12.92 KB, patch)
2010-08-11 21:36 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
no flags Details | Diff | Splinter Review
Patch v0.2 (WIP) (18.50 KB, patch)
2010-08-13 18:26 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dietrich: feedback+
Details | Diff | Splinter Review
Patch v0.3 (WIP) (23.22 KB, patch)
2010-09-07 13:42 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
no flags Details | Diff | Splinter Review
Patch v0.4 (WIP) (26.37 KB, patch)
2010-09-13 17:24 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
no flags Details | Diff | Splinter Review
Patch v0.6 (WIP) (26.70 KB, patch)
2010-09-15 00:05 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
no flags Details | Diff | Splinter Review
Patch v0.7 (22.72 KB, patch)
2010-09-16 00:08 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
no flags Details | Diff | Splinter Review
Patch v0.8 (32.79 KB, patch)
2010-09-16 16:15 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dietrich: review+
Details | Diff | Splinter Review
Patch v1.0 (as checked in) (34.03 KB, patch)
2010-09-17 16:04 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
no flags Details | Diff | Splinter Review

Description User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-08-10 13:36:25 PDT
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?
Comment 1 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-08-11 21:36:24 PDT
Created attachment 465120 [details] [diff] [review]
Patch v0.1 WIP)

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?
Comment 2 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-08-12 15:36:36 PDT
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).
Comment 3 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-08-13 18:26:59 PDT
Created attachment 465914 [details] [diff] [review]
Patch v0.2 (WIP)

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 4 User image Dietrich Ayala (:dietrich) 2010-08-18 04:05:48 PDT
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.
Comment 5 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-08-18 12:18:43 PDT
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.
Comment 6 User image Mike Beltzner [:beltzner, not reading bugmail] 2010-08-23 21:15:42 PDT
(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
Comment 7 User image Mike Beltzner [:beltzner, not reading bugmail] 2010-08-27 12:44:24 PDT
Paul, any chance we can get this in before Monday?
Comment 8 User image Dietrich Ayala (:dietrich) 2010-08-29 03:08:31 PDT
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.
Comment 9 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-09-07 01:13:49 PDT
(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.
Comment 10 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-09-07 13:42:52 PDT
Created attachment 472746 [details] [diff] [review]
Patch v0.3 (WIP)

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.
Comment 11 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-09-13 17:24:32 PDT
Created attachment 474924 [details] [diff] [review]
Patch v0.4 (WIP)

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...
Comment 12 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-09-15 00:05:48 PDT
Created attachment 475436 [details] [diff] [review]
Patch v0.6 (WIP)

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?
Comment 13 User image thinsoldier 2010-09-15 14:19:20 PDT
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.
Comment 14 User image Alex Limi (:limi) — Firefox UX Team 2010-09-15 14:26:34 PDT
(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.
Comment 15 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-09-15 14:55:48 PDT
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.
Comment 16 User image Alex Limi (:limi) — Firefox UX Team 2010-09-15 15:10:03 PDT
(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.
Comment 17 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-09-16 00:08:49 PDT
Created attachment 475770 [details] [diff] [review]
Patch v0.7

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.
Comment 18 User image Nicolas Mandil (:NicolasWeb) 2010-09-16 01:44:28 PDT
(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 ?
Comment 19 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-09-16 16:15:23 PDT
Created attachment 476079 [details] [diff] [review]
Patch v0.8

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 20 User image Dietrich Ayala (:dietrich) 2010-09-16 22:40:21 PDT
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.
Comment 21 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-09-16 23:39:25 PDT
(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!
Comment 22 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-09-17 16:02:35 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/a73c063e52cb
Comment 23 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-09-17 16:04:41 PDT
Created attachment 476423 [details] [diff] [review]
Patch v1.0 (as checked in)

Patch as landed
Comment 24 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-09-18 06:22:41 PDT
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)
Comment 25 User image tabmix.onemen 2010-09-19 02:25:59 PDT
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
Comment 26 User image pal-moz 2010-09-19 17:25:47 PDT
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.
Comment 27 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2010-09-19 19:51:32 PDT
Please file new bugs for problems with the functionality implemented here, rather than commenting (or reopening), and mark them as blocking this bug.
Comment 28 User image pal-moz 2010-09-19 20:56:36 PDT
(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
Comment 29 User image Michael Kraft [:morac] 2010-09-20 15:41:39 PDT
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?
Comment 30 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-09-20 22:22:13 PDT
(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")
Comment 31 User image pal-moz 2010-09-23 22:12:16 PDT
BTW,

session is restored in a new windows, not in a current window.
is this intended ?
should be in current window ?
Comment 32 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-09-23 23:48:32 PDT
(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.
Comment 33 User image pal-moz 2010-09-24 00:29:50 PDT
(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.
Comment 34 User image Please Ignore This Troll (Account Disabled) 2010-10-08 13:44:28 PDT
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?
Comment 35 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-11-19 10:42:48 PST
*** Bug 521964 has been marked as a duplicate of this bug. ***
Comment 36 User image The 8472 2010-11-25 11:30:51 PST
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?
Comment 37 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-11-26 10:11:08 PST
(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.
Comment 38 User image Please Ignore This Troll (Account Disabled) 2010-12-06 14:32:43 PST
(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?
Comment 39 User image Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-12-06 15:07:00 PST
(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.
Comment 40 User image Volkmar Kostka 2011-08-16 10:04:34 PDT
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++
Comment 41 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-08-16 10:16:13 PDT
Volkmar, your problem has nothing to do with this bug.
Could you please file a new bug, and CC me.

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