Closed Bug 36810 Opened 24 years ago Closed 15 years ago

Autosave URIs of open windows and tabs for later restoration in event of crash (session restore)

Categories

(SeaMonkey :: UI Design, enhancement, P3)

enhancement

Tracking

(Not tracked)

VERIFIED FIXED
seamonkey2.0a3

People

(Reporter: BenB, Assigned: misak.bugzilla)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: fixed-seamonkey2.0, Whiteboard: [Firefox-parity])

Attachments

(2 files, 30 obsolete files)

7.14 KB, text/plain
Details
130.35 KB, patch
ajschult784
: review+
Details | Diff | Splinter Review
<quote src="news://news.mozilla.org/3900EFDB.B3117412@gwu.edu">
Subject: Failure Tolerance of Netscape
Date: Fri, 21 Apr 2000 20:18:35 -0400
From: Ben Ruppel <slate@gwu.edu>
Newsgroups: netscape.public.mozilla.seamonkey

[...] I am trying to
think of ways to make the imminent Mozilla crash less frustrating,
because when Netscape 4.x crashes and I loose 15 some pages and the
message I've been typing, it's quite enraging.
[...]
I think Mozilla should keep a persistant record of open
windows.  It should generate an html file that is updated every time the
open URL's change.  This should be accessable from the browser so we can
get the heck back to what we were doing without searching the history or
hunting for the links again.
</quote>
(Also see follow-ups)

Such a feature could save me a lot of time (for reconstructing 10 windows 3
times a day or so).


I don't even think, it's that hard to implement. Maybe, you just have to hook
into some URL loading function (anywhere high-level, i.e. in the browser?),
update the state and save it. Maybe I'm naive, but why can't you just write file
B and then copy B over A? The copy operation is pretty atomic, isn't it? But I'd
capsulate the save operation, so there can be platform-specific implementations,
which don't go to a harddisk.
Oh boy, I just lost 4 paragraphs on this because mozilla forgot my text field
when I hit the back button when I tried to send the message but didn't have the
password.. here goes again...

Okay, it should keep an html log.  Security nuts won't like it so it'll be
optional.  The log could have a closing tag that mozilla puts in when properly
shut down.  If no closing tag is found, mozilla would know not to overwrite the
existing tags and to open it up upon launch, allowing for link recovery.

But then things can get sticky, as what happens if it crashes halfway through
restoring those links?  Would session number 2's history overwrite the first
one's?  What would be recovered on the third restart?

Obviously, multiple logs should be kept.  User definable limits, like save my
last 10 or 100 logs.  There could be a menubar button called "sessions" or
something, and the dropdown box would contain the last X logs, clicking on one
would open the pages full of the urls that were open.

A little complex, make it an option for power users only and nobody gets hurt,
and mozilla will have an incredible incredible incredible (did I say
incredible?) advantage over internet explorer.  The code wouldn't be that hard,
I curse my choice of mechanical engineering over comp sci!
Okay, I was thinking of creating the concept of "sessions" in mozilla.  
Basically, besides keeping these log files of html pages open at a time, a user 
could at any time create a "session", which could be added to a menu that 
works much like bookmarks work.  In the "sessions" menu or button or whatever, 
there would be a section of user save sessions and sessions from recent crashes. 
 There would have to be a distinction between loading and viewing a session, as 
loading would open up the links, each in a new browser window, while viewing it 
would open up an index page full of the links.  I really think each of these 
options would be neccessary in order to make sessions the most usefull. 

Furthermore, with the loading sessions concept, people could open up the exact 
pages they want automatically upon opening mozilla.  I've heard calls for this 
forever.  Most people first start up their browser to check a preset set of 
pages, and this pefectly enhances that process.  Configuration would be done in 
a command line, such as " mozilla -session compnews".  So if I wanted to read 
computer news, I could make a shortcut for it and there you go.  People could 
have a bunch of mozilla shortcuts, each having certain groupings of what they 
want to see.  The many ways this could be used astounds me.  
What does everyone think?
Alphanumerica is working on it (crash recovery). Maybe, David can keep slate's

session management ideas in mind and we file a new dependant bug for them?

adding helpwanted keyword and assigning to nobody@mozilla.org where a bunch of 
the helpwanted bugs live.
Assignee: asadotzler → nobody
Keywords: helpwanted
Sorry for the spam.  New QA Contact for Browser General.  Thanks for your help
Joseph (good luck with the new job) and welcome aboard Doron Rosenberg
QA Contact: jelwell → doronr
REASSIGNing to Pete Collins, who's "Total Recall" fixes this bug. Pete, could
you take care of the checkin (i.e. get a review, approval by brendan or waterson
and find somebody to check it in), please?
Assignee: nobody → pete
Ben, I'll talk to Brendan and Waterson about this bug and checking in the fix.

I have this sitting in my tree on my unix build but i need to update it to the
latest version, as well as find out where to put it in the mozilla menus.

I'll gladly accept this bug. ;-)

pete
> i need to [...] find out where to put it in the mozilla menus.

Preferable something where I can make it go away during the session after the crash.
accepting bug. I won't have time to do this until after PR2.


pete
accepting bug
Status: NEW → ASSIGNED
Does it still work with M18? can we get this in, now? I don't have the time to
catch every build, but the number of crashes make the browser still unusable for me.
em, patch rather. pete, if it still works, you just need to find somebody to
review it and check it in. Not much work for you.
No Ben, I talked to Waterson and Brendan. What i need to do is move the code
over to extensions and add the necessary makefiles so this can be a build flag.

I is not going to be be part of the default build.


--pete

> over to extensions and add the necessary makefiles

ic.

> I is not going to be be part of the default build.

Why not? IMO, this is one of the coolest non-Netscape contributions Mozilla has
seen.
Well, i think everyone wants to test it out and make sure it is solid before any
merging is done. They are past feature freeze right now i beleive.

pete
Ok, i have this ready to go . . .

I mailed Brendan for approval.

pete
any development on this?
ping?
Keywords: mozilla1.0
I don't think we are going to see any checkins for a while. I can't get anything
reviewed. Everyone i think is pushing hard to get a browser out the door.

--pete
Don't matter. Mozilla live must go on. Who is it who doesn't answer?
It is up to Brendan or Waterson i beleive . . . right?

--pete
Gimme a patch to review.  Or give a better reviewer from the list, by area.

/be
I am not going to be able to do this anytime soon . .
For those interested, there is a full WORKING IMPLEMENTAION of this in Aphrodite
0.05. I did some updating today so it is working again.

--pete
What is the current status of this bug?
What would I have to to to add the code to my source tree (on Linux)?
OK, i'll put this together as an extensions package as soon as i can.
Then perhaps people can poke and tweak.

Thanks

--pete
Blocks: 9274
is this dead?!

i would love this (lost 23 pages just a few minutes )8
mozilla is alot more stable, but now when i lose it, it take down too many hours
of surfing, too many pages...
and would be a great thing to "pause" the surfing (changing computers, unstable
PC, boss coming, etc), just close, run later and all the pages back

what is needed for this enter in mozilla tree?!
i could try to help, but i dont know C and i dont know what to do to help, but i
learn fast 8)
Basically what this implementation did is on each page load it queried the open
windows through nsIWindowMediator and saved things like the url, title, width,
height and name to a crash.rdf file in the users moz home dir.

It is really not hard to do at all.

You don't need to implement anything in C, it can all be done in js.

I'll attach the old js file used in aphrodite.

--pete
Attached file js file —
I started a project here but haven't worked on it in a while.

http://recall.mozdev.org/

This was going to be an xpi package.

--pete
*** Bug 84914 has been marked as a duplicate of this bug. ***
This bug better not be dead -- mine was changed to being a duplicate of this one
so I'm watching for new details.

My proposal(s):
1) have a manual "save session (as...)" option that allows the user to define
sessions of browser windows.  In the save dialog, allow the user to request the
windows' positions and state to be saved or just the URIs.
2) have an automatic session maintenance function that uses the same back-end
function as #1 above, but runs whenever the user changes URIs.  This need not be
intensive -- we are already dealing with history information.  There is also no
need for this to be stored seperately from history information to some degree --
marking pages in the history with a session identifier should be sufficient, and
then not removing session-related pages from the history.

Sessions should be visible from within the Bookmarks view and loading a previous
session should offer the option of either replacing all current browser windows
or being in addition to those already open.

Eventually adding a back-end "make this page available offline" function like
the one in Internet Explorer&tm; would help as well, since the session caching
routine could then mark the respective URIs for local (semi-permanent) caching
(if not just by markers stating to not expire the data, until reloaded of
course).  For those of us who spend much of our time with the same sites open in
session-like activities, bookmarks are frustrating and almost useless compared
to being able to resume a full arbitrary session.  Since the browser still
crashes from time to time (depending on what you do with it), automatic sessions
would also be wonderful for the testing community (--resume-session=last or some
such command-line option).
Michael, if a bug is open then by definition it is not `dead'. But the speed at 
which a bug gets fixed depends entirely on the number of people wanting to fix 
it (*not* on the number of people wanting it fixed). At the moment no-one is 
willing and able to implement this RFE, which is why it is marked `helpwanted'.

--> XP Apps
Assignee: pete → pchen
Status: ASSIGNED → NEW
Component: Browser-General → XP Apps
QA Contact: doronr → sairuh
the <a href="http://recall.mozdev.org/">recall</a> isnt working anymore with the
new mozilla, i think that the tab support broke it
marking future
Target Milestone: --- → Future
*** Bug 113494 has been marked as a duplicate of this bug. ***
*** Bug 115790 has been marked as a duplicate of this bug. ***
Blocks: 63094
Blocks: advocacybugs
No longer blocks: advocacybugs
-> default assignee
Assignee: pchen → trudelle
Target Milestone: Future → ---
->future
Target Milestone: --- → Future
Maybe use X session management on Unix/X11?
<http://developer.gnome.org/doc/GGAD/sec-sessionmanagement.html>
If I shut down right after I finish loading
http://www.webstore.com/purchase.cgi?item=fedora&one_click_shop=1, will I buy
another hat when I start up the next time?
That store is broken. You should never buy using GET. What happens, if you
bookmark that page?
I'm sure the store is broken, and that I should never buy with GET.  My point,
perhaps lost in the fact that I chose an example, which can be deconstructed and
disputed in its meaningless component parts rather than taken as a
representative of a class of concern, is this: not all URLs are idempotent. 
Various server-administration URLs for Netscape web servers, back in the day
when they existed, were GET- and query-string-driven, probably precisely so that
you _could_ bookmark "restart server" or "reload app" for ease of use during
development and debugging.

(Bookmarking the page is a much more explicit act than shutting down when I
forget that I have a window open on another desktop there.)

Maybe we don't care, and caveat user who turns this feature on (in which case,
perhaps we should have a caveat-window pop up the first time it's enabled for a
profile), but I'd like to at least see someone say that.  There had been no
mention of this in the course of this bug, that I could see, and I just wanted
to make sure that we took it into account.

I want this feature too, and I think the Unix session-management angle is a good
one for platforms that support it (if not, none of your other apps have implicit
cross-session state either).  Of course, I don't think it's going to happen for
1.0, unless someone comes up with a miracle patch in the very near future --
minimal, non-intrusive and easy to subject to large-scale testing.  (And I sure
wouldn't hold Mozilla 1.0 if this were the last bug left.)
shaver, I indeed misunderstood you, thinking that you argued for wontfix. Yes,
you are right, this is something to look out for. Maybe a confirmation dialog
(when the browser is reopened) like Mozilla does for POST-Reloads now (because
it can't cache them). Maybe that's overkill, however, considering that most
queries (GETs) are probably harmless and useful, while dialog boxes are
annoying, esp. with session managment (you just logged into your machine and
Mozilla asks 10 questions).
If it is a true "come back to where I left off" then I don't think, ideally,
Mozilla should re-fetch pages from the network on session resume. Just like we
don't re-fetch the page when the user switches tabs or windows.
No longer blocks: 9274
*** Bug 128423 has been marked as a duplicate of this bug. ***
*** Bug 127637 has been marked as a duplicate of this bug. ***
Once this feature request is implemented, we should be able to resolve bug
19454, crash recovery, and save session information, including URI's
automatically in the event of a crash. We should also be able to resolve bug
63094, ability to manually save session information. 
Blocks: 19454
Blocks: 93789
*** Bug 146170 has been marked as a duplicate of this bug. ***
Would the desired functionality on the saving of open window data not be the 
easiest part?  Writing a file each time the browser changes pages / opens new 
windows could be done in the history-handling routines.  The reloading and 
positioning of those windows may be more difficult, but it can be left out 
while the former is tested; once the information is being tracked, using it is 
a lot easier.
I think saving URL of open windows and tabs is now very easy, because now you
can add all open tabs as group in bookmark (menu "Bookmarks"->"File Bookmark..."
and chechbox "File as group").
We need simple program, that uses this automatically, when user opens new window
or new tab.
Cool! I'm not even sure this bug needs fixing anymore :-)
We still need "autosave" to work. That functionality can probably be built on
top of bookmark groups.
Keywords: mozilla1.1
I agree with Andrew Hagen
*** Bug 149002 has been marked as a duplicate of this bug. ***
*** Bug 155319 has been marked as a duplicate of this bug. ***
Re comment 51: this is about autosaving for purposes of crash recovery, not
saving lots of user-preferred pages in the bookmarks file easily. 

Total Recall still works ok for me on 1.0, as long as I don't worry about loing
all but one tab from each window :|
Crash recovery is the killer feature that keeps me using Galeon.  Why isn't
there support in the mainline Mozilla code for good crash recovery like Galeon
has?  This shouldn't be an add-on, but core code...
deven, we accept patches.
Well, I'd love to offer a patch, but I don't have one.  Moreover, I wouldn't
know where to begin in terms of creating one.

I'm an experienced developer, but there is a barrier to entry here.  I realize
it's not an intentional barrier, but it's there nonetheless.  Basically, Mozilla
is so large and complex that it's hard to get started at working on the code,
especially for people who have little time to spare.  I've wanted to work on
various things in Mozilla for years, but I haven't had the time to familiarize
myself with this million-line codebase or the many APIs and standards Mozilla
uses or establishes.  Maybe I'm just missing it, but at the moment it's not very
clear how to effectively get involved without a large time commitment...
I think that, even if it's off topic part of Comment #60, Deven is right. I have
similar problem. I'm working with many big projects in many different languages,
but Mozilla is really huge. Even if i read a part about downloading a source,
and building (part for Windows is not true). But i think there should be some
more or less simple tutorial "how to change one simple thing and view results".

Back to bug - wouldn't reslove problem simple "dirtyhack" with using some hidden
"Group of Bookmarks"? And after crash this group should be placed as Start Location.
This would be a killer feature.
Nav triage team: nsbeta1-
Keywords: nsbeta1nsbeta1-
*** Bug 187175 has been marked as a duplicate of this bug. ***
*** Bug 193302 has been marked as a duplicate of this bug. ***
Blocks: majorbugs
For the non-crash case, see attachment 94617 [details] (of bug 39057) for an example of
how the UI could be implemented (the attachment is a collage of screenshots of
Opera).

For the crash case (and, actually, perhaps for non-crash too), please note that
if we are to preserve all state (and I would love that), it's not enough to do
what manual bookmarking would do: the browsing _history_ of the windows/tabs
should also be preserved, so that the back/forward buttons work as before the
crash. Also, cookies that are set to expire at end of browsing session should be
preserved. And data in forms, too. And any downloads should be resumed, but
there's a separate bug for that capability.
Those would all be nice, but I don't think they're essential or a necesary part
of this bug.
Here is one possible solution. First, we add an additional flag to each
history.dat entry. Its state will be either 0 or 1. When a user browses to a
page, the history.dat entry that is generated has its state set to 0. Once the
user closes the tab or window, or browses to a new site in the existing tab or
window, it is set to 1. When you need a list of all pages that were open before
Mozilla crashed, query history.dat for all entries with flags set to 0.
Good idea.
*** Bug 200723 has been marked as a duplicate of this bug. ***
This bug is already fixed - just install Tabbrowser Extensions:

http://white.sakura.ne.jp/~piro/xul/_tabextensions.html.en

these extensions add lot of useful functions to mozilla:
 - Auto Reload (Reloads tabs with favorites interval)
 - You can open last visited tabs instead of last visited page when Navigator
starts up (also browsing history is saved !!!)
 - Also this can restore tabs at the next startup of crash - Tabbrowser
Extensions autosaves URL of open web pages !!! (browsing history is saved too !!!)
 - this closes bug #36810 (Multi-tabbed windows should confirm close)
 - You can undo "Close Tab" until fifty times ago
 - You can re-open the closed tab by Ctrl+Shift+Z or Alt+Z
 - Tabs are moved and re-ordered with drag and drop.
 - zillion of other improvements.

Mozilla developers, please include this wonderfull utility into Mozilla.
Sorry but even if Egle's suggested extension solves the problem, the
solution must become a core part of Mozilla. Is this extension part of
the build testing? If so, when could this capability be merged into
the trunk? From reading comments, it sounds as though there have been
several patches for this problem. How many hours have been lost by this
basic missing capability in the three years since this bug was reported?

Personally, I think the severity of this should be 'major', not
'enhancement' since basic instability and lack of robustness prompts
people to look for alternatives that don't cause them to lose work.
I agree. When I am working on something very important, I choose Opera (6.05;
not sure if the newer is as fault tolerant). If I need to be able to save whole
web pages with images in one file, I use IE. Very inconvenient...
Is Pete Collins' recall.mozdev.org a dead project? does it still work with
current builds?

Prog.
It's dead in the respect that I can't make the time to work on it. It is a great
starting point if anyone want's to adopt the project.
I do think that this is one of the most important features when it comes to
making people move from Opera/IE to Mozilla. I for example (and a lot of my
friends, and I mean A LOT), still mainly use Opera for browsing as I simply had
entering my 5 favorite sites addresses everytime I start my browser. Though I
never think that this feature will make it while the Mozilla project still
exists (because of the new 1.4+ branch with the Phoenix replacement), I still
hope that it will be implemented soon.
No longer blocks: 63094
No longer blocks: 93789
Keywords: mozilla1.3
*** Bug 207082 has been marked as a duplicate of this bug. ***
Still no progress on this non-capability? Yet work continues on other
non-essentials. Sorry, but Mozilla's instability and lack of robustness
is becoming unbearable. It's been more than three years now. Is it really
that hard to maintain a file that contains the URLs of all open windows?
Even a partial solution would be better than the nothing that exists now!
I've been a loyal user since Netscape 0.9 but losing work constantly is
getting really old...

The severity of this bug must be elevated to 'critical'. By not fixing this
problem, crashes are causing a major loss of data.
Pete Collins updated the recall to work again in recent builds and with tabs...
check http://bugzilla.mozilla.org/show_bug.cgi?id=19454 and send him some beer...

also check recall.mozdev.org 

isnt perfect, specially for tabs, but works... now people can hack it to add
more things, fix bugs, etc
Does recall work for Firebird?
I was wondering about the relation between this bug and bug that you pointed out
now: http://bugzilla.mozilla.org/show_bug.cgi?id=19454
Above on this page, it shows 36810 block 19454, but they seem more like
duplicates to me. If 19454 is solved, this should also be assumed to be solved,
and vice versa. So shouldnt they be marked duplicates?
And yes, I think both of these should be marked "critical", loss of browsing
information is actually critical, imho. (I havent check recall yet)
Let's keep in mind that projects like Recall are not solutions to this bug, and
all of us saying how much we want this feature won't make it materialize.

I just wanted to mention that the TabBrowser extension (for Firebird) can save
your tabs and reload them the next time you start.  It doesn't always work when
Mozilla actually crashes, but it's better than nothing.  What I'd like to see is
saved history in each tab, but that's another dream I'm not helping materialize.
Recall <http://recall.mozdev.org/> installed a Jar file but it doesn't seem
to do anything. Hell, it took five minutes to even find mention of it on
UI. Don't seem to be able to turn it on and crashing the browser doesn't
invoke it automatically. I'm also not sure I'm following what exactly
it does since there didn't appear to be any documentation either. Was there
some configuration required?

Since various sites across the internet will cause Mozilla to crash
on my machine (e.g. http://ericjohnson.com), restoring all open windows
will leave me right where I started. What's the name of the file containing
the open window history?

For the record, even assuming this extension can be made to work, I don't
find this a general purpose solution unless it's integrated into the product.
For that matter, does it need to be reinstalled every time I upgrade this 
browser?
install the recall XPI, it will install the javascript lib that he uses also...

restart the browser

now in the tools menu you have a recall item
check the on/off to enable the recall funcionality

browse the web, open the tabs, new windows, etc
see that you have one item in the recall sub-menu for each windows...
for tabs there is no diference, its stays the same

"crash" the browser (usually is some pluging crashing, not really mozilla)

reopen the browser
go to the recall menu, click on the itens with the name of the windows you want
to recover, they will popup with only the first tab loaded... it appers that is
all over, but wait...

wait a few seconds/minutes (on a slow computer 8) , the more tabs you had on
that windows, the more it takes loading
all the tabs will show up at the same time, all loading

do the same for the others windows you want to recover

if you start browsing the web without recovering, i think you will lose the
saved windows and tabs


Well, upgrading from Shockwave Flash NP-PPC 6.0 r29 to.
Shockwave Flash NP-PPC 6.0 r79 seems to have fixed the
crashing problem with http://ericjohnson.com/.

>now in the tools menu you have a recall item
>check the on/off to enable the recall functionality

Now this is where I ran into problems. The menuitem label is
literally "Off/On" (as opposed to just "Off"). But selecting
the menuitem never produces a checkmark (nor, more logically,
just changes the menuitem label to "On"). The other menuitem
(labeled ".Restore All") is always desensitized. Any ideas?

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4) Gecko/20030624
Any possibility my inability to get Recall to function is related to this?

<http://www.mozilla.org/projects/plugins/plugin_scripting_ABI_technote.html>
No, it's talking about a different kind of plugin. You cannot use such an old
add-on with new Mozillas, way too much changed. Don't even try.
*** Bug 218985 has been marked as a duplicate of this bug. ***
Flags: blocking1.6a?
This could be considered a privacy issue and we'd need to make sure people
realize that this information would not be used in any way by Mozilla.org
*** Bug 220754 has been marked as a duplicate of this bug. ***
*** Bug 224523 has been marked as a duplicate of this bug. ***
Flags: blocking1.6b?
Flags: blocking1.6a?
Flags: blocking1.6b?
Flags: blocking1.6b-
Flags: blocking1.6a?
Recall is back in action. I updated it last week to work w/ Mozilla 1.5 +.

  http://recall.mozdev.org/

Any bugs, RFE's etc, file them on mozdev.

Regards

Voting there, but I liked the bug #220754 phrasing better (or Camino #152147 bug
description).

I am much more concerned about keeping as much of the UI layout (at least open
tabs and windows with respective URLs, possibly pages history also) when
quitting. Crash recovery is not much of an issue in my experience (I found
Mozilla pretty stable for years now). But as a end user, I often restrain myself
from quitting mozilla for fear of losing my navigation state. Most important is
a 'transparent' solution to the bug (plain quitting and restarting, no menu to
invoke, no bookmarking tab groups for every window).
That would be session management, probably.
I found bug #93789 regarding X11 session management, and it indeeds points here.
So I guess those things are related.

However the proposed solution should be cross-platform, not X11 only.
Manually saving session information is bug 63094.

See meta bug 19454.
Flags: blocking1.7a?
Summary: Autosave URIs of open windows → Autosave URIs of open windows and tabs for later restoration in event of crash
blocking? you're kidding. there's no way this should block a release.
Assignee: trudelle → nobody
The browser pages need to be saved not only in the event of crash, but also for
future browsring this group of pages, when creating a group does not justify it.
Just like having the feature of last page open, restored if the browser is
exited,  we need to have a group of pages restored in the event the browser is
exited.
We're not likely to hold 1.7 alpha for this enhancement. 
Flags: blocking1.7a? → blocking1.7a-
*** Bug 232994 has been marked as a duplicate of this bug. ***
(In reply to comment #99)
> We're not likely to hold 1.7 alpha for this enhancement. 

Didn't figure you (all) would since it didn't hold any other release
prior (since 2000) to this either. Since no work is apparently ever
going to be done on this, perhaps it's just time to give up hope that
setting flags in bugzilla might get this ~four year old NEW bug fixed.

For any Mac OS X users out there, OmniWeb 5 is available and contains
saved state as one of its major features. Read about workspaces in the
review; even given quibbles concerning current implementation, this far
exceeds anything currently available (or possibly ever will be) in Mozilla.
Imagine not having to remember which 25 pages you had open when it crashed,
nor having to hope your kids couldn't hear when the expletives spewed...

<http://www.arstechnica.com/reviews/004/software/mac/omniweb/ow5-4.html>
I wouldn't jump to the conclusion that no one will ever fix this. Right now
Mozilla is going through a major foundation and we need to get Firebird so it
replaces Seamonkey, stabilize code, and start thinking about freezing APIs for 2.0

Following all that, you'll probably see another massive influx of
experimentation and changes just like after 1.0

I wouldn't have such a gloomy outlook.
Is there more to this bug at this point than getting Total Recall bundled,
probably with its own Preferences section?  
Then some decision as to whether it's on or off by default?
*** Bug 236054 has been marked as a duplicate of this bug. ***
Whiteboard: parity-opera
There is also a Firefox extension. 

http://members.lycos.nl/mozillafirefox/forums/index.php?showtopic=166&hl=saver

We don't want to just bundle an extension. We would like, however, to integrate
their code into Mozilla so it fits naturally.
In the event of "switch profile" the URI's should be saved as well in order to
restore the workspace.
*** Bug 245512 has been marked as a duplicate of this bug. ***
*** Bug 250853 has been marked as a duplicate of this bug. ***
Product: Core → Mozilla Application Suite
*** Bug 272036 has been marked as a duplicate of this bug. ***
*** Bug 283706 has been marked as a duplicate of this bug. ***
(In reply to comment #110)
> *** Bug 283706 has been marked as a duplicate of this bug. ***

You would think after the dozens and dozens of duplicate requests to fix this
somebody would actually get around to doing it. I looked through most of the
posts and noticed a good chunk of them are just there saying "Bug X is a
duplicate of this bug." 

Why does nobody want to fix this after 5 years? It seems like a huge feature
that a lot of people want in Mozilla's core that competition already offers. Is
the code for something like this just that hard to add in?

Nick
no. why don't you do it?
Assignee: nobody → nickhbo
(In reply to comment #112)
> no. why don't you do it?

Because I am the end user of the product, not the designer or a programmer for
the project. I am merely expressing my opinions as to what would make the
product, Firefox/Mozilla core, better. It seems as though a large number of
people have reported this request and that means there are hundreds, maybe
thousands, more who haven't reported the request but desire the feature.

The definition of an end user is as follows: "The ultimate consumer of a
product, especially the one for whom the product has been designed." Open source
project or not, saying "fix/make it yourself" to someone who doesn't work on the
project is quite silly in my opinion. If I were able/willing to fix it myself,
why would I report it here as a request to the project members?

Nick
He was merely saying this, because you appeared to "blame" the developers for
not putting in the fix imho. And in open source, free as in speech and in beer,
the developers are not "responsible" for fixing it to the users, since what they
are getting from it is not substantial. 

I have always been confused about "open source" in general -- and the concept
doesnt really make sense to me. In fact, the supporters of "open source" seem to
be nothing more than asking "Give It To Me For Free!!!", something like
"demanding a Good Sale/Discount Deal" on the software. 

(Sorry for ranting on a bug....if you need to reply to me, plz email me personally.)
sorry about the spam, but i want to clarify something:

most people here already have some kind of soluctions, what they want its
 to make mozilla and firefox better, not nag the dev team just for fun...

timeless: 
the main problem is that there is already support for this, recall (dont 
know if still works) and session saver (that i use in firefox and works 
fine, better than recall, and works in mozilla also)

what is need is to include this as default!!

if people dont want to be able to save sessions, at least crash recover...
i know no one that after a browser crash didnt want to recover the 
open pages... a simples question restarting the browser asking if they 
want they last state restore would allow a exit on saved "crash all the 
time" page
a simple warn "some data may have been lost"; would make invalid 
any bug about not restoring form state, in page locations, whatever...

its also small, the full session saver is 53Kb xpi, the lite was about 
20Kb IIRC

mozilla and firefox install the quality feedback, so why not include 
session saver lite (the one that only restores crashs)? those that dont
 want it could disable in the same way as the quality feedback
i guess that most of the users would prefer crash recover than the 
quality feedback
i already use session saver, but most of the mozilla and firefox users 
dont know about it, that is why this should be on the default install

if mozilla team want firefox with at least 10% of the web browser share,
this would help  for sure, if not for anything, at least it would stop 
people going back to IE to lunch one (new) process for each page to
stop losing work

its not easy for the marketing people to lose new users that were so
hard to get  (sometimes with great personal/social/professional 
expense)  just beause some plugin crash and took away all browser
windows

finally the "Assigned To:" should go back to something more userfull, as nick will not work on it
Assignee: nickhbo → nobody
No longer blocks: majorbugs
Note that there's a firefox extension to do this called Session Saver:
https://addons.mozilla.org/extensions/moreinfo.php?id=436
If there's a working implementation it should be integrated in the default
installation, this is not a feature that only a restricted group of people needs.
I also support the idea that this feature should be integrated into
the core rather than left to extensions like SessionSaver (Firefox)
and Total Recall (Mozilla Suite).  See my comments at bug 159357
comment 64 regarding why having this in a separate extension like
SessionSaver is not the right solution.
SessionSaver supports both Firefox and SeaMonkey / Mozilla Suite.

More information about this extension see the mozillazine knowledgebase:
http://kb.mozillazine.org/SessionSaver

To download the latest version and to discuss bugs see the following thread:
http://forums.mozillazine.org/viewtopic.php?t=47184

Prog.
Whiteboard: parity-opera → parity-opera | Workaround in comment #119
Recent versions of Session Saver work pretty well in Firefox, but do NOT work properly in Mozilla Suite.  I reported some of the bugs I've been seeing about a month ago:

http://forums.mozillazine.org/viewtopic.php?t=47184&postdays=0&postorder=asc&postsperpage=15&start=1800#1965481

but was never acknowledged.  

As for Total Recall, it has not been updated in years and does not work with current versions of Mozilla.

Therefore, there is no working workaround, and yes, this really needs to get implemented in core browser code.  (Someone with the power to do so, please remove the misleading "Workaround in comment #119" from Status Whiteboard.)
I use Session Saver 2 in Firefox 1.5, it's great.  I'm not just fanboying here, I am throwing weight behind the idea of supporting SS2 in Fx2, folding it into the default UI even.

/be
> I wrote the Tab Session Manager for MultiZilla long before others even had 
> tabs..
Do u know the date or year? Please. :-)

MultiZilla's session-manager is fine, but somewhen is little buggy for me, because I'm not a normal user .-)

When I use SeaMonkey without MultiZilla I save sessions to Bookmarks (group of tabs)..little tricky, but very useful.

The saving session is important and other developers (K-Meleon, Opera..) know this. (Thank HJ!)

SessionSaver has nice feature..after crash it saves data from forms!
While starting on crash recovery, the ability to define and save multiple sessions is extremely useful and desirable.  See also 63094 which seems to be incorporated in the expanded 'plans' for this bug fix/enhancement.

I am out of the loop it seems and have just became aware of SessionSaver last week after a reference to it was posted on 63094.  It is now a 'can't live without' feature extension for me.  I have since become aware of another extension called Tab Mix Plus which I think also has related features, but am not so familiar with it.  From my short experience, the feature set of SessionSaver would seem very desirable in guiding any implementation of the needed crash-recovery and session saving.

Privacy can be an issue.  Saving the detailed session includes elements of history, bookmarks. cookies, typed form information, etc.. and all of these have purge functions to erase tracks already implemented.  The concept of saving session infos would seem to conflict with the desire for privacy.  But need there be such a conflict?  What about encrypting the saved session info, so that the files containing the data are not 'in plain language', readable in notepad.  After a crash, perhaps a password would be required to re-open the last known session.  Without the password, it opens to  about:blank.
(In reply to comment #122)

> You may not like it, but the fact is that I wrote the Tab Session
> Manager for MultiZilla long before others even had tabs, ...

Sounds great!  Can you then help in getting this functionality into
some software component supported by Mozilla.Org for general use in
Mozilla-based applications?

> Now a question: "Why should extension authors put efforts into
> extensions if they will taken over by Mozilla eventually?".
> Wouldn't that effectively eliminate the need of extensions?

Extensions are a testing ground for ideas and also provide flexibility
for special needs.  If there are one or more extensions that provide a
particular feature that would be a significant advantage to 90% or
more of the users, then it is time to incorporate into the main
product line the ideas that have been tested in the extension.

I think in many cases the ultimate ambition of an extension author can
be for the extension to become a supported component of the
mainstream.

If this is done well, it will produce a reusable component that can be
used in Firefox, Seamonkey, Thunderbird, etc.

The ability to restore application state after a shutdown (whether due
to a crash or other reason) is of general use to nearly everyone.  If
it provides robustness in the face of other bugs (e.g., Firefox
crashes but less than a minute later you're back browsing exactly as
before), then this only makes it more important.
Hmm sounds like a dream :)

So why Firefox has not e.g. Mouse Gesture?

Firefox is like BeOS (and clons)..long way. That's all.
Note that this is being considered for Firefox currently (blocking&#8209;firefox2 has been granted).  See bug 342701.
Oops -- I confused this bug with another.  This is about crash recovery.  Sorry.
isn't this now done in current Trunk (using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070623 ID:2007062304)
have a look at §History/Restore from last session" and the already for Firefox 2 introduced Session Management even in a case of crashing ?
Yes, isn't this feature in place now on the trunk? And there is also now the option to save all your open tabs on shut down as well.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Reopening. The feature is not in SeaMonkey yet. Note this bug is filed against the Suite not Firefox.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 17 years ago16 years ago
Resolution: --- → DUPLICATE
This one is much older and has more info. REOPENing.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: Autosave URIs of open windows and tabs for later restoration in event of crash → Autosave URIs of open windows and tabs for later restoration in event of crash (session restore)
This is my first patch, don't blame soo much. It almost working, so can be reviewed. Some notable glitches - extra about:blank on restored session (should be something related with browser startup preference), and no preference for session restore.
Attachment #319287 - Flags: review?(kairo)
Comment on attachment 319287 [details] [diff] [review]
ports sessionstore from firefox, saves and restores session


>Index: suite/browser/navigator.js
.
.
.
> 
>@@ -1268,32 +1294,23 @@ function BrowserOpenTab()
>         default:
>           uriToLoad = "about:blank";
>           break;
>         case 1:
>           uriToLoad = pref.getComplexValue("browser.startup.homepage",
>                                            Components.interfaces.nsIPrefLocalizedString).data;
>           break;
>         case 2:
>-          uriToLoad = gBrowser ? getWebNavigation().currentURI.spec
>-                               : Components.classes["@mozilla.org/browser/global-history;2"]
>-                                           .getService(Components.interfaces.nsIBrowserHistory)
>-                                           .lastPageVisited;
>+          uriToLoad = getWebNavigation().currentURI.spec;
>           break;
>       }
>     } catch(e) {
>       uriToLoad = "about:blank";
>     }
> 
>-    // Open a new window if someone requests a new tab when no browser window is open
>-    if (!gBrowser) {
>-      openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no", uriToLoad);
>-      return;
>-    }
>-
>     gBrowser.selectedTab = gBrowser.addTab(uriToLoad);
>     var navBar = document.getElementById("nav-bar");
>     if (uriToLoad == "about:blank" && !navBar.hidden && window.locationbar.visible)

Not a big deal right now, but you might want to do a new diff later on so you don't accidentally backout bug 429926 :-)
http://oreilly.wirelessdevnet.com/pub/a/mozilla/2000/09/29/keys.html
This URL does not currently exist so removing.
Assignee: nobody → misak
Status: REOPENED → NEW
Keywords: helpwanted
Whiteboard: parity-opera | Workaround in comment #119 → Firefox-parity | Workaround in comment #119
Attachment #319287 - Attachment is obsolete: true
Attachment #319287 - Flags: review?(kairo)
Attachment #319341 - Flags: review?(kairo)
Attachment #319341 - Attachment is obsolete: true
Attachment #319367 - Flags: review?(neil)
Attachment #319341 - Flags: review?(kairo)
Comment on attachment 319367 [details] [diff] [review]
better patch, eliminates extra abot:blank , but fires error on console

Hmmm, I do see you calling nsISessionStore here, but I don't see you porting the actual component from
mozilla/browser/components/sessionstore/

Am I just missing something?
Attached patch version 0.6, includes all files except tests (obsolete) — — Splinter Review
Sorry, i'm soo newbie in mozilla and cvs yet, correct diff is this one.
Attachment #319367 - Attachment is obsolete: true
Attachment #319402 - Flags: review?(neil)
Attachment #319367 - Flags: review?(neil)
I'm currently maintaining the Session Manager extension which uses SessionStore in Firefox.  I got a heads up on this from Philip Chee so I'm CCing myself.    Assuming that the nsSessionStore component is ported over it will be simple to get Session Manager working with whichever browser you're adding it to (SeaMonkey?).

I'm using sessionstore enabled Seamonkey for a while without a problem and tried to install "Session Manager" using Philip Chee suggestions on the forum. It installs without errors, but i didn't get any UI, only preferences from addon manager are accessible.
(In reply to comment #145)
> I'm using sessionstore enabled Seamonkey for a while without a problem and
> tried to install "Session Manager" using Philip Chee suggestions on the forum.
> It installs without errors, but i didn't get any UI, only preferences from
> addon manager are accessible.
> 

That's the standard location (provided by the addons manager) for an add-on's preferences; anything else (such as a menuitem under Tools) is extra (and provided by the extension). If the Preferences button isn't greyed-out when you select the add-ons in the add-ons manager, you can't say there isn't "any UI"; there just isn't any duplicate access to the UI.

Or did you mean the only not-greyed-out buttons there were "Disable" and "Uninstall"?
Tony: <http://sessionmanager.mozdev.org/screenshots.html>

Bascially SeaMonkey's "Tools" menu (i.e.menupopup) has a different id so two things need to be done:

1. Overlay navigator.xul instead of browser.xul;
2. change "menu_ToolsPopup" -> "taskPopup" in the session manager overlay;
(In reply to comment #147)
> Bascially SeaMonkey's "Tools" menu (i.e.menupopup) has a different id so two
> things need to be done:
> 
> 1. Overlay navigator.xul instead of browser.xul;
> 2. change "menu_ToolsPopup" -> "taskPopup" in the session manager overlay;
> 

Session Manager used to support SeaMonkey (using the Crash Recovery extension) back in version 4.  The latest version of Session Manager kept all the old SeaMonkey code (though it wasn't updated) so Session Manager should work in SeaMonkey without modification (assuming it can find the nsSessionStore component).

I did find that the latest version of Session Manager there are typos in the locale's content.rdf files which prevented installation in SeaMonkey 1.1.9, but after I corrected that I could see all the Session Manager menus (though nothing worked obviously).  To get Session Manager to display in SeaMonkey 2.0a I needed to add a line to the chrome.manifest.  Of course without the nsSessionStore component, nothings works.

If I can manage to get SeaMonkey compiled I can test out and see if things work.  In the meantime if anyone wants to play around with what I have so far (it may or may not work), you can grab a version that installs at:
http://downloads.mozdev.org/sessionmanager/session_manager-0.6.1.13-seamonkey.xpi
I am using SeaMonkey 1.1.8 and got an download error.
First you should upgrade to 1.1.9 (for security purposes).  Second, try again.  I just tried and it worked now, so i might not have finished replicating.
And third "Of course without the nsSessionStore component, nothings works."

This is a WIP patch that hasn't been reviewed or checked in to the CVS. And even then it will not be backported to SeaMonkey 1.1.x (unless an extension author does it).
I've got it installed on my Seamonkey with patch applied and it get menu in Tools, but nothing works. When i click on on "Save Session..." for example, nothing happens. Preferences also displaying correctly. It seems something also should be tweaked.
OK, I figured it out. You need @mozilla.org/suite/sessionstore instead of @mozilla.org/browser/sessionstore in sessionmanager.js line 3. Now i have working SessionManager.
One correction, closed tabs list is not working now, because suite uses it's
own undoclosetab implementation, and it not yet integrated with sessionstore.
That's why i'm getting 

Error: [Exception... "'TypeError: No JSON representation for this object!' when
calling method: [nsISessionStore::getClosedTabData]"  nsresult: "0x8057001c
(NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "JS frame ::
chrome://sessionmanager/content/sessionmanager.js :: anonymous :: line 761" 
data: no]
Source File: chrome://sessionmanager/content/sessionmanager.js
Line: 761

This part needs some rework to integrate undoclosetab in sessionstore.
I updated the Session Manager link above to work correctly based on comment #153.     It will install in both Firefox and SeaMonkey, but will uninstall itself if it can't find the SessionStore component.

As for the undoclosetab stuff, the patch looks like it includes the getClosedTabData method from SessionStore so it chould work, but it looks like the closed tab data is not being initiated correctly nor is it populated when tabs are closed.  I guess the question is, will this be fixed to make it work with the suite's current implementation or is it going to be tied off?
> I guess the question is, will this be fixed to make it work
> with the suite's current implementation or is it going to be tied off?

It's up to Misak, but I guess initially we could do NS_ERROR_NOT_IMPLEMENTED or just return a null object until we figure out what to do.
in reply to comment #155
You have extra space in sessinmanager.js, which prevents installing on sessionstore enabled Seamonkey. After correction it install normally. Also I've contacted with seamonkey undoclosetab implementer, i think he can show the way to provide full sessionstore API functionality. 
I'm trying to get working sessionstore API, but it seems i've missed something in this patch - all API functions that should return JSON encoded strings are throwing exception "TypeError: No JSON representation for this object!' when calling method: ". Same code works in Firefox. So if anybody know details of JSON implementation differences in Seamonkey and Firefox, or extension developer, which uses sessionstore, or just a guy with better knowledge than my very basic one in mozilla development - please help and provide as much detail as you can.
JSON is documented at http://www.json.org/

There is also a javascript specific example and parser on that site.  That parser is actually included in the trunk and is required for sessionstore to work.  The following two files are required.  I don't know if they are built into SeaMonkey or not.

http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/loader/JSON.jsm
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/loader/XPCOMUtils.jsm


You can test JSON in SeaMonkey by running the tests located at:

http://lxr.mozilla.org/seamonkey/source/dom/src/json/test/unit/
If SeaMonkey is not building or not packaging the JSON module, please file a separate bug on that, possibly blocking this one. We really want to have the JSON module included.
JSON, is included for sure, just because it throws exception. Otherwise i'll get other errors - not defined or something like that. The problem is that JSON don't like sessionstore component objects. Again, same code in Firefox works.
Comment on attachment 319402 [details] [diff] [review]
version 0.6, includes all files except tests

>Index: suite/browser/browser-prefs.js
>+pref("browser.startup.page",                3);

"browser.startup.page = 3" means always restore.  I think we want to leave the default at 1 (which is already set earlier in this file).  Also, because the pref UI isn't updated, it throws NS_ASSERTIONS and shows "blank page" as the page shown at browser startup.

>Index: suite/browser/navigator.js
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+const Cr = Components.results;
>+const Cu = Components.utils;

I doubt you'll get these past Neil.  :)  You never even use Cr or Cu.  Just use the full Components.foo in the different places.

>+  // this part of code should be settled when shell service will be ported,
>+  // leaving it there for reference. got from firefox browser.js

What does this mean for now?  What session store functionality is broken?

>Index: suite/browser/nsBrowserContentHandler.js
>     switch (prefs.getIntPref("browser.startup.page")) {

>+    case 3:
>+      // this fires error on console but works, don't know how to deal with it yet
>+      return null;

I'm not seeing any errors in the console, but this does cause a few problems.  This is called
1) at startup, either with no flags or with the -browser flag
2) when someone invokes seamonkey when one instance is already running, with no flags, with the -browser flag or with -remote "xfedocommand(openbrowser)"

For case #1, returning null results in a window with no tabs, and then session store restores the tabs
For case #2, returning null still results in a window with no tabs, but there's no session store data, so you just get a blank tab.  With sessionstore, it seems navigator.js needs to handle cases #1 and #2 differently

>Index: suite/browser/tabbrowser.xml
>+             // Session store
>+            if (gPrefService.getBoolPref("browser.sessionstore.enabled")) {
>+              this.mUndoCloseTabMenuItem.hidden =

As discussed on IRC, undo closed tabs functionality shouldn't change (it shouldn't switch to using to using sessionstore), so this whole change to tabbrowser.xml should not be needed.

This would also mean that the "browser.sessionstore.max_tabs_undo" pref would be unneeded.

>Index: suite/common/src/nsSessionStartup.js
>Index: suite/common/src/nsSessionStore.js
>+  onTabClose: function sss_onTabClose(aWindow, aTab) {

>+    // store closed-tab data for undo
>+    if (tabState.entries.length > 1 || tabState.entries[0].url != "about:blank") {
>+      this._windows[aWindow.__SSi]._closedTabs.unshift({
>+        state: tabState,
>+        title: aTab.getAttribute("label"),
>+        image: aTab.getAttribute("image"),
>+        pos: aTab._tPos

This is the reason for the JSON failure.  JSON does not handle undefined and aTab._tPos is undefined.  Our tabs do not have a _tPos property.  When it tries to serialize closeTabs, it fails.  Practically speaking, since you don't need to be keeping the closedTabs around, this problem will take care of itself.  If you want to still fully implement the sessionstore API, you'll need to get a position, but we don't remember the closed tab's position (we always restore it at the end).  So you could just pretend that all the closed tabs were the last one.

>Index: suite/common/src/nsSuiteGlue.js

>+  _onQuitRequest: function(aCancelQuit, aQuitType)

This method never gets called, so I never get the dialog to ask if I want to save, but I still get the close tabs warning dialog.  Sessionstore still works because the pref is set to 3.
Attachment #319402 - Flags: review-
Ok, this patch make API really work. It has some problems, but it almost working. The undoclosetab API function call suite own restoretab method(it's better that ff's one).
To sessionmanager developer: undoclosetab function didn't work. It fires "Error: undoCloseTab is not defined
Source File: chrome://sessionmanager/content/sessionmanager.js
Line: 139". I don't why it doing this, will try to figure out later. But sessinmanager now correctly displays closed tabs in menu. All thing seems to work.
The patch is safe enough to use it on test environment, so all interrested people can apply it and test their extensions and main functionality and report problems if they have time.
Attachment #319402 - Attachment is obsolete: true
Attachment #321266 - Flags: review?(ajschult)
Attachment #319402 - Flags: review?(neil)
I'm going to need to make changes to Session Manager to get undoCloseTab to work correctly.  Currently Session Manager calls the undoCloseTab function in browser.js (not the one in the SessionStore component) because it provides some existing functionality.  

Since SeaMonkey doesn't have an undoCloseTab function in browser.js, an error occurs.  I'm going to need to make a number of changes on my end to handle other discrepancies from using the built in restoretab method (namely different preferences, variables, etc) so I'll fix this on my end.  


There is still a few issues though.  If you want this to be "correct", you'll need to modify any function that references this._windows[aWindow.__SSi]._closedTabs or aWindow.__SS_dyingCache._closedTabs to call browser.getUndoList() instead since the _closedTab list isn't maintained in SeaMonkey.

In addition, you should probably modify the onTabClose function to never actually store the close tabs since that's just a waste of memory as the _closedTabs array isn't used.


Currently my extension call getCloseTabCount() which will currently never be correct in SeaMonkey.  I can implement my own call to browser.getUndoList() to count the closed tabs in SeaMonkey, but it would be nice if the sessionstore component in SeaMonkey could do it since I'm trying to keep the required browser specific code to a minimum.
> In addition, you should probably modify the onTabClose function to never

Is there any reason to have onTabClose at all?

> Currently my extension call getCloseTabCount() which will currently never be
> correct in SeaMonkey.

wouldn't it be correct to just return getUndoList().length ?
(In reply to comment #165)
> There is still a few issues though.  If you want this to be "correct", you'll
> need to modify any function that references
> this._windows[aWindow.__SSi]._closedTabs or aWindow.__SS_dyingCache._closedTabs
> to call browser.getUndoList() instead since the _closedTab list isn't
> maintained in SeaMonkey.
> 
> In addition, you should probably modify the onTabClose function to never
> actually store the close tabs since that's just a waste of memory as the
> _closedTabs array isn't used.

As i understand the code, sessionstore maintains _closedTab list. As discussed
in IRC with Andrew Schultz, CTho and KaiRo suite will maintain both _closedTab
and savedBrowser lists in parallel. The memory footprint shouldn't be
noticeable. Of course, it's not an elegant solution, in future I'll modify code
to get rid of _closedTabs, when will have enough knowledge in mozilla code.
Anyway i think you should use "legal" API calls and expect thet they should
work as expected.
 
> Currently my extension call getCloseTabCount() which will currently never be
> correct in SeaMonkey.

Why ?

(In reply to comment #167)
> > Currently my extension call getCloseTabCount() which will currently never be
> > correct in SeaMonkey.
> 
> Why ?
> 

Because while the code to add closed tabs to the _closedTabs array exists, the code to remove them is no longer there.  

There's two problems:

#1. You removed the line from undoCloseTab which removes the restored tab from the _closedTab array so currently that array won't match the savedBrowser list after a tab is restored.  The line that did that was:

      var closedTab = closedTabs.splice(aIndex, 1).shift();

You can change this to simply:

      closedTabs.splice(aIndex, 1);


#2. The undoCloseTab function in sessionstore is not called by SeaMonkey when restoring a tab so even if you fix problem #1 above, it still wouldn't work.

In Firefox whenever a tab is restored, the undoCloseTab function in browser.js executes, which then calls the SessionStore.undoCloseTab function. That's how the restored tabs get removed from the _closedTabs array (and how Firefox restores the tab).

You could call the Sessionstore undoCloseTab function from browser.removeTab(), but the way undoCloseTab is currently written, that would create an infinite loop.  
I was thinking that onTabOpen and onTabClose event listeners should catch and update _closedTabs on these events. Do they ?
onTabClose does update _closedTabs when a tab closes.  The onTabOpen event triggers any time a tab is open (not just when one is restored).  It currently calls the onTabAdd procedure which doesn't open _closedTabs.

The only time _closedTabs every had a tab removed was on a call to the undoCloseTabs.  This made sense since that was the only way to restore a closed tab.
OK, i understand. This patch adds extra updateClosedTabList() call to API, which is not recommended to use by developers - it should be called only by restoreTab. Now getCloseTabCount() should work as expected.
Attachment #321266 - Attachment is obsolete: true
Attachment #321321 - Flags: review?(ajschult)
Attachment #321266 - Flags: review?(ajschult)
> As discussed in IRC with Andrew Schultz, CTho and KaiRo suite will maintain both > _closedTab and savedBrowser lists in parallel.

Not me... saving parallel lists can only cause pain.  The relevant data you need should be in savedBrowsers.
Ok, this patch uses savedBrowsers, API should work, patch should work in general. Michael, can you test it ? My sessionmanager seems to work with it fairly well.

This patch has very bad and dirty code, but i'll be very grateful if ajschult review it, just to know that it's going to right direction.

Notable glitches:

Closed tabs list does not go to right place when reading session state from file.
There should be more things to clean.
Attachment #321321 - Attachment is obsolete: true
Attachment #321968 - Flags: review?(ajschult)
Attachment #321321 - Flags: review?(ajschult)
Comment on attachment 321968 [details] [diff] [review]
removed updateClosedTabList(), removed _closedTabs, using savedBrowsers as suggested

With the patch applied, the behavior I see if I crash is that sessionstore kicks in -- the window is restored with the tabs I had, but it also opens the home page in an additional tab (which we don't really want).  If I have more than one window open, an additional window is restored, but the first tab of that window is replaced with my home page.

>Index: suite/browser/nsBrowserContentHandler.js
>     switch (prefs.getIntPref("browser.startup.page")) {

>+
>+    case 3:
>+      return null;

Hmm.  Is this what we want?  I suspect that this results in behavior that we want for case 3, but it doesn't seem like the right fix.  Doesn't firefox return the same for 1 and 3?  I suspect that restore-after-crash might behave better if we had a better fix for this.

>Index: suite/browser/tabbrowser.xml
>             if (aIndex >= this.savedBrowsers.length || aIndex < 0)
>               return;
>-
>+            

oops.  This line was happy as it was.

>Index: suite/common/src/nsSessionStartup.js
>+# ***** BEGIN LICENSE BLOCK *****

You'll want to format the license block like we do for other .js files in suite/.  (no "#")

>+          this._lastSessionCrashed =
>+            initialState.session && initialState.session.state &&
>+            initialState.session.state == STATE_RUNNING_STR;

_lastSessionCrashed doesn't need to be a property of |this|.  It's only used in this function.

>+        // invalid .INI file - nothing can be restored
>+        }
>+        catch (ex) { debug("The session file is invalid: " + ex); } 

The comment above is totally in the wrong place... it should be in the catch block.  This should also set _iniString to null to prevent the rest of the code from having any effect (and so that the state getter below returns null).  Also, it looks like the evalInSandbox is the only part that would throw an exception.  I don't see why the rest of it needs to be in a try block (you'd have to declare initialState before the try block so you could use it after the try block).

>+    // prompt and check prefs
>+    if (this._iniString) {

This block can go up somewhere in the above if blocks, since you already know that this._iniString is not null up there.

>+    if (this._prefBranch.getBoolPref("sessionstore.resume_session_once")) {
>+      this._prefBranch.setBoolPref("sessionstore.resume_session_once", false);
>+    }

This is the only thing that still needs to happen if you hit the catch block above for an invalid file.  If you move this part up to the beginning, you could return from the catch block, which would simplify the code flow a lot.  That'd break _doResumeSession, so you'd want to perhaps have a doResumeSession variable that's equal to what _doResumeSession returns.  That'd save you from calling into the pref service repeatedly as well.  Moving this code to the top would also unset the pref when sessionstore is disabled, which also seems like a good idea.

>+     * Since we're garanteed to be at least the second caller of defaultArgs
>+     * (nsBrowserContentHandler calls it to determine which arguments to pass

Our nsBrowserContentHandler doesn't have defaultArgs.  I suspect this is why restore-after-crash isn't happy and why case 3 above needed to return null to get the desired behavior.

>+      var cvstream = Cc["@mozilla.org/intl/converter-input-stream;1"].

cvstream = "converter stream"?  I guess name this cvStream.

>+    catch (ex) { } // inexisting file?

"nonexistent".  Practically speaking, this method should only be called if the file exists, so I'd guess that a more likely cause would be a permission problem or perhaps a sufficiently corrupted file.
Attachment #321968 - Flags: review?(ajschult) → review-
this patch will work as it should. 

The sessionstore.js file will be the most hard part, i think i should have many errors here. Will be happy if someone else have a look to my code.
Attachment #321968 - Attachment is obsolete: true
Attachment #322252 - Flags: review?(ajschult)
Comment on attachment 322252 [details] [diff] [review]
addresses all ajschult comments, now restores multiple windows correctly

Sorry, this patch also didn't do its job well, new one is coming.
Attachment #322252 - Attachment is obsolete: true
Attachment #322252 - Flags: review?(ajschult)
Attached patch Correctly restores multiple windows. (obsolete) — — Splinter Review
Here are the new patch. I've added new read only attribute to API, it should be useful. The boolean restoreInProgress is true if sessionstore session restoration in progress. I'm planning to backport it to ff later, if reviewers agree that it's a good addition.
Attachment #322626 - Flags: review?(ajschult)
Comment on attachment 322626 [details] [diff] [review]
Correctly restores multiple windows.

Behavior I see with this patch and browser.startup.page=3 is that the last tab in the window is replaced with my homepage.  If I crash, it restores all the tabs, but also loads my homepage in an extra tab.


>Index: suite/browser/navigator.js
>+  // initialize the session-restore service (in case it's not already running)
>+  if (document.documentElement.getAttribute("windowtype") == "navigator:browser") {

Startup only gets called for windows of type navigator:browser, so you don't need this.

>+    try {
>+      var ss = Components.classes["@mozilla.org/suite/sessionstore;1"].
>+               getService(Components.interfaces.nsISessionStore);
>+      ss.init(window);
>+      var isRestoring = ss.restoreInProgress;

you need to declare (and initialize) isRestoring before the try block.  Also, restoreInProgress is false for me even when session store is going to restore the window.  This is probably causing the problems I was seeing.

>+      dump("restoring "+ isRestoring+ "\n");

dump here is OK for debugging, but remember to remove for the final patch.

>Index: suite/common/src/nsSessionStartup.js
>+    doResumeSession = this._doResumeSession();

Right.  And you're only using this here, so you can just inline the function.

>+        // invalid .INI file - nothing can be restored
>+        catch (ex) {
>+          this._iniString = null;

Right.  Move the comment into the catch block (below the catch line)

>+      // prompt and check prefs
>+        if (lastSessionCrashed && this._doRecoverSession())

nit: indentation of the comment

>+    // Note: Seamonkey doesn't have defaultArgs. do we need this ?

actually, no.  Our deafult arguments are nothing.  See http://mxr.mozilla.org/seamonkey/source/suite/common/tasksOverlay.js#151

The code in navigator.js (with some tweaking) should be able to handle this properly.
Attachment #322626 - Flags: review?(ajschult) → review-
Attached patch Addressed all comments above. (obsolete) — — Splinter Review
This one should correctly restore windows in any case, but IMHO there will be prettier way to do this. For unknown reason the first window handles differently, so I'm forced to add check for thins in nsBrowserContentHandler.js. Also restoreInProgress code is ugly yet and have lag (it has delay when it's still true, but all windows are restored). I need suggestions there.
Attachment #322626 - Attachment is obsolete: true
Attachment #322770 - Flags: review?(ajschult)
Comment on attachment 322770 [details] [diff] [review]
Addressed all comments above.

>+//  if (isRestoring) {
>+//    window.arguments = [];
>+//  }

you can just remove that

>-      } else {
>+      } else if (!isRestoring) {

this works, but only because navigator.js throws an exception later:

JavaScript error: chrome://navigator/content/navigator.js, line 674: uriArray is undefined

I think you want to turn a rather large chunk of code into an !isRestoring if block:

>  if (!isPageCycling) {
>    var uriToLoad = "";
>
>    if (!isRestoring) {

down to the chunk of code where it's calling loadURI.

>Index: suite/browser/nsBrowserContentHandler.js
>+    case 3:
>+      if (!Components.classes["@mozilla.org/appshell/window-mediator;1"]
>+                     .getService(Components.interfaces.nsIWindowMediator)
>+                     .getMostRecentWindow("navigator:browser")) {
>+        return null;
>+      }
>+      return getHomePageGroup(prefs);

hmm, shouldn't need to do this... just add case 3 up above:

switch
case1:
case3:
  return getHomePage...

>Index: suite/browser/tabbrowser.xml
>-            this.savedBrowsers.unshift({browser: oldBrowser, history: oldSH});
>+            // Sessionstore already unshifted savedBrowsers and inserted tabData
>+            // doing the rest

eek.

>Index: suite/common/src/nsSessionStartup.js
>+    if ((resumeFromCrash || this._doResumeSession) && this._sessionFile.exists()) {

this._doResumeSession is a function and so it's always true.  I think you want

var doResumeSession = this._prefBranch.getIntPref("startup.page") == 3 || 
                      this._prefBranch.getBoolPref("sessionstore.resume_session_once");

and then remove the function entirely.

>+  /**
>+   * Removes the default arguments from the first browser window
>+   * (and removes the "domwindowopened" observer afterwards).
>+   */
>+  _onWindowOpened: function sss_onWindowOpened(aWindow) {

this doesn't actually do anything other than remove itself as an observer (it used to do what's described in the comment).  So you can remove this function and the bit above that's calling into here (and then remove the thing that's causing that to be called, etc).
Attachment #322770 - Flags: review?(ajschult) → review-
>>Index: suite/browser/tabbrowser.xml
>>-            this.savedBrowsers.unshift({browser: oldBrowser, history: oldSH});
>>+            // Sessionstore already unshifted savedBrowsers and inserted tabData
>>+            // doing the rest
>
>eek.

> Index: suite/common/src/nsSessionStore.js
> +  onTabClose: function sss_onTabClose(aWindow, aTab) {
[...]
> +    aWindow.getBrowser().savedBrowsers.unshift({browser: {}, history: {}, tabData: tabsData});

How about leaving tabsData on the |aTab| and then letting the removeTab method in tabbrowser.xml pick up the tabsData e.g.

Index: suite/common/src/nsSessionStore.js
  onTabClose: function sss_onTabClose(aWindow, aTab) {
[...]
    aTab.tabsData = tabsData;
[...]
Index: suite/browser/tabbrowser.xml
            this.savedBrowsers.unshift({browser: oldBrowser, history: oldSH, tabData: aTab.tabsData});

> +    var postdata;
> +    if (aEntry.postdata_b64) {  // Firefox 3
> +      postdata = atob(aEntry.postdata_b64);
> +    } else if (aEntry.postdata) { // Firefox 2
> +      postdata = aEntry.postdata;
> +    }

Um, we aren't going to backport this to branch are we?
(In reply to comment #181)
[...]
> > +    var postdata;
> > +    if (aEntry.postdata_b64) {  // Firefox 3
> > +      postdata = atob(aEntry.postdata_b64);
> > +    } else if (aEntry.postdata) { // Firefox 2
> > +      postdata = aEntry.postdata;
> > +    }
> 
> Um, we aren't going to backport this to branch are we?
> 

and in addition, shouldn't these comments rather mention SeaMonkey 2 and SeaMonkey 1 respectively? This is a Suite bug, and Firefox 2 and higher already has a built-in session saver.
Actually, SeaMonkey 2 now using Firefox 2 postdata type. It's safer not to touch these until SeaMonkey developers decide what to pick.
Attached patch addresses comments #180 and #181 (obsolete) — — Splinter Review
I made all changes suggested, but now have extra blank tab on first window. Should be something related with content handler, but I can't catch it. Anyway asking review, just because there should be more glitches than this one.
Attachment #322770 - Attachment is obsolete: true
Attachment #323699 - Flags: review?(ajschult)
This is the same patch, I've just replaced Cc Cr Ci and cleaned code a bit. Still extra tab on first window, i'm totally lost in navigator and content handler code. sessiosstore.js needs review, sessionstartup.js should be close to final version.
Attachment #323699 - Attachment is obsolete: true
Attachment #324460 - Flags: review?(ajschult)
Attachment #323699 - Flags: review?(ajschult)
Attached patch No extra blank tab, handles all situations (obsolete) — — Splinter Review
Ok, i tweaked navigator.js code a bit, now it correctly restores windows without notable errors. Ajschult should like this, i hope.
Attachment #324460 - Attachment is obsolete: true
Attachment #324612 - Flags: review?(ajschult)
Attachment #324460 - Flags: review?(ajschult)
>Index: suite/browser/tabbrowser.xml
>-            this.savedBrowsers.unshift({browser: oldBrowser, history: oldSH});
>+            this.savedBrowsers.unshift({browser: oldBrowser, history: oldSH, tabData: aTab.tabsData});

Hmm. I just realized that if the sessionstore component doesn't get initialized then |aTab.tabsData| will be undefined. I notice that the initialization code in navigator.js is wrapped in a try/catch block so I think that this may be a possibility.
Something like this ?

try {
  this.savedBrowsers.unshift({browser: oldBrowser, history: oldSH, tabData: aTab.tabsData});
} catch {
  this.savedBrowsers.unshift({browser: oldBrowser, history: oldSH});
}

or just make tabData null? Or replace try/catch block with if ?

BTW i disabled sessionstore and ran seamonkey and undoclosetab capability of tabbrowser.xml is OK. Extensions may have problems if sessionstore didn't get initialized, but they always should check that case.
Sorry for questions like this one, i'm just on first step of learning.
This patch incorporates workaround for comment 187, suggested by Philip Chee and ceorrect handling of restoreinprogress(should be no delay).
Attachment #324612 - Attachment is obsolete: true
Attachment #325952 - Flags: review?(ajschult)
Attachment #324612 - Flags: review?(ajschult)
Comment on attachment 325952 [details] [diff] [review]
correct handling of restoreInProgress, also workaroung for Philip Chee's case

sorry for the slow review.  I'm still seeing the homepage loaded after a crash if I select restore.  So I get the tabs restored plus the homepage.  It seems to work OK with browser.startup.page=3.

>Index: suite/common/src/nsSessionStore.js

>+      else {
>+        // Nothing to restore, notify observers things are complete.
>+      awindow.setTimeout(function() {

oops.  "aWindow"
Also needs extra indenting.  I'll try to get to a more complete review this weekend.
I tweaked logic a bit, now it works in any case of browser.startup.page.
Attachment #325952 - Attachment is obsolete: true
Attachment #326091 - Flags: review?(ajschult)
Attachment #325952 - Flags: review?(ajschult)
Comment on attachment 326091 [details] [diff] [review]
Fixes case above, better logic in content handler.

This works for me now recovering from crashes (yay).  I still need to look through nsSessionStore, but the rest is looking good.  Just a few fixups:

>Index: suite/browser/browser-prefs.js
>   // only load url passed in when we're not page cycling
>   if (!isPageCycling) {
>     var uriToLoad = "";
>-
>     // Check window.arguments[0]. If not null then use it for uriArray

oops.  Just leave these extra lines.

>+      } else uriArray = ["about:blank"];

style nit: put the uriArray assignment on its own line.

>Index: suite/browser/nsBrowserContentHandler.js
>+    var ss = Components.classes["@mozilla.org/suite/sessionstartup;1"]
>+             .getService(Components.interfaces.nsISessionStartup);
>+    haveUpdateSession = ss.doRestore();

you can just do:
if (ss.doRestore())
  return null;

since you only use haveUpdateSession to return below.

>Index: suite/browser/tabbrowser.xml
>-            this.savedBrowsers.unshift({browser: oldBrowser, history: oldSH});
>+            this.savedBrowsers.unshift({browser: oldBrowser, history: oldSH, tabData: aTab.tabsData ? aTab.tabsData : {} });

ya, I like this better (thanks, Philip)

>Index: suite/common/src/nsSessionStartup.js
>+  this._doResumeSession = this._prefBranch.getIntPref("startup.page") == 3 || 
>+      this._prefBranch.getBoolPref("sessionstore.resume_session_once");

right!  But now you can make this into a local variable, since it's only used in this function (and remove it as a member listed above).
Status: NEW → ASSIGNED
Flags: blocking-seamonkey2?
Whiteboard: Firefox-parity | Workaround in comment #119 → [Firefox-parity] [Workaround: Comment 119]
Target Milestone: Future → ---
This is definitely wanted for SeaMonkey 2, but we won't block a release for a feature.
Flags: blocking-seamonkey2? → blocking-seamonkey2-
I've updated patch with ajschult fixups, commented out savedBrowsers restoration code, it will go to separate bug, as disscussed on IRC.
Attachment #326091 - Attachment is obsolete: true
Attachment #327922 - Flags: review?(ajschult)
Attachment #326091 - Flags: review?(ajschult)
Attachment #327922 - Flags: review?(ajschult) → review?(neil)
Attached patch same as above, plus port of bug 343352 to suite (obsolete) — — Splinter Review
I've included fix of bug 343352 in a patch.
Attachment #327922 - Attachment is obsolete: true
Attachment #332522 - Flags: review?(ajschult)
Attachment #327922 - Flags: review?(neil)
Depends on: 449967
Attachment #332522 - Flags: review?(ajschult) → review-
You might also consider porting bug 357419 which just got fixed.
It's already ported, I'm just waiting reviewer comments for previous patch to upload new one.
Comment on attachment 332522 [details] [diff] [review]
same as above, plus port of bug 343352 to suite

I had comments before... not sure where they went.  Hopefully, I remembered all of them.

>+++ b/suite/common/src/nsSessionStore.js	Wed Aug 06 17:46:51 2008 +0500
>+  // During the initial restore tracks the number of windows yet to be restored

Grammar isn't right here.  Perhaps
During the initial restore, the number of windows yet to be restored

>+  // closed tabs list
>+  _tabData: {},

Can you just grab this info from tabbrowser as needed (in _getCurrentState)?  You do this in _collectWindowData.  With that, you could drop _tabData entirely.

>+  // Restore state in progress
>+  _restoreInProgress : false,
>+  
>+  _setRestoreInprogress : false,

What does this variable represent?

>+    if (iniString) {
>+      try {
>+        // parse the session state into JS objects
>+        this._initialState = this._safeEval(iniString);

as in nsSessionStartup, this is the only part of the try block that would throw, except for the delete at the end (which shouldn't be try/caught).

>+        delete this._initialState.windows[0].hidden;

this can be
if (this._initialState.windwos[0])
  delete this._initialState.windows[0].hidden;

>+    // if last session crashed, backup the session
>+    if (lastSessionCrashed) {

this needs to be in the same scope as lastSessionCrashed is declared.
Ok, _tabData is really unnecessary, i've deleted it. This patch also ports fixes for Bug 450595, Bug 409115, Bug 367052, Bug 448741, Bug 450879. No tests yet, i'll include all tests, when get review+ for patch.

> +  _setRestoreInprogress : false,

>What does this variable represent?

This variable is flag to actually set RestoreInprogress to false. This needed to workaround SeaMonkey implementation of navigator.js, which uses extra variables when opening new window.
Attachment #332522 - Attachment is obsolete: true
Attachment #334662 - Flags: review?(ajschult)
Attached patch all above plus ported more bugs (obsolete) — — Splinter Review
ported bugs - Bug 368677, Bug 346337, Bug 452975, Bug 439675, Bug 366572
Attachment #334691 - Attachment is obsolete: true
Attachment #337040 - Flags: review?(ajschult)
Attached patch more bugfixes ports (obsolete) — — Splinter Review
ported bugs - bug 342635, bug 367052, bug 451702, bug 381349
Attachment #337040 - Attachment is obsolete: true
Attachment #337292 - Flags: review?(ajschult)
Attachment #337040 - Flags: review?(ajschult)
Attached patch more bugfixes ports, correct (obsolete) — — Splinter Review
Oops, I forgot to replace CC,CI stuff.
Attachment #337292 - Attachment is obsolete: true
Attachment #337404 - Flags: review?(ajschult)
Attachment #337292 - Flags: review?(ajschult)
Flags: wanted-seamonkey2+
Attached patch bunch of bugfix ports, keeping in sync with ff (obsolete) — — Splinter Review
Many bugs fixed since last patch, so it's time to refresh this patch.

Ported - Bug 355284 ,  Bug 339445,  Bug 456465,  Bug 408470,  Bug 458959,  Bug 458954,  Bug 457195,  Bug 458963,  Bug 456342,  Bug 459041
Attachment #337404 - Attachment is obsolete: true
Attachment #342770 - Flags: review?(ajschult)
Attachment #337404 - Flags: review?(ajschult)
Blocks: 459550
QA Contact: bugzilla → ui-design
Attached patch another bugfix respin (obsolete) — — Splinter Review
This is another bugfix respin of patch to keep in sync with ff.

Ported  Bug 345898, Bug 459906, Bug 460334, Bug 427186, Bug 463206, Bug 46420. Additionally copied some bits of code from private browsing patch, which reorganizes sessionstore's functions.
Attachment #342770 - Attachment is obsolete: true
Attachment #349629 - Flags: review?(ajschult)
Attachment #342770 - Flags: review?(ajschult)
Comment on attachment 349629 [details] [diff] [review]
another bugfix respin

>diff --git a/suite/common/src/nsSessionStartup.js b/suite/common/src/nsSessionStartup.js
>+    try {
>+      // parse the session state into JS objects
>+      var s = new Components.utils.Sandbox("about:blank");
>+      var initialState = Components.utils.evalInSandbox(this._iniString, s);
>+    }

initialState needs to be declared before the "try {"

>+    else if (!lastSessionCrashed && doResumeSession)
>+      this._sessionType = Components.interfaces.nsISessionStartup.RESUME_SESSION;

this shouldn't happen if we failed to get initialState, but it will.  The most straightforward thing to do would be to set doResumeSession to false within the catch block above.

>diff --git a/suite/common/src/nsSessionStore.js b/suite/common/src/nsSessionStore.js
>+      try {
>+        // parse the session state into JS objects
>+        this._initialState = this._safeEval(iniString);
>+      }
>+      catch (ex) { debug("The session file is invalid: " + ex); }
>+      
>+      let lastSessionCrashed =
>+        this._initialState.session && this._initialState.session.state &&
>+        this._initialState.session.state == STATE_RUNNING_STR;

you need to test this._initalState here (like you do in nsSessionStartup), since it will be undefined if _safeEval threw.

>+      // if last session crashed, backup the session
>+      if (lastSessionCrashed) {
>+      }

hmm... something not right here.  Should this just be removed, or is there something that should be in that block?

>+    // attempt to update the current URL we send in a crash report
>+    this._updateCrashReportURL(aWindow);

I'm having trouble imagining why crash report functionality should live in here.  crash reports have almost nothing to do with session restoring.  Shouldn't this live in one of our core (suite) files?

Other than that, things look OK up to onTabLoad
Comment on attachment 349629 [details] [diff] [review]
another bugfix respin

>+  locale/@AB_CD@/communicator/sessionstore.properties                       (%chrome/common/sessionstore.properties)

this file seems to be missing from your patch
Attached patch patch v2 (obsolete) — — Splinter Review
(In reply to comment #207)
> 
> initialState needs to be declared before the "try {"
> 

Fixed.

> 
> this shouldn't happen if we failed to get initialState, but it will.  The most
> straightforward thing to do would be to set doResumeSession to false within the
> catch block above.

Fixed.
 
> 
> you need to test this._initalState here (like you do in nsSessionStartup),
> since it will be undefined if _safeEval threw.
> 

Fixed.

> >+      // if last session crashed, backup the session
> >+      if (lastSessionCrashed) {
> >+      }
> 
> hmm... something not right here.  Should this just be removed, or is there
> something that should be in that block?
> 

Actually, the code in this block is in bug 459550. There is a little mess with all the patches porting for sessionstore, and aboutsessionrestore. I removed this if, will add to bug 459550 patch, also all work porting patches will go to that bug, keeping this in stable state.

> >+    // attempt to update the current URL we send in a crash report
> >+    this._updateCrashReportURL(aWindow);
> 
> I'm having trouble imagining why crash report functionality should live in
> here.  crash reports have almost nothing to do with session restoring. 
> Shouldn't this live in one of our core (suite) files?
> 

This is actually bug 375083, and nobody gives a reason there why this code should go to sessionstore. Give me suggestion where is the best to put this code, I'll do that.

> this file seems to be missing from your patch

Added.
Attachment #349629 - Attachment is obsolete: true
Attachment #354149 - Flags: review?(ajschult)
Attachment #349629 - Flags: review?(ajschult)
Attachment #354149 - Flags: superreview?(neil)
Comment on attachment 354149 [details] [diff] [review]
patch v2

Requesting sr from Neil in parallel to speed up the process of finally getting this into the tree.
In https://wiki.mozilla.org/SeaMonkey:StatusMeetings:2009-01-13#Longer-Term_SeaMonkey_2_Planning we decided we can possibly go with a rs+ from ajschult (and post-facto-review) on the sessionstore backend copied from Firefox, given those already were reviewed once, the parts special to SeaMonkey were already reviewed by ajschult, and the whole patch should get sr from Neil as well.
This also should be done for bug 459550, because these two bugs are very related, also, this patch misses preferences part, which i'll add as soon as i return from Kiev (monday). Also i have tests for sessionstore ready, and Seamonkey passes 90-95% of them, which is a good indicator of patch good shape.
Attached patch patch v2.1 (obsolete) — — Splinter Review
This patch adds preferences part. I'm not sure about text and accesskey, so requesting ui-review too.
Attachment #354149 - Attachment is obsolete: true
Attachment #359048 - Flags: ui-review?(neil)
Attachment #359048 - Flags: superreview?(neil)
Attachment #359048 - Flags: review?(ajschult)
Attachment #354149 - Flags: superreview?(neil)
Attachment #354149 - Flags: review?(ajschult)
With the latest patch applied, how do I actually get my build to restore sessions?
if all is well, you can

1. terminate a process (simulate a crash)
2. set the pref (browser.startup.page) to 3.  the latest patch appears to add appropriate UI for this to the Browser pref pane.
3. choose to save when you quit -- in some cases you won't get prompted (from the patch, if browser.warnOnQuit or browser.tabs.warnOnClose is false)
Attachment #359048 - Flags: superreview?(neil) → superreview+
Comment on attachment 359048 [details] [diff] [review]
patch v2.1

>+      } else if (!isRestoring) {
I think this is the wrong place to put the check, it belongs earlier, probably with the "arguments" in window check (i.e. we don't look for arguments if we're restoring). Also the isRestoring variable should be set nearer this code.

>-
Nit: pointless whitespace changes (Ă—2).

>   try {
>+    var ss = Components.classes["@mozilla.org/suite/sessionstartup;1"]
>+             .getService(Components.interfaces.nsISessionStartup);
>+    // return null if we are restoring previous session
>+    if (ss.doRestore())
>+      return null;
>+  } catch (e) {
>+  }
I'm not sure why this is here. We could be here because someone used -browser, because they have browser checked in the startup preferences, or because they had no command line flags or preferences. Is it always correct to restore?

>+  try {
>     switch (prefs.getIntPref("browser.startup.page")) {
>     case 1:
>+    case 3:
If there is no session to restore then I'd tempted to restore a blank window.

>+            this.savedBrowsers.unshift({browser: oldBrowser, history: oldSH, tabData: aTab.tabsData ? aTab.tabsData : {} });
I think we should rename tabsData to tabData to be consistent. Also, use aTab.tabData || {} if you need to avoid null.

>+function debug(aMsg) {
>+  aMsg = ("SessionStartup: " + aMsg).replace(/\S{80}/g, "$&\n");
Weird. But don't bother with the replace. See below.

>+  Components.classes["@mozilla.org/consoleservice;1"].getService(Components.interfaces.nsIConsoleService)
>+                                     .logStringMessage(aMsg);
Badly wrapped (Ă—few). Should look something like this:
Components.classes["@mozilla.org/consoleservice;1"]
          .getService(Components.interfaces.nsIConsoleService)
          .logStringMessage("SessionStartup: " + aMsg);

>+    this._prefBranch = Components.classes["@mozilla.org/preferences-service;1"].
>+                       getService(Components.interfaces.nsIPrefService).getBranch("browser.");
Correct style in suite is something like this (Ă—lots):
this._prefBranch = Components.classes["@mozilla.org/preferences-service;1"]
                             .getService(Components.interfaces.nsIPrefService)
                             .getBranch("browser.");
>+    if (!resumeFromCrash && !doResumeSession || !sessionFile.exists())
Confusing; please use:
if ((!resumeFromCrash && !doResumeSession) || !sessionFile.exists())

>+    try {
>+      // parse the session state into JS objects
>+      var s = new Components.utils.Sandbox("about:blank");
>+      initialState = Components.utils.evalInSandbox(this._iniString, s);
Isn't that what JSON.jsm is for?

I didn't look too carefully at the new files, although I did try to compare them to Firefox's versions. I also haven't tried this out yet.
Comment on attachment 359048 [details] [diff] [review]
patch v2.1

>+      } else if (!isRestoring) {
Something's not quite right because I get an extra blank tab on the first window of a restored session.

>+<!ENTITY restoreSessionRadio.label      "Show my windows and tabs from last time">
On those platforms that have a shell service (currently only Windows, but you can simulate it by unhiding the groupbox using DOM Inspector), the window is now too cramped. The simplest solution would seem to be to rename this to "Restore Previous Session" (with a changed access key of course.)
Attachment #359048 - Flags: ui-review?(neil) → ui-review-
(In reply to comment #215)
> I'm not sure why this is here. We could be here because someone used -browser,
> because they have browser checked in the startup preferences, or because they
> had no command line flags or preferences. Is it always correct to restore?
> 
As discussed on IRC, logic behind this seems reasonable.

> If there is no session to restore then I'd tempted to restore a blank window.
> 
Fixed.
> I think we should rename tabsData to tabData to be consistent. Also, use
> aTab.tabData || {} if you need to avoid null.
> 
Fixed.
> Weird. But don't bother with the replace. See below.
> 
> Badly wrapped (Ă—few). Should look something like this:
Fixed.
> Correct style in suite is something like this (Ă—lots):
> this._prefBranch = Components.classes["@mozilla.org/preferences-service;1"]
>                              .getService(Components.interfaces.nsIPrefService)
>                              .getBranch("browser.");
Fixed.
> Confusing; please use:
> if ((!resumeFromCrash && !doResumeSession) || !sessionFile.exists())
Fixed. 
> Isn't that what JSON.jsm is for?
> 
Will file followup bug for this. Currently doing what Firefox does.
(In reply to comment #216)
> (From update of attachment 359048 [details] [diff] [review])
> >+      } else if (!isRestoring) {
> Something's not quite right because I get an extra blank tab on the first
> window of a restored session.
> 
I aggree that logic behind isRestoring is weird, but it's only working version for all situations now. Will investigate other possibilities.

> On those platforms that have a shell service (currently only Windows, but you
> can simulate it by unhiding the groupbox using DOM Inspector), the window is
> now too cramped. The simplest solution would seem to be to rename this to
> "Restore Previous Session" (with a changed access key of course.)
Changed as suggested.
Attached patch comments addressed (obsolete) — — Splinter Review
I'm making this patch with old (working) isRestoring logic mainly for KaiRo, to have sessionstore showed at FOSDEM. BTW all comments and nits excluding isRestoring are addressed. Note to all testers - this patch (and other before) will not work if applied without patch for bug 459550. It uses obsoleted and removed json.jsm, which is removed from tree some time ago. Patch for bug 459550 adds fixes to use native JSON.
Attachment #359048 - Attachment is obsolete: true
Attachment #360258 - Flags: ui-review?(neil)
Attachment #360258 - Flags: review?(ajschult)
Attachment #359048 - Flags: review?(ajschult)
Attached patch comments addressed (correct) (obsolete) — — Splinter Review
Oops, sorry :(
Previous patch contains other fixes from other patches. This is correct one.
Attachment #360258 - Attachment is obsolete: true
Attachment #360270 - Flags: ui-review?(neil)
Attachment #360270 - Flags: review?(ajschult)
Attachment #360258 - Flags: ui-review?(neil)
Attachment #360258 - Flags: review?(ajschult)
(In reply to comment #217)
> (In reply to comment #215)
> > If there is no session to restore then I'd tempted to restore a blank window.
> Fixed.
Although getURLToLoad() return about:blank by default, no need to add a case.

> > Badly wrapped (Ă—few). Should look something like this:
> Fixed.
Unfortunately you only changed the version in SessionStore.js, and I was quoting the fix to SessionStartup.js which uses a different string...

> > Correct style in suite is something like this (Ă—lots):
> > this._prefBranch = Components.classes["@mozilla.org/preferences-service;1"]
> >                              .getService(Components.interfaces.nsIPrefService)
> >                              .getBranch("browser.");
> Fixed.
I see it fixed in a few places, but there are still lots more I'd like fixed...

I still need to look into the startup code to see what I can figure out.
Attachment #360270 - Flags: ui-review?(neil) → ui-review-
Comment on attachment 360270 [details] [diff] [review]
comments addressed (correct)

>+  _isCmdLineEmpty: function sss_isCmdLineEmpty(aWindow) {
>+    var defaultArgs = Components.classes["@mozilla.org/commandlinehandler/general-startup;1?type=browser"].
>+                      getService(Components.interfaces.nsICommandLineHandler).defaultArgs;
>+    if (aWindow.arguments && aWindow.arguments[0] &&
>+        aWindow.arguments[0] == defaultArgs)
>+      aWindow.arguments[0] = null;
>+
>+    return !aWindow.arguments || !aWindow.arguments[0];
OK, so your problem is here: we open windows differently to Firefox. In particular, a null or lack of arguments means to use the new window preference. Also, there is no defaultArgs property on our command line handler! So:
return "arguments" in aWindow && aWindow.arguments.length &&
       aWindow.arguments[0] == "about:blank";
is about the closest you can get to "is the window blank".

>+    if (ss.doRestore())
>+      return null;
To make this work, you probably have to return "about:blank" here.

>+    argString.data = "";
And this probably needs to be about:blank too.

>+    isRestoring = ss.restoreInProgress;
And then you don't need any of this.

>+        var buttonChoice = promptService.confirmEx(null, restoreTitle, restoreText,
>+                                          flags, okTitle, cancelTitle, null,
>+                                          null, {});
I spotted some more bad wrapping; the problem here is of course that confirmEx takes so many parameters. I think probably the best way to wrap it is this:
        var buttonChoice =
            promptService.confirmEx(null, restoreTitle, restoreText, flags,
                                    okTitle, cancelTitle, null, null, {});
Attached patch v 3.0 (obsolete) — — Splinter Review
Finally thins patch gets proper logic without isRestoring weirdness! Also addresses comments above.
Attachment #360270 - Attachment is obsolete: true
Attachment #360326 - Flags: ui-review?(neil)
Attachment #360326 - Flags: review?(ajschult)
Attachment #360270 - Flags: review?(ajschult)
Attachment #360326 - Flags: ui-review?(neil) → ui-review+
Comment on attachment 360326 [details] [diff] [review]
v 3.0

>@@ -586,25 +595,26 @@ function Startup()
Not sure what happened here, there shouldn't be any changes to this bit.

>+    // return null if we are restoring previous session
>+    if (ss.doRestore())
>+      return "about:blank";
Comment isn't quite right here.

>+++ b/suite/common/src/nsSessionStartup.js

>+function debug(aMsg) {
>+  aMsg = ("SessionStartup: " + aMsg).replace(/\S{80}/g, "$&\n");
Could remove this and...

>+  Components.classes["@mozilla.org/consoleservice;1"]
>+            .getService(Components.interfaces.nsIConsoleService)
>+            .logStringMessage(aMsg);
change this to .logStringMessage("SessionStartup: " + aMsg);

>+++ b/suite/common/src/nsSessionStore.js

>+function debug(aMsg) {
>+  Components.classes["@mozilla.org/consoleservice;1"]
>+            .getService(Components.interfaces.nsIConsoleService)
>+            .logStringMessage("SessionStartup: " + aMsg);
Should be .logStringMessage("SessionStore: " + aMsg);
Attached patch v 3.1 (obsolete) — — Splinter Review
Sorry, fixed all comments, carrying forward Neil sr+ and ui-review+
Attachment #360326 - Attachment is obsolete: true
Attachment #360455 - Flags: review?(ajschult)
Attachment #360326 - Flags: review?(ajschult)
Comment on attachment 360455 [details] [diff] [review]
v 3.1

>diff --git a/suite/browser/navigator.js b/suite/browser/navigator.js
>--- a/suite/browser/navigator.js
>+++ b/suite/browser/navigator.js
>@@ -564,6 +564,15 @@ function Startup()
>     document.documentElement.setAttribute("screenY", screen.availTop);
>   }
> 
>+  // initialize the session-restore service
>+  try {
>+    var ss = Components.classes["@mozilla.org/suite/sessionstore;1"]
>+                       .getService(Components.interfaces.nsISessionStore);
>+    ss.init(window);
I have discovered what might be a bug in Session Restore. The problem is that this code executes before the window has been shown in its correct size and position, so that (at least on Windows) the window's current size is bogus. This is a problem if you crash before the next save interval.
(In reply to comment #226)
> I have discovered what might be a bug in Session Restore.

I hope this can be fixed in a followup (and possibly even so that FF can also be fixed) and doesn't block this patch going in. I favor the large features going in ASAP and then doing smaller followups on top of it, as those smaller chunks are easier to review, can possibly be done by different people, and the large thing is less likely to bitrot.
Attached patch v 3.2 (pushed) — — Splinter Review
I've noticed strange behavior with window placement in Linux too. FF has function delayedStartup, where sessionstore is initialized. Seamonkey doesn't have that function. So i think this is a Seamonkey only bug. Fixed as Neil proposed during IRC chat. Crrying ui-review+ and sr+
Attachment #360455 - Attachment is obsolete: true
Attachment #360505 - Flags: review?(ajschult)
Attachment #360455 - Flags: review?(ajschult)
Comment on attachment 360505 [details] [diff] [review]
v 3.2 (pushed)

r=ajschult on the bits that touch existing SM code, nsSessionStartup.js and parts of nsSessionStore.js.  rs=ajschult on the rest of nsSessionStore.js
Attachment #360505 - Flags: review?(ajschult) → review+
(In reply to comment #217)
> > Isn't that what JSON.jsm is for?
> Will file followup bug for this. Currently doing what Firefox does.

JSON.jsm was removed in bug 462774. Bug 387859 is open on Firefox to get rid of eval usage, but I think that was mostly fixed in bug 407110.
http://hg.mozilla.org/comm-central/rev/212adb96ed90
Status: ASSIGNED → RESOLVED
Closed: 16 years ago15 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0a3
Comment on attachment 360505 [details] [diff] [review]
v 3.2 (pushed)

Two nits for a follow-up:

>+    //this is firefox part of code, keeping it for reference only. Will be removed in final patch
>+    //misak if (winData._closedTabs && (root._firstTabs || aOverwriteTabs)) {

Wasn't removed.

>+    // restore text data saved by SeaMonkey

/SeaMonkey/Firefox 2.0\/3.0/ (that's our own legacy format which you might not even need to support).
Attachment #360505 - Attachment description: v 3.2 → v 3.2 (pushed)
Depends on: 478506
Depends on: 478470
(In reply to comment #232)

Filed as bug 478470.
The getClosedTabData() API function is hopelessly broken because SeaMonkey doesn't store any tabData in gBrowser.saveBrowsers.  This results in calls to getClosedTabData returning an array of empty arrays.

I filed Bug 478579 to document this.
Depends on: 478579
No longer depends on: 478579
Depends on: 479048
Blocks: 479962
Blocks: 479992
Blocks: 480109
Blocks: 480110
I've only just started using Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.1b3pre) Gecko/20090221 SeaMonkey/2.0b1pre which incorporates the "Restore previous session" option in the Preferences.

I left that option unchecked as I really have no use for it.

Seamonkey crashed after playing a flash movie clip. When I restarted Seamonkey it started trying to restore the previous session regardless of the Preference setting - and I really did not want to try to reload the website that had caused Seamonkey to crash.

Is there likely to be a fix for this?
Flags: blocking-seamonkey2.0b1?
As of now, it shows a page called "about:sessionstore" and it will display the pages it will restore.
You can uncheck the bad pages.
It even tells you that you should disable if think some page is causing the crash.
> When I restarted Seamonkey it started trying to restore the previous session
> regardless of the Preference setting

Yes, sessionstore is designed to work for restoring after a crash even with the pref set to false.  You can set browser.sessionstore.resume_from_crash to false to disable crash recovery.  And/or see comment 236.

Anyway, if you think there is a bug, please file a new bug.
Flags: blocking-seamonkey2.0b1?
Should I understand from comment 236 that there should be something displayed prior to the actual restore session happening?

I have had Seamonkey crash twice and in both instances on restarting Seamonkey simply starts loading the pages that were open at the time of the crash. There is no option about loading those pages or not and no page called "about:sessionstore" displayed.
First, sorry for the typo.
It's about:sessionrestore.

That was not present when session restore landed, but already was present at the time of the writing of my comment.
I meant, if you download a recent nightly build a page will be displayed asking which tabs/windows to restore.

You can close seamonkey using some process manager (control+alt+del and kill the seamonkey.exe process on windows, pkill seamonkey on linux) and see the new UI.

This feature will be present on beta1.
(In reply to comment #239)
> It's about:sessionrestore.
> 
> That was not present when session restore landed, but already was present at
> the time of the writing of my comment.
> I meant, if you download a recent nightly build a page will be displayed asking
> which tabs/windows to restore.

In comment 235 Peter said he was using Gecko/20090221 SeaMonkey/2.0b1pre. The error page landed on 2009-02-13 (changeset 1cba9a3b59cb) and 2009-02-14 (packaging fix, changeset dc10ce1e0345, both in time for the next nightly build) so he clearly has these changes in his build.

I rather suspect that what Peter sees is described in comment 214, point 3: under certain conditions the session will be restored without asking [1]: when either
- browser.warnOnQuit is false
- SeaMonkey is restaring and browser.warnOnRestart is false
- browser.tabs.warnOnClose is false.
Especially the last one is easy to hit: Just open multiple tabs, close the window and untick the checkbox in the resulting dialog (if you get no dialog the pref is already set to false).
If anyone feels like discussing whether to keep this behavior and/or add something to the Preferences window (BTW the Restore Previous Session radio option controls normal behavior, not the event of a crash) I think it's best to open a new bug for that.

[1] <http://mxr.mozilla.org/comm-central/source/suite/common/src/nsSuiteGlue.js#178>
I had a look at comment 214 and then checked my browser configuration using about:config

The only setting is browser.tabs.warnOnClose True

The following settings do not exist

- browser.warnOnQuit
- browser.warnOnRestart


I will try adding browser.warnOnRestart true to prefs.js and report back.
I added browser.warnOnRestart true, started the browser, generated a crash with 3 tabs open and then went for a coffee.

Returned with coffee and restarted browser. All tabs started lading, No "about:sessionrestore" page/message/whatever.

Anything else to try before opening a new bug on this?
Peter, if your browser crashing, your session will be restored on next start. If it crashes again, about:sessionrestore will be shown. You can read more on bug 448976. Especially there was a pref browser.sessionstore.max_resumed_crashes, which controls for how many consequent crashes we don't bother the user with
questions at all. So basically, your session will be always restored if it's crashed. If you don't like that behavior, try to set browser.sessionstore.resume_from_crash to false to disable sessionstore.
Whiteboard: [Firefox-parity] [Workaround: Comment 119] → [Firefox-parity]
(In reply to comment #239)
> I meant, if you download a recent nightly build a page will be displayed asking
> which tabs/windows to restore.

Will this be available in Firefox as well? This is a very valuable feature.
It's in Firefox 3.1b2(In reply to comment #244)
> Will this be available in Firefox as well? This is a very valuable feature.

It will be in the Firefox 3.1 release (it's currently in Firefox 3.1b2).
And technically it was in Firefox first.
Silly questions: Why is "about:sessionrestore" not shown when restarting from the 1st crash? - Why does SM have to crash a 2nd time before that page of options is displayed?

Seems like lousy design to me but maybe answers to the above will clarify the issue...
That design decision came from Firefox.  See bug 448976 comment 10.
If you want it to prompt you every time set the browser.sessionstore.max_resumed_crashes preference to 0.
Thanks Michael, that cures the problem.

Why the default should be set so that the browser has to crash twice before the helpful "about:sessionrestore" page appears is not clear to me...
The strangest thing is that Peter crahsed more than once and didn't saw that page.

Whether to display such page or not would make more sense when such "consecutive" crashes happened without a successful restore of the previous section.

If SM crashes, then restore the section successfully, then navigate around and use mail for 10 hours. If SM crashes again, it will display the prompt, even though that wouldn't make much sense.

The option on the UI to show that prompt for every crash is great, but deserves an own request for enhancement - on another bug.
Depends on: 514388
Depends on: 516330
Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query
Depends on: 521112
(In reply to comment #249)
> The strangest thing is that Peter crahsed more than once and didn't saw that
> page.

I got this experience a while ago too.
 
> The option on the UI to show that prompt for every crash is great, but deserves
> an own request for enhancement - on another bug.

I filed bug 522466 for that. It depends on Peters bug 481400 for a changed default setting.

VERIFIED
Status: RESOLVED → VERIFIED
Blocks: 110535
Depends on: 734883
You need to log in before you can comment on or make changes to this bug.