328159 - Wire up the Session-data Service

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement)

Description

[Dietrich Ayala (:dietrich)]

convert the service used in the CrashRestore extension for inclusion in core:

- add inline docs
- remove CrashRestore-specific jargon
- add props to original author(s)
- write unit tests

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: session-restore patch for branch

Initial hookup of SessionStore service into Fx2 (trunk patch coming, still testing).

- force-kill app to test crash restoration
- browser.sessionstore.resume_session=true to always restore
- browser.sessionstore.resume_session_once=true to resume once

I'll file a separate bug for GUI elements of this feature.

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 214507 [details] [diff] [review]
session-restore patch for branch

bad patch, has tabs

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: session-restore patch for branch

new patch for branch, sans tabs

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: patch for branch and trunk

* fixed obsoleted-ness introduced since yesterday
* contains proper nsSessionStore version
* works on trunk and branch

Comment 5

[Nickolay_Ponomarev]

Comment on attachment 214702 [details] [diff] [review]
patch for branch and trunk

Some drive-by comments even though I'm not a reviewer.

>+    mCID: Components.ID("{5280606b-2510-4fe0-97ef-9b5a22eafe6b}"), //zeniko: new ID needed for new API
I don't think this is a valuable comment.

>+    mContractID: "@mozilla.org/browser/sessionstore;0", //zeniko: change to version 1 with new API
Why not start with v. 1?

>+    mObserverService: Components.classes["@mozilla.org/observer-service;1"].getService(Components.interfaces.nsIObserverService),
Normally, JS files are wrapped at 80 chars in the tree. Also I'm not sure if getting all the services at this point is a good idea.

>+    mUnicodeConverter: Components.classes["@mozilla.org/intl/scriptableunicodeconverter"].createInstance(Components.interfaces.nsIScriptableUnicodeConverter),
Perhaps the 1.8-only converter stream components should be used instead of this? I guess it would be a bit more efficient.
http://developer.mozilla.org/en/docs/Reading_textual_data#Gecko_1.8_and_newer

>+    // xul:tab attributes to (re)store (TMP/TBP features)
>+    mXulAttributes: ["locked", "protected", "marked"],
Restoring extension-specific attributes in the core code? I think it would be better to allow extensions specify additional data to be stored.

>+    mLoadState: 0, // 1 = ready, -1 = shutting down
Perhaps use named constants?

>+            this.getBrowserWindows().forEach(function(aWindow) {
Since getBrowserWindows()'s return value is only used to call forEach on it, perhaps make it execute the callback function directly instead of returning an array?

>+            var len = this.mPrefRoot.length;
>+            if (aData.substr(0, len) == this.mPrefRoot)
>+            {
This check is redundant, as you registered the pref observer for the mPrefRoot subbranch only, right?

>+            // notification of tab add/remove/selection
Note bug 322898.

>+        // react on "load" (persisted == undefined) and solitary "pageshow" events
>+        // (the first "pageshow" following "load" is too late for deleting the data caches)
>+        if (aEvent.persisted == undefined || aEvent.persisted)
Why not aEvent.type == "load" || aEvent.persisted ?

>+/* ........ nsISessionStore API .............. */
Is nsISessionStore interface defined anywhere?

>+        if (!id || aTextarea.tagName.toLowerCase() != "textarea" && (aTextarea.tagName.toLowerCase() != "input" || aTextarea.type.toLowerCase() != "text"))

The tagName check fails for <xhtml:input xmlns:xhtml="...">. You should probably use localName instead.

>+var tabData = SessionStoreService.mWindows[aWindow.__SSi].Tab[aIx] || null;
>+if (!tabData)
>+{
>+    return;
>+}
What's the point of "||null"? That part only evaluates if the first operand is 'falsy', in which case you would bail out of the function anyway.

>+        if (!aWindow.getBrowser || !aWindow.getBrowser())
>+        { // happens for nglayout.debug.disable_xul_cache == true
>+            aWindow.setTimeout(this.restoreWindow_proxy, 100, aWindow, root, aOverwriteTabs);

I'm not sure I understand why getBrowser would be undefined in a window "load" handler?

>+// prevent the erasure of the cache after a crash by resetting its "dirty" flag
>+// see bug 105843 - code adapted from Danil Ivanov's "Cache Fixer" extension

I'm sure necko guys will protest against this.

>+    // restart downloads
Is it a good idea to restart downloads automatically, given that we don't support resume? Will this overwrite the already downloaded part? Are you sure it is what user expects?

>+                var window = this.getMostRecentBrowserWindow();
>+                if (window)
>+                {
>+                    this.mSavePending = true;
>+                    window.setTimeout(this.saveState_proxy, this.mPref_interval - delay, true);
>+                    return;

What if the window gets closed before the timeout fires? I think you should use XPCOM timers instead.

>+// allow extensions to hook in a more elaborate restore prompt
>+var dialogURI = this.getPref(this.mPrefRoot + "restore_prompt_uri");
Can't they just override the URI in chrome.manifest?

>+    // bool don't safe sensitive data if the user doesn't want to
"bool, don't save"?

>+            sstream.close();
>+            stream.close();
The first call actually closes the underlying stream. Not sure if it is preferred to close it again manually.

>+
>+writeFile: function(aFile, aData)
>+{
>+  if (!aFile.exists())
>+  {
>+    aFile.create(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0600);
Doesn't the PR_CREATE_FILE (0x08) flag to nsIOutputStream.init create the file for you?

>+    delFile: function(aFile)
>+    {
>+        try
>+        {
>+            aFile.remove(false);
>+        }
>+        catch (ex) { }
Is it a good idea to swallow the exception?

>+    QueryInterface: function(aIID)
>+    {
>+        if (!aIID.equals(Components.interfaces.nsISupports) && !aIID.equals(Components.interfaces.nsIObserver) && !aIID.equals(this.mCID))

The last check doesn't make any sense to me.

>+ * The .INI format used here is for performance and convenience reasons somewhat limited:
Yet another file format? FWIW, there is an ini parser in tree:
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsIINIParser.idl
although I don't think it's usable in 1.8 Firefox builds.

Myself I would use toSource/eval[InSandbox], but I'm not sure if others will like the idea.


>+        catMan.addCategoryEntry("app-startup", SessionStoreService.mClassName, "service," + SessionStoreService.mContractID, true, true, null);

What is the 6th param?

>+restore_title=Firefox - Crash Recovery
Is it ok that this will say "Firefox" even in builds without --enable-official-branding?

Comment 6

[timeless]

                 >+var dialogURI = this.getPref(this.mPrefRoot + "restore_prompt_uri");

is *not* the way the pref apis are supposed to be used. we have prefbranches for a reason.

you're using a lot of anonymous and nested functions in places where it'd probably be friendlier to use global functions. afaik you aren't benefiting from scoping and the fanciness doesn't gain anything but it might be more expensive.

i'm amazed at how much you're using forEach especially in combination with recursion. is that really necessary?

also, it'd be appreciated in my circles (i can't speak for firefox, but certainly anyone using a debugger or profiler) if you actually gave your functions names. e.g. observe: function SessionStoreService_observe ...

since you're using arrays as arrays, you should consider doing normal iteration over them. if someone later tries to extend your code by polluting something on the Object or Array prototype, the for (a in somearray) notation will break. we've actually had some amusing firefox bug reports about this. i know everyone likes syntactic sugar, and i must say you seem to like it more than everyone i've ever met (and i use sugar a lot).


Why did you make an object with a property named Window (Initial Caps)?

from experience with venkman which has similar code,
+                    throw new Error();
+            catch (ex) // in case browser or history aren't ready yet
+                    aWindow.setTimeout(this.restoreHistory_proxy, 100, aTabs, aCurrentTabs, aSelectTab, aCount);

worries me. now you're definitely trying to avoid repeating forever (which is good, venkman will get stuck infinitely trying to get some things...), the interactions between your aCount and your for loop aren't well described. is it absolutely the case that when one tab isn't ready all other tabs won't be ready, and once one tab is ready, all the other tabs will be? it seems to me like you're looping over two problems and possibly stomping over stuff that has already settled in some cases if the first 5 tabs restore properly and the 6th one takes some time. perhaps you should split the for loop in two, and ensure that all the tabs are ready before trying to operate on them (if you give up on the ensurance, you can then operate on all the rest once).

if you're likely to create a number of objects of the same kind, you should consider using Components.Constructor(contract, interface, initializer).

InputStream = Components.Constructor("@mozilla.org/binaryinputstream;1", Components.interfaces.nsIBinaryInputStream, "setInputStream")
...
var input = new InputStream(stream);

Does AutoDownloader need to be a nested class? are you actually using the scope chain? if not, you're creating distinct classes for each instantiation, which isn't cheap.

Comment 7

[Dietrich Ayala (:dietrich)]

Thanks for the comments Nickolay and timeless. I'll update the patch to address most of these issues, and then will respond to the rest.

Re: nsISessionStore - The interface hasn't been nailed down yet. I'm going to include those changes in a separate bug/patch.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 214702 [details] [diff] [review]
patch for branch and trunk

Some general comments:
- Two space indentation is most common in browser/toolkit code
- lines need to wrap at 80 characters
- I know this isn't actually defined anywhere, but braces are usually same-line, and only used when surrounding a multiline block
- What are all the "_proxy" functions for? Seems like most are unnecessary.
- forEach takes a "this" object as the second param, so no need to hardcode "SessionStoreService" everywhere it's used
- all the setTimeouts make me worry - are they really all needed? seems like relying on multiple arbitrary timeouts to do stuff may lead to unexpected problems, and makes things hard to debug
- returning early can save a bunch of indentation - some entire functions are in a single if block
- There's no need to use explicit XPCNativeWrappers. See http://developer.mozilla.org/en/docs/XPCNativeWrapper.

> Index: components/nsSessionStore.js
> 
> +    mClassName: "Firefox Session Store Service",

A generic name, like "Browser Session Store Service", is probably better (I guess it doesn't matter that much).

> +    mObserverService: Components.classes["@mozilla.org/observer-service;1"].getService(Components.interfaces.nsIObserverService),
> +    mPrefBranch: Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch2),
> +    mWindowMediator: Components.classes["@mozilla.org/appshell/window-mediator;1"].getService(Components.interfaces.nsIWindowMediator),
> +    mIOService: Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService),
> +    mUnicodeConverter: Components.classes["@mozilla.org/intl/scriptableunicodeconverter"].createInstance(Components.interfaces.nsIScriptableUnicodeConverter),
> +    mProfileDirectory: null,

Unless these are all frequently used, it might be better to just get these when needed.

> +        // minimal interval between two save operations (in milliseconds)
> +        this.mPref_interval = this.getPref(this.mPrefRoot + "interval", 10000);
> +        // maximum number of closed windows remembered
> +        this.mPref_max_windows_undo = this.getPref(this.mPrefRoot + "max_windows_undo", 10);
> +        // maximal amount of POSTDATA to be stored (in bytes, -1 = all of it)
> +        this.mPref_postdata = this.getPref(this.mPrefRoot + "postdata", -1);
> +        // on which sites to save text data, POSTDATA and cookies
> +        // (0 = everywhere, 1 = unencrypted sites, 2 = nowhere)
> +        this.mPref_privacy_level = this.getPref(this.mPrefRoot + "privacy_level", 1);

maybe use |const SAVE_EVERYWHERE = 1|, etc, or something similar, defined globally?

> +        // resume the current session at startup (otherwise just recover from crashes)
> +        this.mPref_resume_session = this.getPref(this.mPrefRoot + "resume_session", false);
> +        // resume the current session at startup just this once
> +        this.mPref_resume_session_once = this.getPref(this.mPrefRoot + "resume_session_once", false);

These prefs seem a bit overkill - shouldn't we just choose some sane defaults and leave it at that? For example, does how much POSTDATA to store really something that needs to be configured? Why put a configurable maximum on the number of windows to save data for? Why put a maximum at all?

> +        this.mEOL = this.getEOL();

Why bother with all this per-platform EOL stuff? What's wrong with just using \n cross platform? This file isn't meant to be read by other programs or anything, right?

> +    // on window open
> +    onLoad: function(aWindow)
> +    {
> +        // if window is a browser window
> +        if (aWindow.document.documentElement.getAttribute("windowtype") == "navigator:browser")

could just return early and save the extra indentation

> +                // don't save during the first three seconds
> +                // (until most of the pages have been restored)
> +                // TODO: set a flag instead of arbitrary time
> +                //zeniko: flagging is pretty difficult since we'd have to wait for
> +                //zeniko: all tabs of all restored windows to be loaded (if any)
> +                this.mLastSaveTime = Date.now() - this.mPref_interval + 3000;

three seconds seems pretty arbitrary - and fragile. not sure how this can be done any better.

> +    // on window close
> +    onClose: function(aWindow)
> +    {
> +        if (aWindow.document.documentElement.getAttribute("windowtype") == "navigator:browser")
> +        {

again, you could just return early

> +    closedWindowNameAt: function(aIx)
> +    {
> +        return (aIx in this.mClosedWindows)?this.mClosedWindows[aIx]._title:null;

spaces between "?", ":" is the norm

> +    // updates the current document's cache of user entered text data
> +    saveTextData: function(aPanel, aTextarea)
> +    {
> +        var id = (aTextarea.id)?"#" + aTextarea.id:aTextarea.name;
> +        if (!id || aTextarea.tagName.toLowerCase() != "textarea" && (aTextarea.tagName.toLowerCase() != "input" || aTextarea.type.toLowerCase() != "text"))

These should be instanceof checks, they shouldn't rely on localName comparisons (it doesn't take into account namespaces, and can lead to security issues). There's code in browser.js that does this, see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.588#4399 .

> +    getCookies: function(aHosts)
> +    {
> +        var cookiesEnum = Components.classes["@mozilla.org/cookiemanager;1"].getService(Components.interfaces.nsICookieManager).enumerator;
> +        var cookies = { count: 0 };
> +        
> +        while (cookiesEnum.hasMoreElements())
> +        {
> +            var cookie = cookiesEnum.getNext().QueryInterface(Components.interfaces.nsICookie);
> +            if (cookie.expires == 0 && cookie.host)
> +            {
> +                var host = cookie.host.replace(/^\./, "");

could use nsICookie2's rawHost instead of host.

> +    restoreHistory: function(aWindow, aTabs, aCurrentTabs, aSelectTab, aCount
> 
> +            browser[id] = eval(this.restoreDocument_proxy.toString()
> +                .replace(/__ID__/g, id)
> +                .replace(/__SCROLL_POSITION__/g, currentScrollPos.join(", "))
> +                .replace(/__TEXT_ARRAY__/g, (tabData.text)?"\"" + tabData.text.replace(/[\\"]/g, '\\$&') + "\".split(' ')":"[]")
> +                .replace(/__FRAMESCROLL_ARRAY__/g, (tabData.framescroll)?"\"" + tabData.framescroll.replace(/[\\"]/g, '\\$&') + "\".split(' ')":"[]"));

This eval looks dangerous. Are you sure there is no chance that this can be exploited to cause arbitrary code exceution? Taking input from text fields in content and then eventually eval'ing it seems pretty risky.

> +    // restart downloads
> +    retryDownloads: function(aWindow)
> 
> +            onStopRequest: function(aRequest, aContext, aStatus)
> +            {
> +                if (aStatus == Components.results.NS_OK)

Components.isSuccessCode(aStatus) is theoretically better, though I suppose in this case it might not matter.

> +    // bool whether or not to resume session, if not recovering from a crash
> +    doResumeSession: function()
> 
> +        // resume session if pref is set
> +        return this.mPref_resume_session 
> +            // also resume if the user has set the last visited page as startup page
> +            || this.getPref("browser.startup.page", 1) == 2;

This means a change in behavior for people who have that pref set, right? The pref essentially becomes "use last session" instead of "use last visited page"?

> +    // bool prompt user whether or not to restore the previous session,
> +    // if the browser crashed
> +    doRecoverSession: function()
> +    {
> ...
> +    },

What kind of use cases are there for extension provided dialogs? Seems like a lot of trouble to go through to support it.

> +    // bool whether the user wants to load any other page at startup
> +    // (except the homepage) - needed for determining whether to overwrite the current tabs
> +    isCmdLineEmpty: function(aWindow)

This seems like something that nsBrowserContentHandler should be passing to the window, instead of comparing the URI to load to the homepage/about:blank/last page visited here once it's been decided. The cases where it sends defaultArgs are the cases where you want to overwrite, so it could just send another argument to the window.

> +    {
> +        if (!aWindow.arguments)
> +        {
> +            return true;
> +        }
> +        
> +        var homepage = null;
> +        switch (this.getPref("browser.startup.page", 1))
> +        {
> +        case 0:
> +            homepage = "about:blank";
> +            break;
> +        case 1:
> +            try
> +            {
> +                homepage = this.mPrefBranch.getComplexValue("browser.startup.homepage", Components.interfaces.nsIPrefLocalizedString).data;
> +            }
> +            catch (ex)
> +            {
> +                homepage = this.getPref("browser.startup.homepage", "");
> +            }

getComplexValue checks user prefs on it's own - this shouldn't be necessary.

> +    // bool don't safe sensitive data if the user doesn't want to
> +    // (distinguishes between encrypted and non-encrypted sites)
> +    checkPrivacyLevel: function(aURL)
> +    {
> +        return this.mPref_privacy_level < ((aURL.substr(0, 6) == "https:")?1:2);
> +    },

This is really hard to follow. Shouldn't this function take a boolean for whether the site is secure or not, and have callers pass in schemeIs("https") (or isSecure for the cookie case)?

> +    getStringBundle: function(aURI)
> +    {
> +        return Components.classes["@mozilla.org/intl/stringbundle;1"].
> +                          getService(Components.interfaces.nsIStringBundleService).
> +                          createBundle(aURI, Components.classes["@mozilla.org/intl/nslocaleservice;1"].
> +                          getService(Components.interfaces.nsILocaleService).
> +                          getApplicationLocale());

is there something wrong with temporary variables? :) This function is hard to read, as is.

> +    getEOL: function()
> +    {
> +        var OS = Components.classes["@mozilla.org/xre/app-info;1"].getService(Components.interfaces.nsIXULRuntime).OS;
> +        
> +        return /win|os[\/_]?2/i.test(OS)?"\r\n":/mac/i.test(OS)?"\r":"\n";
> +    },

I don't see why this is necessary. See comments above.

> +/* ........ Storage API .............. */
> +
> +    getPref: function(aName, aDefault)
> +    {
> +        var pb = this.mPrefBranch;
> +        switch (pb.getPrefType(aName))
> +        {
> +        case pb.PREF_STRING:
> +            return pb.getCharPref(aName);
> +        case pb.PREF_BOOL:
> +            return pb.getBoolPref(aName);
> +        case pb.PREF_INT:
> +            return pb.getIntPref(aName);
> +        default:
> +            return aDefault;
> +        }
> +    },

Hopefully some better pref wrappers come along at some point. This method will throw for non-existent prefs, is that expected? Usually wrappers that take a default handle that.

> Index: locales/en-US/chrome/browser/sessionstore.properties
> 
> +restore_title=Firefox - Crash Recovery

As Nickolay said, Firefox shouldn't be hardcoded here.  You can get the brandShortName string from brand.properties instead.

Comment 9

[Nickolay_Ponomarev]

(In reply to comment #8)
> - There's no need to use explicit XPCNativeWrappers. See
> http://developer.mozilla.org/en/docs/XPCNativeWrapper.
> 
Actually I had the same question, but from reading that page it appears that XPCNativeWrappers are created automatically only for "protected scripts". That term has a rather vague definition, but it appears that only chrome:// scripts are "protected", not that it really makes sense. :-/

Comment 10

[Simon Bünzli]

Some replies to what might remain the same for the next patch:

(In reply to comment #5)
> >+var tabData = SessionStoreService.mWindows[aWindow.__SSi].Tab[aIx] || null;
> What's the point of "||null"? That part only evaluates if the first operand is
> 'falsy', in which case you would bail out of the function anyway.

This prevents a strict warning (although currently, there should be no code path which could trigger one).

> >+        if (!aWindow.getBrowser || !aWindow.getBrowser())
> I'm not sure I understand why getBrowser would be undefined in a window "load"
> handler?

That's a left-over from when I called this function too early. The check can be safely removed.

> >+// prevent the erasure of the cache after a crash by resetting its "dirty" flag
> >+// see bug 105843 - code adapted from Danil Ivanov's "Cache Fixer" extension
> I'm sure necko guys will protest against this.

So am I. But as long as we don't store the entire DOM, preserving as much of the cache as possible should produce better results. Of course, fixing bug 105843 should be the way to go.

> >+    // restart downloads
> Is it a good idea to restart downloads automatically, given that we don't
> support resume? Will this overwrite the already downloaded part? Are you sure
> it is what user expects?

This will overwrite all partial downloads. Not sure why you'd want to preserve these as is (especially since in my tests it wasn't possible to cancel and restart them manually).

> >+// allow extensions to hook in a more elaborate restore prompt
> >+var dialogURI = this.getPref(this.mPrefRoot + "restore_prompt_uri");
> Can't they just override the URI in chrome.manifest?

Only if there's a dialog to overwrite in the first place. Currently, a simple confirm prompt is used (which should not be overwritten for such a specific reason).

> >+    aFile.create(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0600);
> Doesn't the PR_CREATE_FILE (0x08) flag to nsIOutputStream.init create the file
> for you?

It does indeed.

> >+    delFile: function(aFile)
> Is it a good idea to swallow the exception?

Generally not. In the specific cases this function is called: maybe.

> >+        if (!aIID.equals(Components.interfaces.nsISupports) && !aIID.equals(Components.interfaces.nsIObserver) && !aIID.equals(this.mCID))
> The last check doesn't make any sense to me.

this.mCID equals Components.interfaces.nsISessionStore.

> >+ * The .INI format used here is for performance and convenience reasons somewhat limited:
> Yet another file format? FWIW, there is an ini parser in tree:
> http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsIINIParser.idl
> although I don't think it's usable in 1.8 Firefox builds.

It isn't accessible for scripts - and it's only a parser (no nice writing wrapper).

> Myself I would use toSource/eval[InSandbox], but I'm not sure if others will
> like the idea.

The advantage of .INI was that it's quite simple to read and write (e.g. the RDF format I tried was a PITA) and still quite standardized (Opera uses a similar format, should be accessible for outside applications - such as session convertors). The best alternative would probably be an XML dialect. See also bug 328162.

(In reply to comment #6)
> you're using a lot of anonymous and nested functions in places where it'd
> probably be friendlier to use global functions. afaik you aren't benefiting
> from scoping and the fanciness doesn't gain anything but it might be more
> expensive.

The few anonymous functions there are usually need access to the local scope. As for using global functions: that's currently not the code's style.

> i'm amazed at how much you're using forEach especially in combination with
> recursion. is that really necessary?

No. They could of course all be replaced with simple loops.

> since you're using arrays as arrays, you should consider doing normal iteration
> over them.

for (... in ...) is only used to iterate over objects, not arrays (note that mWindows is an object).

> i must say you seem to like it more than everyone
> i've ever met (and i use sugar a lot).

Yeah, I've got a tendency to diabetes. ;-)

> Why did you make an object with a property named Window (Initial Caps)?

These objects (Window, Tab, Entry and Child) are all arrays of objects which get their own .INI sections. The objects' names are used for the sections header - and I preferred capitalization for them; e.g. [Window1.Tab1.Entry1] instead of [window1.tab1.entry1].

(In reply to comment #8)
> - What are all the "_proxy" functions for? Seems like most are unnecessary.

They're most probably a WTF - I wanted to make sure not to create unnecessary closures.

> - all the setTimeouts make me worry - are they really all needed? seems like
> relying on multiple arbitrary timeouts to do stuff may lead to unexpected
> problems, and makes things hard to debug

Only one setTimeout is really hairy (saveStateDelayed) - and that one can easily be replaced with an nsITimer.

> - There's no need to use explicit XPCNativeWrappers. See
> http://developer.mozilla.org/en/docs/XPCNativeWrapper.

I thought so, too - until I tested it.

> These prefs seem a bit overkill - shouldn't we just choose some sane defaults
> and leave it at that? For example, does how much POSTDATA to store really
> something that needs to be configured? Why put a configurable maximum on the
> number of windows to save data for? Why put a maximum at all?

I'd at least ensure that users can disable POSTDATA saving, as long as we don't prevent reposting when recovering. And the configurable maximum of restorable closed windows allows users to decide between safety and resource usage.

> Why bother with all this per-platform EOL stuff? What's wrong with just using
> \n cross platform? This file isn't meant to be read by other programs or
> anything, right?

Depends. I could think of a session convertor (e.g. between Opera's format and ours). Or simply people wanting to get a bit of information out of a session with Notepad.

> This eval looks dangerous. Are you sure there is no chance that this can be
> exploited to cause arbitrary code exceution? Taking input from text fields in
> content and then eventually eval'ing it seems pretty risky.

Should've been harmless (due to escaping), but can be done without.

> This means a change in behavior for people who have that pref set, right? The
> pref essentially becomes "use last session" instead of "use last visited page"?

Yes. Makes sense to me (never figured out another use case for that option) - and this would fix bug 159357. An alternative would be to add a fourth option (3 = use last session).

> What kind of use cases are there for extension provided dialogs? Seems like a
> lot of trouble to go through to support it.

E.g. my Session Manager replaces that prompt with a list of possible sessions to restore after a crash. You could also display a list of all tabs of the crashed session and allow to deselect the crashy ones. - And even if we don't go the chrome.manifest overriding way, that's just about 5 to 10 lines of code.

> > +    getPref: function(aName, aDefault)
> Hopefully some better pref wrappers come along at some point. This method will
> throw for non-existent prefs, is that expected? Usually wrappers that take a
> default handle that.

This method shouldn't throw. getPrefType returns PREF_INVALID for non-existent prefs. And I don't know whether including an external pref wrapper (which would have to be loaded through a mozIJSSubScriptLoader) is worth the hassle at this point.

Comment 11

[Nickolay_Ponomarev]

(In reply to comment #10)
> > >+var tabData = SessionStoreService.mWindows[aWindow.__SSi].Tab[aIx] || null;
> > What's the point of "||null"? 
> This prevents a strict warning (although currently, there should be no code
> path which could trigger one).
> 
I don't know how this can cause a strict warning, and as you say it doesn't cause it with current code anyway.

> > >+// prevent the erasure of the cache after a crash by resetting its "dirty" flag
> > >+// see bug 105843 - code adapted from Danil Ivanov's "Cache Fixer" extension
> > I'm sure necko guys will protest against this.
> 
> So am I. But as long as we don't store the entire DOM, preserving as much of
> the cache as possible should produce better results. Of course, fixing bug
> 105843 should be the way to go.
> 
I think the reason the cache is being cleared is to prevent weird instability problems from happening.

> > >+        if (!aIID.equals(Components.interfaces.nsISupports) && !aIID.equals(Components.interfaces.nsIObserver) && !aIID.equals(this.mCID))
> > The last check doesn't make any sense to me.
> 
> this.mCID equals Components.interfaces.nsISessionStore.
> 
First, there is no Components.interfaces.nsISessionStore in this patch, second if it's an interface id, it shouldn't be put in a property with name mCID.

> > Myself I would use toSource/eval[InSandbox], but I'm not sure if others will
> > like the idea.
> 
> The advantage of .INI was that it's quite simple to read and write (e.g. the
From your comments, I read that it's a limited version of INI format (so the file looks like INI, but isn't actually INI). And do we really care about others being able to read/write this format easily? A custom INI parser/writer is a large chunk of code...

Comment 12

[timeless]

> >+var tabData = SessionStoreService.mWindows[aWindow.__SSi].Tab[aIx] || null;
> What's the point of "||null"? That part only evaluates if the first operand is
> 'falsy', in which case you would bail out of the function anyway.

> This prevents a strict warning (although currently, there should be no code
> path which could trigger one).

that should *not* prevent the strict warning.

the strict warning is probably reference to undefined property. which you do the instant you poke Tab[aIx].

> >+        if (!aIID.equals(Components.interfaces.nsISupports) && !aIID.equals(Components.interfaces.nsIObserver) && !aIID.equals(this.mCID))
> The last check doesn't make any sense to me.

> this.mCID equals Components.interfaces.nsISessionStore.

then it's mislabeled. you should not use the same uuid for a CID and an IID it's confusing and will break things and people eventually and it shows a general confusion about the things. they're independent namespaces.

You should compare against Components.interfaces.nsISessionStore.

> >+ * The .INI format used here is for performance and convenience reasons somewhat limited:
> Yet another file format? FWIW, there is an ini parser in tree:
> http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsIINIParser.idl
> although I don't think it's usable in 1.8 Firefox builds.

> It isn't accessible for scripts - and it's only a parser (no nice writing
> wrapper).

what do you mean? both interfaces are marked as scriptable.

Normally InitialCaps is reserved for ClassNames, doing something else will confuse a lot of people and violates style.

wrt the iteration. oops i clearly confused which variable you were iterating over :(.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #9)
> but it appears that only chrome:// scripts are "protected", not that it really
> makes sense. :-/

Eek, I guess I was wrong about it being unnecessary - I thought JS Components where considered protected, but apparently they are not. Might be worth adding that to the documentation.

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 214702 [details] [diff] [review]
patch for branch and trunk

clearing review request, I'm sure dietrich has enough feedback to revise the patch by now!

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Please make sure to get me to sr= this and other Session-Restore related patches. I will give you a faster turnaround than I gave Gavin, I promise!

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 214702 [details] [diff] [review]
patch for branch and trunk

>Index: components/Makefile.in
> EXTRA_PP_COMPONENTS = \
> 	nsBrowserContentHandler.js \
> 	nsBrowserGlue.js \
>+	nsSessionStore.js \

Is this feature ever going to have its UI that's not part of any existing dialog? (e.g. preferences)... i.e. its own XUL files? If so, I would stick this in its own directory...

browser/components/session-restore or something, with the component under src and the xul+js under content/

>Index: components/nsSessionStore.js

>+const SessionStoreService = {
>+    mCID: Components.ID("{5280606b-2510-4fe0-97ef-9b5a22eafe6b}"), //zeniko: new ID needed for new API

As a general style nit: All new code should use _ as a prefix for private data, not 'm' because '_' can generally be used for both properties and methods without looking weird. (You've seen this._foo() a lot, but how many times have you see this.mFoo() ?)

>+    mObserverService: Components.classes["@mozilla.org/observer-service;1"].getService(Components.interfaces.nsIObserverService),

Echo other comments about wrapping. Suggestion: 

define 
const Cc = Components.classes;
const Ci = Components.interfaces;

_observerService: 
  Cc["@mozilla.org/observer-service;1"].
  getService(Ci.nsIObserverService);

>+    mPrefBranch: Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch2),
>+    mWindowMediator: Components.classes["@mozilla.org/appshell/window-mediator;1"].getService(Components.interfaces.nsIWindowMediator),
>+    mIOService: Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService),
>+    mUnicodeConverter: Components.classes["@mozilla.org/intl/scriptableunicodeconverter"].createInstance(Components.interfaces.nsIScriptableUnicodeConverter),

Ditto for all of these. 

>+    mObserving: ["domwindowopened", "domwindowclosed", "quit-application-requested", "quit-application-granted", "quit-application", "browser:purge-session-history"],

Wrap!

>+    // window attributes to (re)store
>+    mWinAttributes: ["width", "height", "screenX", "screenY", "sizemode"],
>+    // hideable window features to (re)store
>+    mWinHidable: ["menubar", "toolbar", "locationbar", "personalbar", "statusbar", "scrollbars"],
>+    // docShell capabilities to (re)store
>+    mCapabilities: ["Subframes", "Plugins", "Javascript", "MetaRedirects", "Images"],
>+    // xul:tab attributes to (re)store (TMP/TBP features)
>+    mXulAttributes: ["locked", "protected", "marked"],

A more detailed documentation overview of whow these properties are restored and what for would be nice. 

>+    init: function()
>+    {

For new code, use this style:

init: function SS_init() {

},

also: providing an explicit name means you can use NS_ASSERT to produce call stacks that are useful in debugging. 


>+        // minimal interval between two save operations (in milliseconds)
>+        this.mPref_interval = this.getPref(this.mPrefRoot + "interval", 10000);

If you're definining branch as a field, why not just make this function take the sub-pref and have it consult your branch internally? If not, what's the point of separating branch at all?

Speaking of init, when does this function get called? 

>+        this.mProfileDirectory = Components.classes["@mozilla.org/file/directory_service;1"].getService(Components.interfaces.nsIProperties).get("ProfD", Components.interfaces.nsILocalFile);
>+        this.mStringBundle = this.getStringBundle("chrome://browser/locale/sessionstore.properties"); // for the recover prompt

Wrap!

>+        // get string containing session state
>+        var iniString = this.stateFromDisk();
>+        if (iniString)
>+        {
>+            try
>+            {

nit: 

if (iniString) {
  try {

nit applies for entire file. 

>+                // parse the session state into JS objects
>+                this.mInitialState = IniObjectSerializer.decode(iniString);
>+                // set bool detecting crash
>+                this.mLastSessionCrashed = this.mInitialState.Session && this.mInitialState.Session.state && this.mInitialState.Session.state == "running";

Wrap, use constant for "running"

>+                if (this.mInitialState.Window && this.mInitialState.Window[0])

What's .Window? if this is a property you define, why is it uppercase?

>+        // if last session crashed, backup the session and try to restore the disk cache
>+        if (this.mLastSessionCrashed)

An overview of how things are supposed to work, as a single documentary overview, in the code, would be nice. 

>+    // handle notifications
>+    observe: function(aSubject, aTopic, aData)

nit: don't prefix parameters with 'a'. 

>+        switch (aTopic)
>+        {
>+        // on app startup, register observer for when profile is completely loaded
>+        // alternative would be to init service in browserGlue instead of hooking in via catmanager

If this function is ever likely to grow to be more complex, breaking some of these things out into sub-functions may be desirable. 

Your functions should all have comments like this:

/**
 * Discombobulates the frinko-regulator
 * @param   foo
 *          Does something. Cannot be null. 
 * @returns true if the frinko-regulator was discombobulated. 
 */

>+    serializeHistoryEntry: function(aEntry, aContent)
>+    {
>+        var entry = { uri: aEntry.URI.spec, Child: [] };
>+        
>+        if (aEntry.title && aEntry.title != entry.uri)
>+        {
>+            entry.title = aEntry.title;
>+        }

braces for single line blogs begin to get really tiring once you have this many small sub-blocks. 

Since you're going to produce more patches, and I need to dash off to a meeting I'm going to leave this at just these style and documentation nits. Will do a more comprehensive read-through of the code later.

Comment 17

[Dietrich Ayala (:dietrich)]

Attachment: patch for branch and trunk

* fixes style issues
* fixes most reviewer comments thus far (pre-#16, which will be addressed in next patch)

Comment 18

[logan]

It seems _getSessionFile() is called pretty often. Would it be better to create two nsIFile objects under SessionStoreService to avoid the clone() and appending of sessionstore.{ini,bak} each time?

Also, is it a good idea to save POSTDATA by default, regardless of the privacy setting? I just see this causing some headaches on forums and the like. If it were to stay on, perhaps a reasonable default of 128kb could be used...

Comment 19

[Dietrich Ayala (:dietrich)]

POSTDATA is going to be turned off by default in the next patch.

Comment 20

[Dietrich Ayala (:dietrich)]

> ------- Comment #16 from [e-mail]  2006-03-27 12:17 PST -------
> (From update of attachment 214702 [details] [diff] [review])
>> Index: components/Makefile.in
>> EXTRA_PP_COMPONENTS = \
>> 	nsBrowserContentHandler.js \
>> 	nsBrowserGlue.js \
>> +	nsSessionStore.js \
> 
> Is this feature ever going to have its UI that's not part of any existing
> dialog? (e.g. preferences)... i.e. its own XUL files? If so, I would stick this
> in its own directory...
> 
> browser/components/session-restore or something, with the component under src
> and the xul+js under content/

Yes, it's likely that UI will be added. Can someone with commit access add the browser/components/sessionstore and browser/components/sessionstore/src directories? (as the component is nsSessionStore)

Comment 21

[Nickolay_Ponomarev]

>+  // xul:tab attributes to (re)store (extensions might want to hook in here)
>+  _xulAttributes: [],
How would an extension go about hooking in there? I guess the answer is "using the not yet defined nsISessionStore API" but this is something that needs to be remembered.

>+        !aIID.equals(Ci.nsISessionStore || null)) {
I would strongly advise leaving this out of the initial patch and add this line (sans the ||null part) in the same patch that introduces the interface.

Comment 22

[Dietrich Ayala (:dietrich)]

Attachment: addressing most recent reviews/comments

> ------- Comment #16 from [e-mail]  2006-03-27 12:17 PST -------
> 
> Is this feature ever going to have its UI that's not part of any existing
> dialog? (e.g. preferences)... i.e. its own XUL files? If so, I would stick this
> in its own directory...
> 
> browser/components/session-restore or something, with the component under src
> and the xul+js under content/

Fixed

>> +    // window attributes to (re)store
>> +    mWinAttributes: ["width", "height", "screenX", "screenY", "sizemode"],
>> +    // hideable window features to (re)store
>> +    mWinHidable: ["menubar", "toolbar", "locationbar", "personalbar", "statusbar", "scrollbars"],
>> +    // docShell capabilities to (re)store
>> +    mCapabilities: ["Subframes", "Plugins", "Javascript", "MetaRedirects", "Images"],
>> +    // xul:tab attributes to (re)store (TMP/TBP features)
>> +    mXulAttributes: ["locked", "protected", "marked"],
> 
> A more detailed documentation overview of whow these properties are restored
> and what for would be nice. 

Fixed

> Speaking of init, when does this function get called? 

It's called at startup, after receiving the profile-after-change notification.

>> +                // parse the session state into JS objects
>> +                this.mInitialState = IniObjectSerializer.decode(iniString);
>> +                // set bool detecting crash
>> +                this.mLastSessionCrashed = this.mInitialState.Session && this.mInitialState.Session.state && this.mInitialState.Session.state == "running";
> 
> Wrap, use constant for "running"

Fixed

>> +                if (this.mInitialState.Window && this.mInitialState.Window[0])
> 
> What's .Window? if this is a property you define, why is it uppercase?

That object is the parsed representation of the ini file. That property is uppercase because it appears in the ini file as:

[Window1]
selected=1
width=994
height=846
etc

It's possible that storage may change (328162), which may affect how this is done.

> 
>> +        // if last session crashed, backup the session and try to restore the disk cache
>> +        if (this.mLastSessionCrashed)
> 
> An overview of how things are supposed to work, as a single documentary
> overview, in the code, would be nice. 

Added

> Your functions should all have comments like this:
> 
> /**
>  * Discombobulates the frinko-regulator
>  * @param   foo
>  *          Does something. Cannot be null. 
>  * @returns true if the frinko-regulator was discombobulated. 
>  */
> 

Fixed

> ------- Comment #18 from [e-mail]  2006-03-28 13:43 PST -------
> It seems _getSessionFile() is called pretty often. Would it be better to create
> two nsIFile objects under SessionStoreService to avoid the clone() and
> appending of sessionstore.{ini,bak} each time?

Fixed

> Also, is it a good idea to save POSTDATA by default, regardless of the privacy
> setting? I just see this causing some headaches on forums and the like. If it
> were to stay on, perhaps a reasonable default of 128kb could be used...

Fixed, now is off by default.

> ------- Comment #21 from [e-mail]  2006-04-03 11:56 PST -------
>> +  // xul:tab attributes to (re)store (extensions might want to hook in here)
>> +  _xulAttributes: [],
> How would an extension go about hooking in there? I guess the answer is "using
> the not yet defined nsISessionStore API" but this is something that needs to be
> remembered.

It's currently targeted for Fx2B1 on http://wiki.mozilla.org/Firefox2/Requirements.

https://bugzilla.mozilla.org/show_bug.cgi?id=332774

>> +        !aIID.equals(Ci.nsISessionStore || null)) {
> I would strongly advise leaving this out of the initial patch and add this line
> (sans the ||null part) in the same patch that introduces the interface.

Fixed

Comment 23

[Ben Goodger (use ben at mozilla dot org for email)]

I am very interested to know the effect of this on Ts and Tp.

Primarily because:

- A lot of work gets done on "load"... does this mean pageload? A lot of work was done in the browser to consolidate the effort that gets done as pages load and make it as minimal as possible. 
- A lot of additional new work gets done on startup.

Is it possible to get some performance numbers for this? Maybe have it checked in and turned off, enabled with a ac flag that a tinderbox can build?

Just reading through the code, I see plenty of opportunity for optimization should there be problems. I think we should figure out whether or not those are necessary though before worrying about them. 

Comment 24

[Dietrich Ayala (:dietrich)]

> ------- Comment #23 from [e-mail]  2006-04-06 10:35 PDT -------
> I am very interested to know the effect of this on Ts and Tp.
> 
> Primarily because:
> 
> - A lot of work gets done on "load"... does this mean pageload? A lot of work
> was done in the browser to consolidate the effort that gets done as pages load
> and make it as minimal as possible. 

It's window load (probably what you're referring to). There not much done directly on tab or page load.

> - A lot of additional new work gets done on startup.

Assuming the service loads, but does not do any restoration of session, we'll incur some Ts penalty for:

1. reading the session file
2. adding observers

And some penalty at window load for registering event listeners.

We can probaby mitigate this somewhat by changing our storage mechanism (328162), and there is probably some event-listener consolidation possible.

> Is it possible to get some performance numbers for this? Maybe have it checked
> in and turned off, enabled with a ac flag that a tinderbox can build?
> 
> Just reading through the code, I see plenty of opportunity for optimization
> should there be problems. I think we should figure out whether or not those are
> necessary though before worrying about them. 

Here are some quick Ts testing results (no guarantees on the math):

http://numsum.com/spreadsheet/show/17864

I did 10 runs with and without the patch, dropped hi/lo, which showed a 2.4% increase in Ts. I'll try to isolate where exactly this time is being spent.

Comment 25

[Dietrich Ayala (:dietrich)]

Attachment: includes bugfixes, etc from zeniko

Comment 26

[Dietrich Ayala (:dietrich)]

The previous Ts numbers were incorrect. I had accidentally left "always resume" turned on, but since it was resuming only the start page, I was fooled.

However, the new Ts numbers are entirely counter-intuitive:

http://numsum.com/spreadsheet/show/17864

This shows that startup is *faster* with the session-restore patch than without. It also shows that implementing storage with toSource/eval is *slower* than INI encoding/decoding, which I very much doubt.

It may be that my sample size is too small, which is allowing outliers to skew the results, even after removing highest and lowest. I'm going to clean up my environment and run a longer series of tests, which will hopefully provide a clearer picture.

Comment 27

[Dietrich Ayala (:dietrich)]

Attachment: minor update with some changes requested by mconnor

Comment 28

[Mike Connor [:mconnor]]

Comment on attachment 218774 [details] [diff] [review]
minor update with some changes requested by mconnor


+/* ***** Session Storage and Restoration **** 
+
+Overview
+This service keeps track of a user's session, storing the various bits
+required to return the browser to it's current state. The relevant data is 
+stored in memory, and is periodically saved to disk in an ini file in the 
+profile directory. The service starts when the browser starts, and will restore
+the session from the information contained in the ini file under circumstances
+described below.
+
+Crash Detection
+The initial segment of the INI file is has a single field, "state", that 
+indicates whether the browser is currently running. When the browser shuts 
+down, the field is changed to "stopped". At startup, this field is read, and if
+it's value is "running", then it's assumed that the browser had previously
+crashed, or at the very least that something bad happened, and that we should
+restore the session.
+
+Forced Restarts
+In the event that a restart is required due to application update or extension
+installation, set the browser.sessionstore.resume_session_once pref to true,
+and the session will be restored the next time the browser starts.
+
+Always Resume
+XXXX
+
+*/

fill in the XXXX, and make sure this works with doxygen-based systems



+/* :::::::: Pref Defaults :::::::::::::::::::: */
+
+// minimal interval between two save operations (in milliseconds)
+const PREF_INTERVAL = 10000;
+
+// maximum number of closed windows remembered
+const PREF_MAX_WINDOWS_UNDO = 10;
+
+// maximum number of closed tabs remembered (per window)
+const PREF_MAX_TABS_UNDO = 10;
+
+// maximal amount of POSTDATA to be stored (in bytes, -1 = all of it)
+const PREF_POSTDATA = 0;
+
+// on which sites to save text data, POSTDATA and cookies
+// (0 = everywhere, 1 = unencrypted sites, 2 = nowhere)
+const PREF_PRIVACY_LEVEL = PRIVACY_ENCRYPTED;
+
+// resume the current session at startup (otherwise just recover)
+const PREF_RESUME_SESSION = false;
+
+// resume the current session at startup just this once
+const PREF_RESUME_SESSION_ONCE = false;

these are defaults, can you call that out in the const names?  makes the code more obvious

+/* :::::::: The Service ::::::::::::::: */
+
+const SessionStoreService = {

this should be a var instead

+  _prefBranchName: "sessionstore.",

there's no need for this, just getPref with sessionstore.foo, appending strings when not necessary sucks

+  _observing: [
+    "domwindowopened", "domwindowclosed",
+    "quit-application-requested", "quit-application-granted",
+    "quit-application", "browser:purge-session-history"
+  ],

this can be a const as well, it doesn't change afaict

+  /*
+  XUL Window properties to (re)store
+  Restored in restoreDimensions_proxy()
+  */
+  _winAttributes: ["width", "height", "screenX", "screenY", "sizemode"],

again, doesn't need to be part of the object if its not changing

+  /* 
+  Hideable window features to (re)store
+  Restored in restoreWindowFeatures()
+  */
+  _winHidable: ["menubar", "toolbar", "locationbar", "personalbar",
+    "statusbar", "scrollbars"],

and again (fix indenting)

+  /*
+  docShell capabilities to (re)store
+  Restored in restoreHistory()
+  eg: browser.docShell["allow" + aCapability] = false;
+  */
+  _capabilities: ["Subframes", "Plugins", "Javascript",
+    "MetaRedirects", "Images"],

and again!

+  // xul:tab attributes to (re)store (extensions might want to hook in here)
+  _xulAttributes: {},

if extensions are hooking this, it shouldn't be private, and let's make it an array so we can do nicer things with it where needed

+  // set default load state
+  _loadState: STATE_STOPPED,
+
+  // minimal interval between two save operations (in milliseconds)
+  _interval: PREF_INTERVAL,
+
+  // in milliseconds (usually Date.now())
+  _lastSaveTime: 0, 

the comment is a bit confusing, it sort of implies that the value will be Date.now()

+  // states for all currently opened windows
+  _windows: {},

array instead

+  // storage for Undo Close Window
+  _closedWindows: [],
+
+  // in case the last closed window ain't a navigator:browser one
+  _lastWindowClosed: null,
+
+  // not-"dirty" windows usually don't need to have their data updated
+  _dirtyWindows: {},

array instead

+  // flag all windows as dirty
+  _dirty: false,

so setting this to true adds all windows to dirty? or we just use all windows instead of _dirtyWindows if this is true?

+/* ........ Global Event Handlers .............. */
+
+  /**
+   * Initialization, called upon profile-after-change notification 
+   */
+  _init: function sss_init() {
+    this._prefBranch =
+      Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService).
+                                               getBranch("browser.");
+    this._prefBranch.QueryInterface(Ci.nsIPrefBranch2);
+
+    // allow disabling by extensions that replace this functionality
+    if (!this._getPref("enabled", true)) {
+      return;
+    }

followup to the other browserglue comment, we should only load the component if this pref is set to true :)

+    // if last session crashed, backup the session
+    // and try to restore the disk cache
+    if (this._lastSessionCrashed) {
+      try {
+        this._writeFile(this._getSessionFile(true), aState, true);
+      }
+      catch (ex) { } // nothing else we can do here
+      try {
+        this.restoreCache();
+      }
+      catch (ex) { debug(ex); } // at least we tried
+    }
+  },

I'd like to get darin's opinion on the safety of restoring cache like this before we land

+  /**
+   * Called on application shutdown, after notifications:
+   * quit-application-granted, quit-application
+   */
+  _uninit: function sss_uninit() {
+    if (this._doResumeSession()) { // save all data for session resuming 
+      this.saveState(true);
+    }
+    else { // discard all session related data 
+      this._clearDisk();
+    }
+    
+    // remove observers
+    var observerService = 
+      Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
+
+    this._observing.forEach(function(aTopic) {
+      observerService.removeObserver(this, aTopic);
+    }, this);
+
+    this._prefBranch.removeObserver("", this);
+  },

don't worry about removing observers if we're quitting, and the prefbranch call is wrong now anyway...

+  /**
+   * Handle notifications
+   */
+  observe: function sss_observe(aSubject, aTopic, aData) {
+    var observerService = 
+      Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
+
+    switch (aTopic) {
+    // on app startup, register observer for when profile is completely loaded
+    // alternative would be to init service in browserGlue instead of hooking
+    // in via catmanager
+    case "app-startup": 
+      observerService.addObserver(this, "profile-after-change", false);
+      break;
+    // after profile loaded, start up service activity
+    case "profile-after-change":
+      observerService.removeObserver(this, aTopic);
+      this._init();
+      break;

you want to observe final-ui-startup here.  It'd also probably just be easier to init from browserglue, since that's one less observer to deal with, and that's why browserglue exists.  Otherwise we're at least reading this file before the EM restart, and possibly unnecessarily


+      case this._prefBranchName + "max_tabs_undo":
+        for (var ix in this._windows) {
+          this._windows[ix]._closedTabs.splice(this._getPref("max_tabs_undo", PREF_MAX_TABS_UNDO));
+        }
+        break;
+      case this._prefBranchName + "interval":
+        this._interval = this._getPref("interval");
+      }
+      break;
+    case "timer-callback": // timer call back for delayed saving
+      this._saveTimer = null;
+      this.saveStateDelayed(null, -1);
+    }
+  },

its not strictly necessary, but add the break at the end so people don't screw up if they add cases in the future

+  /**
+   * If it's the first window load since app start...
+   * - determine if we're reloading after a crash or a forced-restart
+   * - restore window state
+   * - restart downloads
+   * Set up event listeners for this window's tabs
+   * @param aWindow
+   *        Window reference
+   */
+  onLoad: function sss_onLoad(aWindow) {
+    // ignore non-browser windows
+    if (aWindow.document.documentElement.getAttribute("windowtype") !=
+      "navigator:browser") {
+      return;
+    }

odd wrapping here, we're not hitting the 80 char limit lots of places, so don't worry about it here
kill the extra brackets

 
+    // perform additional initialization when the first window is loading
+    if (this._loadState == STATE_STOPPED) {
+      // delete the initial state if the user doesn't want to restore it
+      // (since this might delay startup notably, don't set mLoadState before
+      //  this or we might get a corrupted session if Firefox crashes on
+      //  startup)
+      if (this._initialState && !(this._lastSessionCrashed ? this._doRecoverSession() : this._doResumeSession())) {
+        delete this._initialState;
+      }
+      if (this._getPref("resume_session_once", PREF_RESUME_SESSION_ONCE)) {
+        this._prefBranch.setBoolPref("resume_session_once", false);
+      }

as discussed, we need to abstract this out via methods for external callers to hide the impl detail.

+      this._loadState = STATE_RUNNING;
+      this._lastSaveTime = Date.now();

+      // don't save during the first five seconds
+      // (until most of the pages have been restored)
+      this.saveStateDelayed(aWindow, 5000);

make this ten or fiften seconds, even, just to be safe?

 
+    var browser = aWindow.getBrowser();
+    var tabpanels = browser.mPanelContainer;

s/browser/tabbrowser

+    // add tab change listeners to all already existing tabs
+    for (var i = 0; i < tabpanels.childNodes.length; i++) {
+      this.onTabAdd(aWindow, tabpanels.childNodes[i], true);
+    }
+    // notification of tab add/remove/selection
+    // http://developer.mozilla.org/en/docs/Extension_Code_Snippets:Tabbed_Browser
+    // - to be replaced with events from bug 322898
+    tabpanels.addEventListener("DOMNodeInserted", this.onTabAdd_proxy, false);
+    tabpanels.addEventListener("DOMNodeRemoved", this.onTabRemove_proxy, false);
+    tabpanels.addEventListener("select", this.onTabSelect_proxy, false);
+    
+    browser.addEventListener("TabClose", this.onTabClose_proxy, false);
+  },


file a followup bug on killing this code in favour of listening for TabOpen once that lands (should be later this week/early next)

+  /**
+   * On window close...
+   * - remove event listeners from tabs
+   * - save all window data
+   * @param aWindow
+   *        Window reference
+   */
+  onClose: function sss_onClose(aWindow) {
+    if (aWindow.document.documentElement.getAttribute("windowtype") != "navigator:browser") {
+      return;
+    }
+    
+    var browser = aWindow.getBrowser();
+    var tabpanels = browser.mPanelContainer;
+    
+    for (var i = 0; i < tabpanels.childNodes.length; i++) {
+      this.onTabRemove(aWindow, tabpanels.childNodes[i], true);
+    }
+    tabpanels.removeEventListener("DOMNodeInserted", this.onTabAdd_proxy, false);
+    tabpanels.removeEventListener("DOMNodeRemoved", this.onTabRemove_proxy, false);
+    tabpanels.removeEventListener("select", this.onTabSelect_proxy, false);
+    
+    browser.removeEventListener("TabClose", this.onTabClose_proxy, false);

do we need to remove event listeners like this?  I'm almost certain we don't need to.


+/* ........ nsISessionStore API .............. */
+
+// This API is terribly rough. TODO:
+// * figure out what functionality we really need
+// * figure out how to name the functions

I've skipped this part, since we haven't answered these questions yet, we want to get the functionality in first



+  /**
+   * Get an object that is a serialized representation of a History entry
+   * Used for data storage
+   * @param aEntry
+   *        nsISHEntry instance
+   * @returns object
+   */
+  _serializeHistoryEntry: function sss_serializeHistoryEntry(aEntry) {
+    var entry = { uri: aEntry.URI.spec, Child: [] };

use URL, not URI, if its not an nsIURI.  But I'd just save the nsIURI for later since then we don't have to parse strings in _checkPrivacyLevel etc, (we try to use nsIURI.schemeIs() consistently)


+    try {
+      var prefPostdata = this._getPref("postdata", PREF_POSTDATA);
+      if (prefPostdata && aEntry.postData && this._checkPrivacyLevel(entry.uri)) {
+        aEntry.postData.QueryInterface(Ci.nsISeekableStream).
+                        seek(Ci.nsISeekableStream.NS_SEEK_SET, 0);
+        var stream = Cc["@mozilla.org/scriptableinputstream;1"].
+                       createInstance(Ci.nsIScriptableInputStream);
+        stream.init(aEntry.postData);
+        // Warning: LiveHTTPHeaders crashes around here!
+        var postdata = stream.read(stream.available());
+        if (prefPostdata == -1 || postdata.replace(/^(Content-.*\r\n)+(\r\n)*/, "").length <= prefPostdata) {
+          entry.postdata = postdata;
+        }
+        //zeniko: should we close the stream here (since we never really opened it)?

pretty sure yes

+      }
+    }
+    catch (ex) { debug(ex); } // POSTDATA is tricky - especially since some extensions don't get it right
+    
+    if (!(aEntry instanceof Ci.nsISHContainer)) {
+      return entry;
+    }
+    
+    for (var i = 0; i < aEntry.childCount; i++) {
+      var child = aEntry.GetChildAt(i);
+      if (child) {
+        entry.Child.push(this._serializeHistoryEntry(child));
+      }
+      else { // to maintain the correct frame order, insert a dummy entry 
+        entry.Child.push({ uri: "about:blank" });
+      }

again, URL not URI when not an nsIURI (unless the API is weird)
+
+  /**
+   * Updates the current document's cache of user entered text data
+   * @param aPanel
+   *        TabPanel reference
+   * @param aTextarea
+   * @returns bool
+   */
+  _saveTextData: function sss_saveTextData(aPanel, aTextarea) {
+    var id = aTextarea.id ? "#" + aTextarea.id : aTextarea.name;
+    if (!id || !(aTextarea instanceof Ci.nsIDOMHTMLTextAreaElement ||
+                 aTextarea instanceof Ci.nsIDOMHTMLInputElement)) {
+      return false; // nothing to save
+    }

line up the || operaters here


+    // make sure that the current window is restored first
+    var ix = activeWindow ? windows.indexOf(activeWindow.__SSi || "") : -1;
+    if (ix > 0) {
+      total.unshift(total.splice(ix, 1)[0]);
+    }

redeclaring var ix :P


+  /**
+   * prevent the erasure of the cache after a crash by resetting its "dirty" flag
+   * see bug 105843 - code adapted from Danil Ivanov's "Cache Fixer" extension
+   */
+  restoreCache: function sss_restoreCache() {

so, what's the risk here?  sometimes the cache will be horked for real, so how do we detect this and really clear?

+/* ........ Disk Access .............. */
+
+  /**
+   * save state delayed by N ms
+   * marks window as dirty (i.e. data update can't be skipped)
+   * @param aWindow
+   *        Window reference
+   * @param aDelay
+   *        Milliseconds to delay
+   */
+  saveStateDelayed: function sss_saveStateDelayed(aWindow, aDelay) {
+    // interval until the next disk operation is allowed
+    var minimalDelay = this._lastSaveTime + this._interval - Date.now();
+    // if this interval has passed twice without disk access,
+    // the interval has probably changed - resetting
+    if (this._saveTimer && minimalDelay < -this._interval) {
+      this._saveTimer.cancel();
+      this._saveTimer = null;
+      aDelay = -1;
+    }

this math is confusing, if the interval has changed (via a pref change) we should be resetting the timer in the observer

how much does this matter with the write on a different thread?

+  /**
+   * delete session datafile and backup
+   */
+  _clearDisk: function sss_clearDisk() {
+    //zeniko: should we rethrow any exceptions?
+    try {
+      this._getSessionFile().remove(false);
+    }
+    catch (ex) { } // couldn't remove the file - what now?
+    try {
+      this._getSessionFile(true).remove(false);
+    }
+    catch (ex) { } // couldn't remove the file - what now?
+  },

so, if we're unable to clear privacy-related data, we should certainly tell the user somehow.  of course, this means they probably have a read-only profile, so stuff is seriously hosed elsewhere too

+/* ........ Auxiliary Functions .............. */
+
+  /**
+   * call a callback for all currently opened browser windows
+   * (might miss the most recent one)
+   * @param aFunc
+   *        Callback each window is passed to
+   */
+  _forEachBrowserWindow: function sss_forEachBrowserWindow(aFunc) {
+    var windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"].
+      getService(Ci.nsIWindowMediator);
+    var windowsEnum = windowMediator.getEnumerator("navigator:browser");
+    
+    while (windowsEnum.hasMoreElements()) {
+      var window = windowsEnum.getNext();
+      if (window.__SSi) {
+        aFunc.call(this, window);
+      }
+    }
+  },

why might this miss the most recent one?  fix the indenting please (getService aligns with Cc)

+  /**
+   * Returns most recent window
+   * @returns Window reference
+   */
+  _getMostRecentBrowserWindow: function sss_getMostRecentBrowserWindow() {
+    var windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"].
+      getService(Ci.nsIWindowMediator);
+    return windowMediator.getMostRecentWindow("navigator:browser");
+  },

do these get called often enough to cache windowMediator?

+  /**
+   * open a new browser window for a given session state
+   * called when restoring a multi-window session
+   * @param aState
+   *        Object containing session data
+   */
+  _openWindowWithState: function sss_openWindowWithState(aState) {
+    var argString =
+      Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
+    argString.data = "";
+    
+    //zeniko: why isn't it possible to set the window's dimensions here (as feature)?
+    var window = Cc["@mozilla.org/embedcomp/window-watcher;1"].
+                   getService(Ci.nsIWindowWatcher).
+                   openWindow(null, this._getPref("chromeURL", null, true), "_blank", "chrome,dialog=no,all", argString);
+    
+    window.__SS_state = aState;
+    window.addEventListener("load", this.openWindowWithState_onLoad_proxy, true);
+  },

the last arg can be null, so ditch the argString bits and just pass null


+  /**
+   * whether or not to resume session, if not recovering from a crash
+   * @returns bool
+   */
+  _doResumeSession: function sss_doResumeSession() {
+    // resume session if pref is set
+    return this._getPref("resume_session", PREF_RESUME_SESSION)
+      // handle resume-once for extension/plugin installs, etc
+      || this._getPref("resume_session_once", PREF_RESUME_SESSION_ONCE)
+      // also resume if the user has set the last visited page as startup page
+      || this._getPref("startup.page", 1, true) == 2;
+  },

This is kinda hard to read, actually.  Just explain the various cases we return true for in the comment above instead?

I'm not sure if that's the behaviour we want for the startup.page pref, either, but I'm open to arguments either way.

+  /**
+   * prompt user whether or not to restore the previous session,
+   * if the browser crashed
+   * @returns bool
+   */
+  _doRecoverSession: function sss_doRecoverSession() {
+    // if the prompt fails, recover anyway
+    var recover = true;
+    // allow extensions to hook in a more elaborate restore prompt
+    //zeniko: drop this when we're using our own dialog instead of a standard prompt
+    var dialogURI = this._getPref("restore_prompt_uri");
+    
+    try {
+      if (dialogURI) { // extension provided dialog 
+        var params = Cc["@mozilla.org/embedcomp/dialogparam;1"].
+                       createInstance(Ci.nsIDialogParamBlock);
+        // default to recovering
+        params.SetInt(0, 0);
+        Cc["@mozilla.org/embedcomp/window-watcher;1"].
+          getService(Ci.nsIWindowWatcher).
+          openWindow(null, dialogURI, "_blank", "chrome,modal,centerscreen,titlebar", params);
+        recover = params.GetInt(0) == 0;
+      }
+      else { // basic prompt with no options
+        var stringBundle = this._getStringBundle("chrome://browser/locale/sessionstore.properties");
+        var brand_short_name = this._getStringBundle("chrome://branding/locale/brand.properties").GetStringFromName("brandShortName");
+        var restore_title = brand_short_name + stringBundle.GetStringFromName("restore_title");
+        var restore_prompt = stringBundle.GetStringFromName("restore_prompt");

interCaps for var names, be consistent in how you get the strings here too, get the bundle, then the string as two lines, or all as one, but do it the same way (two lines is easier to break up)

restore_title is badly constructed, this needs to be a getFormattedString call, since brandShortName + string won't be right for all languages. (i.e. %s - Crash Recovery)

we really want to just use confirmEx here, not the if/else, and set the labels on the buttons to [Restore Session] [Cancel]

+        recover = Cc["@mozilla.org/embedcomp/prompt-service;1"].
+                    getService(Ci.nsIPromptService).
+                    confirm(null, restore_title, restore_prompt);
+      }
+    }
+    catch (ex) { } // if the prompt fails for some reason, recover anyway
+    
+    return recover;
+  },

we really want to just use confirmEx here, not the if/else, and set the labels on the buttons to [Restore Session] [Cancel]

+  /**
+   * whether the user wants to load any other page at startup
+   * (except the homepage) - needed for determining whether to overwrite the current tabs
+   * @returns bool
+   */
+  _isCmdLineEmpty: function sss_isCmdLineEmpty(aWindow) {
+    if (!aWindow.arguments) {
+      return true;
+    }
+    
+    var homepage = null;
+    switch (this._getPref("startup.page", 1, true)) {
+    case 0:
+      homepage = "about:blank";
+      break;
+    case 1:
+      try {
+        homepage = this._prefBranch.getComplexValue("startup.homepage", Ci.nsIPrefLocalizedString).data;
+      }
+      catch (ex) {
+        homepage = this._getPref("startup.homepage", "", true);
+      }
+      break;
+    case 2:
+      homepage = Cc["@mozilla.org/browser/global-history;2"].
+                   getService(Ci.nsIBrowserHistory).lastPageVisited;
+      break;
+    }
+    
+    for (var i = 0; i < aWindow.arguments.length; i++) {
+      var url = aWindow.arguments[i].split("\n")[0];
+      if (!url || url == homepage) {
+        aWindow.arguments.splice(i--, 1);
+      }
+    }
+    
+    return (aWindow.arguments.length == 0);
+  },

is this the right place to handle this?  shouldn't we just integrate this into the command line handling code and not restore the session if the user is launching with a URL?  the always restore case makes that kinda hard I guess

+  /**
+   * don't save sensitive data if the user doesn't want to
+   * (distinguishes between encrypted and non-encrypted sites)
+   * @param aURL
+   *        String URL
+   * @returns bool
+   */
+  _checkPrivacyLevel: function sss_checkPrivacyLevel(aURL) {
+    return this._getPref("privacy_level", PREF_PRIVACY_LEVEL) < (aURL.substr(0, 6) == "https:" ? PRIVACY_ENCRYPTED : PRIVACY_FULL);
+  },
+

this needs to take an nsIURI and check aURI.schemeIs("https") instead.  Because bz says so.

+  /**
+   * Convenience method to get localized string bundles
+   * @param aURI
+   * @returns nsIStringBundle
+   */
+  _getStringBundle: function sss_getStringBundle(aURI) {
+    var bundleService = Cc["@mozilla.org/intl/stringbundle;1"].
+                        getService(Ci.nsIStringBundleService);
+    return bundleService.createBundle(aURI,
+      Cc["@mozilla.org/intl/nslocaleservice;1"].
+        getService(Ci.nsILocaleService).getApplicationLocale());
+  },

I really don't like nesting xpcom calls in args like this, especially when its sort of half and half like this.  Just do the following:

  _getStringBundle: function sss_getStringBundle(aURI) {
    var bundleService = Cc["@mozilla.org/intl/stringbundle;1"].
                        getService(Ci.nsIStringBundleService);
    var appLocale = Cc["@mozilla.org/intl/nslocaleservice;1"].
                    getService(Ci.nsILocaleService).getApplicationLocale());
    return bundleService.createBundle(aURI, appLocale);
  },


+/* ........ Storage API .............. */
+
+  /**
+   * basic pref reader
+   * @param aName
+   * @param aDefault
+   * @param aUseRootBranch
+   */
+  _getPref: function sss_getPref(aName, aDefault, aUseRootBranch) {
+    if (!aUseRootBranch) {
+      aName = this._prefBranchName + aName;
+    }
+
+    var pb = this._prefBranch;
+    switch (pb.getPrefType(aName)) {
+    case pb.PREF_STRING:
+      return pb.getCharPref(aName);
+    case pb.PREF_BOOL:
+      return pb.getBoolPref(aName);
+    case pb.PREF_INT:
+      return pb.getIntPref(aName);
+    default:
+      return aDefault;
+    }
+  },

this should wrap the switch statement in a try/catch, and return aDefault if the get*Pref call throws (so if the pref doesn't have a default we don't fail)

+/* :::::::::: FileWriter :::::::::::::: */
+
+// a threaded file writer for somewhat better performance in the UI thread
+function FileWriter(aFile, aData) {
+  this._file = aFile;
+  this._data = aData;
+}
+
+FileWriter.prototype = {
+  run: function FileWriter_run() {
+    var stream = Cc["@mozilla.org/network/file-output-stream;1"].
+                   createInstance(Ci.nsIFileOutputStream);
+    stream.init(this._file, 0x02 | 0x08 | 0x20, 0600, 0);
+    var cvstream = Cc["@mozilla.org/intl/converter-output-stream;1"].
+                     createInstance(Ci.nsIConverterOutputStream);
+    cvstream.init(stream, "UTF-8", 0, Ci.nsIConverterInputStream.DEFAULT_REPLACEMENT_CHARACTER);
+    
+    cvstream.writeString(this._data.replace(/\n/g, this._getEOL()));
+    cvstream.flush();
+    cvstream.close();
+  },

this should really use nsISafeOutputStream like the update and search services

+  _getEOL: function FileWriter_getEOL() {
+    var OS = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime).OS;
+    return /win|os[\/_]?2/i.test(OS) ? "\r\n":/mac/i.test(OS)?"\r" : "\n";
+  }
+};

do we need to use OS-appropriate line endings considering that I don't think this is going to touched by people who don't have good editors?  if we do, skip this complex junk and just use the XUL preprocessor to make sure run() does the right thing, instead of runtime checks.  if not, use Unix line endings


+const IniObjectSerializer = {

var

+  // Object which results after traversing the path indicated in the section header
+  _getObjForHeader: function(aObj, aLine) {
+    var names = aLine.split("]")[0].substr(1).split(".");
+    
+    for (var i = 0; i < names.length; i++) {
+      var name = names[i];
+      if (!names[i]) {
+        throw new Error("Invalid header: [" + names.join(".") + "]!");

where does this get caught so we don't hork on startup?

+      }
+      if (/(\d+)$/.test(names[i])) {
+        names[i] = names[i].slice(0, -RegExp.$1.length);
+        var ix = parseInt(RegExp.$1) - 1;
+        aObj = aObj[names[i]] = aObj[names[i]] || [];
+        aObj = aObj[ix] = aObj[ix] || {};
+      }
+      else {
+        aObj = aObj[names[i]] = aObj[names[i]] || {};
+      }
+    }

no brackets for single lines

+    return aObj;
+  },
+
+  _setValueForLine: function(aObj, aLine) {
+    var ix = aLine.indexOf("=");
+    if (ix < 1) {
+      throw new Error("Invalid entry: " + aLine + "!");
+    }
+    
+    var key = this._trim(aLine.substr(0, ix));
+    var value = this._trim(aLine.substr(ix + 1));
+    if (value == "true" || value == "false") {
+      value = (value == "true");
+    }
+    else if (/^\d+$/.test(value)) {
+      value = parseInt(value);
+    }
+    else if (value.indexOf("%") > -1) {
+      value = decodeURI(value.replace(/%3B/gi, ";"));
+    }

no brackets for single lines (fix this anywhere else I missed!)

+/* :::::::: Service Registration & Initialization ::::::::::::::: */
+
+const SessionStoreModuleAndFactory = {
+
+/* ........ nsIModule .............. */

+/* ........ nsIFactory .............. */
+

gavin ran into leaks with nsIFactory and nsIModule implemented in the same js object with nsSearchService.js, please split these up


Index: locales/en-US/chrome/browser/sessionstore.properties
===================================================================
RCS file: locales/en-US/chrome/browser/sessionstore.properties
diff -N locales/en-US/chrome/browser/sessionstore.properties
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ locales/en-US/chrome/browser/sessionstore.properties        18 Apr 2006 05:17:08 -0000
@@ -0,0 +1,2 @@
+restore_title= - Crash Recovery

So I think this is the wrong string, for a placeholder to be revised later lets just call this Firefox - Session Restore (%s - Session Restore as noted in the getFormattedString comment

+restore_prompt=Restore previous session?

I think we want to provide something a bit more descriptive, depending on whether we do this automatically the first time after a crash or not.  Maybe something like this (probably too wordy, its 3 AM).

+----------------------------------------------------------------------+
| Firefox has recovered from a serious error.  One of the pages you    |
| were viewing may have caused the problem, so you may experience      |
| further problems if you continue with the previous browsing session. |
|                                                                      |
|                          [Restore Previous Session] [Open Home Page] |
+----------------------------------------------------------------------+

Comment 29

[Asaf Romano (gone)]

> +/* :::::::: The Service ::::::::::::::: */
> +
> +const SessionStoreService = {

> this should be a var instead

Neither a constant, nor a variable, please use |foo.prototype|, |function foo()| and |new foo()|. Example: http://lxr.mozilla.org/seamonkey/source/browser/components/search/nsSearchService.js#1672

Comment 30

[Simon Bünzli]

(In reply to comment #28)
> +  _xulAttributes: {},
> 
> if extensions are hooking this, it shouldn't be private, and let's make it an
> array so we can do nicer things with it where needed

If this isn't exposed in the API, why not treat it as implementation detail (and have it private)? Although since outside of sessionstore.js only the API will be available, having private members at all is somewhat redundant.

And as long as no special Array functionality is needed here, using it as a hash is easier (and xulAttributes[name] is nicer to test than xulAttributes.indexOf(name) > -1).

> +  // states for all currently opened windows
> +  _windows: {},
> 
> array instead

This is always used as a hash and never as a vector - which makes it a JS Object and no Array. Windows are identified by a string-identifier and iteration is done through "for (ix in _windows)" which might brake if the Array prototype is modified.

> +  _dirtyWindows: {},
> 
> array instead

As above: Object.

> kill the extra brackets

Is this a style requirement? Otherwise I'd rather keep them (one less source of worries should the clause ever be extended).

> +    var entry = { uri: aEntry.URI.spec, Child: [] };
> 
> use URL, not URI, if its not an nsIURI.  But I'd just save the nsIURI for later
> since then we don't have to parse strings in _checkPrivacyLevel etc, (we try to
> use nsIURI.schemeIs() consistently)

Note that we don't have a nsIURI in all places we call checkPrivacyLevel. And should you get keep the nsIURI here, make sure to convert it to a string later (which IMO is too much of a hassle).

> +        //zeniko: should we close the stream here (since we never really
> opened it)?
> 
> pretty sure yes

Will the POSTDATA stream be automatically opened again for all callers? I was under the impression that one of the reasons that LiveHTTPHeaders crashes Firefox at file uploads is that it closes that stream.

> +  _forEachBrowserWindow: function sss_forEachBrowserWindow(aFunc) {
> 
> why might this miss the most recent one?  fix the indenting please (getService
> aligns with Cc)

Don't know. This happens occasionally - and I'm sure there's already a bug for that.

> do these get called often enough to cache windowMediator?

About every ten seconds (_interval) and whenever a windows is closed. I used to cache it, but it's not necessary.

> I'm not sure if that's the behaviour we want for the startup.page pref, either,
> but I'm open to arguments either way.

Could also use a new value (3) for this. See bug 159357.

> we really want to just use confirmEx here, not the if/else, and set the labels
> on the buttons to [Restore Session] [Cancel]

As long as there is no dedicated dialog, I'd prefer offering extensions an option to hook in their own recover prompt.

> is this the right place to handle this?  shouldn't we just integrate this into
> the command line handling code and not restore the session if the user is
> launching with a URL?  the always restore case makes that kinda hard I guess

This isn't used to figure out whether to restore the session or not, but to know whether to additionally load more tabs or not (you wouldn't want to lose a crashed session just because you happened to click on a link just after the browser crashed). Of course, a notification of the command line handler what to do here would be nicer.

> +  _checkPrivacyLevel: function sss_checkPrivacyLevel(aURL) {
> +    return this._getPref("privacy_level", PREF_PRIVACY_LEVEL) <
> (aURL.substr(0, 6) == "https:" ? PRIVACY_ENCRYPTED : PRIVACY_FULL);
> +  },
> 
> this needs to take an nsIURI and check aURI.schemeIs("https") instead.  Because
> bz says so.

In that case you'll have to construct nsIURIs for when there is none available...

> +  _getPref: function sss_getPref(aName, aDefault, aUseRootBranch) {
> 
> this should wrap the switch statement in a try/catch, and return aDefault if
> the get*Pref call throws (so if the pref doesn't have a default we don't fail)

Can get*Pref really throw if getPrefType returned successfully?

> +        throw new Error("Invalid header: [" + names.join(".") + "]!");
> 
> where does this get caught so we don't hork on startup?

Always right where this method is called.

(In reply to comment #29)
> > +const SessionStoreService = {
> > this should be a var instead
> Neither a constant, nor a variable, please use |foo.prototype|, |function
> foo()| and |new foo()|. Example:

This is a singleton object. Why not treat it as such?

Comment 31

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #30)
> > +    var entry = { uri: aEntry.URI.spec, Child: [] };
> > 
> > use URL, not URI, if its not an nsIURI.  But I'd just save the nsIURI for
> > later since then we don't have to parse strings in _checkPrivacyLevel etc,
> > (we try to use nsIURI.schemeIs() consistently)
> 
> Note that we don't have a nsIURI in all places we call checkPrivacyLevel. And
> should you get keep the nsIURI here, make sure to convert it to a string later
> (which IMO is too much of a hassle).

See my mention of this in comment 8. All the callers I see either have an nsIURI, or have an easy way to check that the scheme == https (nsICookie's isSecure), so checkPrivacyLevel can just take an aIsHTTPS argument, passing in schemeIs("https") or isSecure.

> > is this the right place to handle this?  shouldn't we just integrate this
> > into the command line handling code and not restore the session if the user
> > is launching with a URL?  the always restore case makes that kinda hard I
> > guess
> 
> This isn't used to figure out whether to restore the session or not, but to
> know whether to additionally load more tabs or not (you wouldn't want to lose
> a crashed session just because you happened to click on a link just after the
> browser crashed). Of course, a notification of the command line handler what
> to do here would be nicer.

Again, see comment 8. It would be trivial to make nsBrowserContentHandler pass an extra argument to the window when it's sending defaultArgs, which you could then use to see if there is anything else to load.

> > +  _checkPrivacyLevel: function sss_checkPrivacyLevel(aURL) {
> > +    return this._getPref("privacy_level", PREF_PRIVACY_LEVEL) <
> > (aURL.substr(0, 6) == "https:" ? PRIVACY_ENCRYPTED : PRIVACY_FULL);
> > +  },
> > 
> > this needs to take an nsIURI and check aURI.schemeIs("https") instead.
> > Because bz says so.
> 
> In that case you'll have to construct nsIURIs for when there is none
> available...

I don't think this is true, see comment above.

Comment 32

[Dietrich Ayala (:dietrich)]

Attachment: trunk patch w/ fixes for mconnor review

> +
> +Always Resume
> +XXXX
> +
> +*/
> 
> fill in the XXXX

fixed

> and make sure this works with doxygen-based systems

doxygen doesn't seem to catch classes defined using either of these:

myClass function(){
}
myClass.prototype = {
  myMethod: function(){}
}

var myClass = {
  myMethod: function(){}
}

i tried a perl script called js2doxy, but that choked too. are we currently using doxygen to gen docs from code like this?

> 
> +/* :::::::: Pref Defaults :::::::::::::::::::: */
> +
> +// minimal interval between two save operations (in milliseconds)
> +const PREF_INTERVAL = 10000;
> +
> +// maximum number of closed windows remembered
> +const PREF_MAX_WINDOWS_UNDO = 10;
> +
> +// maximum number of closed tabs remembered (per window)
> +const PREF_MAX_TABS_UNDO = 10;
> +
> +// maximal amount of POSTDATA to be stored (in bytes, -1 = all of it)
> +const PREF_POSTDATA = 0;
> +
> +// on which sites to save text data, POSTDATA and cookies
> +// (0 = everywhere, 1 = unencrypted sites, 2 = nowhere)
> +const PREF_PRIVACY_LEVEL = PRIVACY_ENCRYPTED;
> +
> +// resume the current session at startup (otherwise just recover)
> +const PREF_RESUME_SESSION = false;
> +
> +// resume the current session at startup just this once
> +const PREF_RESUME_SESSION_ONCE = false;
> 
> these are defaults, can you call that out in the const names?  makes the code
> more obvious

fixed

> +/* :::::::: The Service ::::::::::::::: */
> +
> +const SessionStoreService = {
> 
> this should be a var instead

fixed per mano's suggestion. i had to do some rejiggering to get external references to the service working (eg: heavy use of closures and Function.apply()). is there a cleaner way to go about this?

(most of the items didn't need exposure via the public api, which is why i didn't go that route, and just QI to it).

> +  _prefBranchName: "sessionstore.",
> 
> there's no need for this, just getPref with sessionstore.foo, appending strings
> when not necessary sucks

fixed

> +  _observing: [
> this can be a const as well, it doesn't change afaict
> again, doesn't need to be part of the object if its not changing
> and again (fix indenting)
> and again!

fixed

> +  // xul:tab attributes to (re)store (extensions might want to hook in here)
> +  _xulAttributes: {},
> 
> if extensions are hooking this, it shouldn't be private, and let's make it an
> array so we can do nicer things with it where needed

Ok, made it public and an array. will investigate how to best expose it as part of 332774.

> +  // set default load state
> +  _loadState: STATE_STOPPED,
> +
> +  // minimal interval between two save operations (in milliseconds)
> +  _interval: PREF_INTERVAL,
> +
> +  // in milliseconds (usually Date.now())
> +  _lastSaveTime: 0, 
> 
> the comment is a bit confusing, it sort of implies that the value will be
> Date.now()

fixed

> +  // states for all currently opened windows
> +  _windows: {},
> 
> array instead
> 
> +  // storage for Undo Close Window
> +  _closedWindows: [],
> +
> +  // in case the last closed window ain't a navigator:browser one
> +  _lastWindowClosed: null,
> +
> +  // not-"dirty" windows usually don't need to have their data updated
> +  _dirtyWindows: {},
> 
> array instead

note zeniko's comments WRT this. what's the reviewer-approved way to do hashes of custom objects?

> +  // flag all windows as dirty
> +  _dirty: false,
> 
> so setting this to true adds all windows to dirty? or we just use all windows
> instead of _dirtyWindows if this is true?

When saving, session data is first collected for windows that are marked as dirty in _dirtyWindows.
The _dirty flag says whether data should be collected for all windows.

> +/* ........ Global Event Handlers .............. */
> +
> +  /**
> +   * Initialization, called upon profile-after-change notification 
> +   */
> +  _init: function sss_init() {
> +    this._prefBranch =
> +      Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService).
> +                                               getBranch("browser.");
> +    this._prefBranch.QueryInterface(Ci.nsIPrefBranch2);
> +
> +    // allow disabling by extensions that replace this functionality
> +    if (!this._getPref("enabled", true)) {
> +      return;
> +    }
> 
> followup to the other browserglue comment, we should only load the component if
> this pref is set to true :)

moved the pref check to browserglue

> +  /**
> +   * Called on application shutdown, after notifications:
> +   * quit-application-granted, quit-application
> +   */
> +  _uninit: function sss_uninit() {
> +    if (this._doResumeSession()) { // save all data for session resuming 
> +      this.saveState(true);
> +    }
> +    else { // discard all session related data 
> +      this._clearDisk();
> +    }
> +    
> +    // remove observers
> +    var observerService = 
> +      Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
> +
> +    this._observing.forEach(function(aTopic) {
> +      observerService.removeObserver(this, aTopic);
> +    }, this);
> +
> +    this._prefBranch.removeObserver("", this);
> +  },
> 
> don't worry about removing observers if we're quitting, and the prefbranch call
> is wrong now anyway...

removed

> +  /**
> +   * Handle notifications
> +   */
> +  observe: function sss_observe(aSubject, aTopic, aData) {
> +    var observerService = 
> +      Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
> +
> +    switch (aTopic) {
> +    // on app startup, register observer for when profile is completely loaded
> +    // alternative would be to init service in browserGlue instead of hooking
> +    // in via catmanager
> +    case "app-startup": 
> +      observerService.addObserver(this, "profile-after-change", false);
> +      break;
> +    // after profile loaded, start up service activity
> +    case "profile-after-change":
> +      observerService.removeObserver(this, aTopic);
> +      this._init();
> +      break;
> 
> you want to observe final-ui-startup here.  It'd also probably just be easier
> to init from browserglue, since that's one less observer to deal with, and
> that's why browserglue exists.  Otherwise we're at least reading this file
> before the EM restart, and possibly unnecessarily

removed the observer, made init() public, and moved initialization to browserglue.

> +    case "timer-callback": // timer call back for delayed saving
> +      this._saveTimer = null;
> +      this.saveStateDelayed(null, -1);
> +    }
> +  },
> 
> its not strictly necessary, but add the break at the end so people don't screw
> up if they add cases in the future

fixed

> +  onLoad: function sss_onLoad(aWindow) {
> +    // ignore non-browser windows
> +    if (aWindow.document.documentElement.getAttribute("windowtype") !=
> +      "navigator:browser") {
> +      return;
> +    }
> 
> odd wrapping here, we're not hitting the 80 char limit lots of places, so don't
> worry about it here
> kill the extra brackets

fixed

> +    // perform additional initialization when the first window is loading
> +    if (this._loadState == STATE_STOPPED) {
> +      // delete the initial state if the user doesn't want to restore it
> +      // (since this might delay startup notably, don't set mLoadState before
> +      //  this or we might get a corrupted session if Firefox crashes on
> +      //  startup)
> +      if (this._initialState && !(this._lastSessionCrashed ?
> this._doRecoverSession() : this._doResumeSession())) {
> +        delete this._initialState;
> +      }
> +      if (this._getPref("resume_session_once", PREF_RESUME_SESSION_ONCE)) {
> +        this._prefBranch.setBoolPref("resume_session_once", false);
> +      }
> 
> as discussed, we need to abstract this out via methods for external callers to
> hide the impl detail.

are you saying that the prefs part should be abstracted away into methods? eg, add resume() and resumeOnce() methods to nsISessionStore intead of having other components/code modify the prefs?

> +      this._loadState = STATE_RUNNING;
> +      this._lastSaveTime = Date.now();
> 
> +      // don't save during the first five seconds
> +      // (until most of the pages have been restored)
> +      this.saveStateDelayed(aWindow, 5000);
> 
> make this ten or fiften seconds, even, just to be safe?

changed to ten. can tweak later if problems arise.

> +    var browser = aWindow.getBrowser();
> +    var tabpanels = browser.mPanelContainer;
> 
> s/browser/tabbrowser

fixed

> +    // add tab change listeners to all already existing tabs
> +    for (var i = 0; i < tabpanels.childNodes.length; i++) {
> +      this.onTabAdd(aWindow, tabpanels.childNodes[i], true);
> +    }
> +    // notification of tab add/remove/selection
> +    //
> http://developer.mozilla.org/en/docs/Extension_Code_Snippets:Tabbed_Browser
> +    // - to be replaced with events from bug 322898
> +    tabpanels.addEventListener("DOMNodeInserted", this.onTabAdd_proxy, false);
> +    tabpanels.addEventListener("DOMNodeRemoved", this.onTabRemove_proxy,
> false);
> +    tabpanels.addEventListener("select", this.onTabSelect_proxy, false);
> +    
> +    browser.addEventListener("TabClose", this.onTabClose_proxy, false);
> +  },
> 
> 
> file a followup bug on killing this code in favour of listening for TabOpen
> once that lands (should be later this week/early next)

https://bugzilla.mozilla.org/show_bug.cgi?id=334839

> +  /**
> +   * On window close...
> +   * - remove event listeners from tabs
> +   * - save all window data
> +   * @param aWindow
> +   *        Window reference
> +   */
> +  onClose: function sss_onClose(aWindow) {
> +    if (aWindow.document.documentElement.getAttribute("windowtype") !=
> "navigator:browser") {
> +      return;
> +    }
> +    
> +    var browser = aWindow.getBrowser();
> +    var tabpanels = browser.mPanelContainer;
> +    
> +    for (var i = 0; i < tabpanels.childNodes.length; i++) {
> +      this.onTabRemove(aWindow, tabpanels.childNodes[i], true);
> +    }
> +    tabpanels.removeEventListener("DOMNodeInserted", this.onTabAdd_proxy,
> false);
> +    tabpanels.removeEventListener("DOMNodeRemoved", this.onTabRemove_proxy,
> false);
> +    tabpanels.removeEventListener("select", this.onTabSelect_proxy, false);
> +    
> +    browser.removeEventListener("TabClose", this.onTabClose_proxy, false);
> 
> do we need to remove event listeners like this?  I'm almost certain we don't
> need to.

removed

> +  /**
> +   * Get an object that is a serialized representation of a History entry
> +   * Used for data storage
> +   * @param aEntry
> +   *        nsISHEntry instance
> +   * @returns object
> +   */
> +  _serializeHistoryEntry: function sss_serializeHistoryEntry(aEntry) {
> +    var entry = { uri: aEntry.URI.spec, Child: [] };
> 
> use URL, not URI, if its not an nsIURI.  But I'd just save the nsIURI for later
> since then we don't have to parse strings in _checkPrivacyLevel etc, (we try to
> use nsIURI.schemeIs() consistently)

fixed

> +      }
> +    }
> +    catch (ex) { debug(ex); } // POSTDATA is tricky - especially since some
> extensions don't get it right
> +    
> +    if (!(aEntry instanceof Ci.nsISHContainer)) {
> +      return entry;
> +    }
> +    
> +    for (var i = 0; i < aEntry.childCount; i++) {
> +      var child = aEntry.GetChildAt(i);
> +      if (child) {
> +        entry.Child.push(this._serializeHistoryEntry(child));
> +      }
> +      else { // to maintain the correct frame order, insert a dummy entry 
> +        entry.Child.push({ uri: "about:blank" });
> +      }
> 
> again, URL not URI when not an nsIURI (unless the API is weird)

fixed

> +  /**
> +   * Updates the current document's cache of user entered text data
> +   * @param aPanel
> +   *        TabPanel reference
> +   * @param aTextarea
> +   * @returns bool
> +   */
> +  _saveTextData: function sss_saveTextData(aPanel, aTextarea) {
> +    var id = aTextarea.id ? "#" + aTextarea.id : aTextarea.name;
> +    if (!id || !(aTextarea instanceof Ci.nsIDOMHTMLTextAreaElement ||
> +                 aTextarea instanceof Ci.nsIDOMHTMLInputElement)) {
> +      return false; // nothing to save
> +    }
> 
> line up the || operaters here

fixed

> +    // make sure that the current window is restored first
> +    var ix = activeWindow ? windows.indexOf(activeWindow.__SSi || "") : -1;
> +    if (ix > 0) {
> +      total.unshift(total.splice(ix, 1)[0]);
> +    }
> 
> redeclaring var ix :P

fixed

> +  /**
> +   * prevent the erasure of the cache after a crash by resetting its "dirty"
> flag
> +   * see bug 105843 - code adapted from Danil Ivanov's "Cache Fixer" extension
> +   */
> +  restoreCache: function sss_restoreCache() {
> 
> so, what's the risk here?  sometimes the cache will be horked for real, so how
> do we detect this and really clear?

The consistency of push-back from Darin over the almost 5 year lifespan of 105843 would seem to indicate that this is a bad idea.

However, as you said previously, Darin would probably be the most qualified to speak to the actual level of risk in doing this.

> +/* ........ Disk Access .............. */
> +
> +  /**
> +   * save state delayed by N ms
> +   * marks window as dirty (i.e. data update can't be skipped)
> +   * @param aWindow
> +   *        Window reference
> +   * @param aDelay
> +   *        Milliseconds to delay
> +   */
> +  saveStateDelayed: function sss_saveStateDelayed(aWindow, aDelay) {
> +    // interval until the next disk operation is allowed
> +    var minimalDelay = this._lastSaveTime + this._interval - Date.now();
> +    // if this interval has passed twice without disk access,
> +    // the interval has probably changed - resetting
> +    if (this._saveTimer && minimalDelay < -this._interval) {
> +      this._saveTimer.cancel();
> +      this._saveTimer = null;
> +      aDelay = -1;
> +    }
> 
> this math is confusing, if the interval has changed (via a pref change) we
> should be resetting the timer in the observer

fixed - moved into the pref observer

> +  /**
> +   * delete session datafile and backup
> +   */
> +  _clearDisk: function sss_clearDisk() {
> +    //zeniko: should we rethrow any exceptions?
> +    try {
> +      this._getSessionFile().remove(false);
> +    }
> +    catch (ex) { } // couldn't remove the file - what now?
> +    try {
> +      this._getSessionFile(true).remove(false);
> +    }
> +    catch (ex) { } // couldn't remove the file - what now?
> +  },
> 
> so, if we're unable to clear privacy-related data, we should certainly tell the
> user somehow.  of course, this means they probably have a read-only profile, so
> stuff is seriously hosed elsewhere too

There's the rub, eh? Actually, if the profile was read-only, we wouldn't have been able to *save* any session data up to this point, so theoretically there should be nothing (from this session) to clear. Unless they made the profile read-only while the app was running, if that's possible.

> +/* ........ Auxiliary Functions .............. */
> +
> +  /**
> +   * call a callback for all currently opened browser windows
> +   * (might miss the most recent one)
> +   * @param aFunc
> +   *        Callback each window is passed to
> +   */
> +  _forEachBrowserWindow: function sss_forEachBrowserWindow(aFunc) {
> +    var windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"].
> +      getService(Ci.nsIWindowMediator);
> +    var windowsEnum = windowMediator.getEnumerator("navigator:browser");
> +    
> +    while (windowsEnum.hasMoreElements()) {
> +      var window = windowsEnum.getNext();
> +      if (window.__SSi) {
> +        aFunc.call(this, window);
> +      }
> +    }
> +  },
> 
> why might this miss the most recent one?  fix the indenting please (getService
> aligns with Cc)

fixed the indenting. see simon's comment about missing the most recent window.

there are some comments about it here:

https://bugzilla.mozilla.org/show_bug.cgi?id=276396

> +  /**
> +   * open a new browser window for a given session state
> +   * called when restoring a multi-window session
> +   * @param aState
> +   *        Object containing session data
> +   */
> +  _openWindowWithState: function sss_openWindowWithState(aState) {
> +    var argString =
> +     
> Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
> +    argString.data = "";
> +    
> +    //zeniko: why isn't it possible to set the window's dimensions here (as
> feature)?
> +    var window = Cc["@mozilla.org/embedcomp/window-watcher;1"].
> +                   getService(Ci.nsIWindowWatcher).
> +                   openWindow(null, this._getPref("chromeURL", null, true),
> "_blank", "chrome,dialog=no,all", argString);
> +    
> +    window.__SS_state = aState;
> +    window.addEventListener("load", this.openWindowWithState_onLoad_proxy,
> true);
> +  },
> 
> the last arg can be null, so ditch the argString bits and just pass null

fixed

> +  /**
> +   * whether or not to resume session, if not recovering from a crash
> +   * @returns bool
> +   */
> +  _doResumeSession: function sss_doResumeSession() {
> +    // resume session if pref is set
> +    return this._getPref("resume_session", PREF_RESUME_SESSION)
> +      // handle resume-once for extension/plugin installs, etc
> +      || this._getPref("resume_session_once", PREF_RESUME_SESSION_ONCE)
> +      // also resume if the user has set the last visited page as startup page
> +      || this._getPref("startup.page", 1, true) == 2;
> +  },
> 
> This is kinda hard to read, actually.  Just explain the various cases we return
> true for in the comment above instead?

fixed

> I'm not sure if that's the behaviour we want for the startup.page pref, either,
> but I'm open to arguments either way.

Regardless of whether it works or not, a value of 2 was *intended* to load the last-visited *page*, which is quite different than the previous session. I think I'd rather see a new value supported (eg: 3 for resume-session), or remove this.

We could swap out the browser.sessionstore.resume_session, instead using startup.page=3 for configuring "always resume".

> +  /**
> +   * prompt user whether or not to restore the previous session,
> +   * if the browser crashed
> +   * @returns bool
> +   */
> +  _doRecoverSession: function sss_doRecoverSession() {
> +    // if the prompt fails, recover anyway
> +    var recover = true;
> +    // allow extensions to hook in a more elaborate restore prompt
> +    //zeniko: drop this when we're using our own dialog instead of a standard
> prompt
> +    var dialogURI = this._getPref("restore_prompt_uri");
> +    
> +    try {
> +      if (dialogURI) { // extension provided dialog 
> +        var params = Cc["@mozilla.org/embedcomp/dialogparam;1"].
> +                       createInstance(Ci.nsIDialogParamBlock);
> +        // default to recovering
> +        params.SetInt(0, 0);
> +        Cc["@mozilla.org/embedcomp/window-watcher;1"].
> +          getService(Ci.nsIWindowWatcher).
> +          openWindow(null, dialogURI, "_blank",
> "chrome,modal,centerscreen,titlebar", params);
> +        recover = params.GetInt(0) == 0;
> +      }
> +      else { // basic prompt with no options
> +        var stringBundle =
> this._getStringBundle("chrome://browser/locale/sessionstore.properties");
> +        var brand_short_name =
> this._getStringBundle("chrome://branding/locale/brand.properties").GetStringFromName("brandShortName");
> +        var restore_title = brand_short_name +
> stringBundle.GetStringFromName("restore_title");
> +        var restore_prompt = stringBundle.GetStringFromName("restore_prompt");
> 
> interCaps for var names, be consistent in how you get the strings here too, get
> the bundle, then the string as two lines, or all as one, but do it the same way
> (two lines is easier to break up)

fixed

> restore_title is badly constructed, this needs to be a getFormattedString call,
> since brandShortName + string won't be right for all languages. (i.e. %s -
> Crash Recovery)

fixed

> +        recover = Cc["@mozilla.org/embedcomp/prompt-service;1"].
> +                    getService(Ci.nsIPromptService).
> +                    confirm(null, restore_title, restore_prompt);
> +      }
> +    }
> +    catch (ex) { } // if the prompt fails for some reason, recover anyway
> +    
> +    return recover;
> +  },
> 
> we really want to just use confirmEx here, not the if/else, and set the labels
> on the buttons to [Restore Session] [Cancel]

I've updated the prompt with these changes. I've left the hook in for now.

> +  /**
> +   * don't save sensitive data if the user doesn't want to
> +   * (distinguishes between encrypted and non-encrypted sites)
> +   * @param aURL
> +   *        String URL
> +   * @returns bool
> +   */
> +  _checkPrivacyLevel: function sss_checkPrivacyLevel(aURL) {
> +    return this._getPref("privacy_level", PREF_PRIVACY_LEVEL) <
> (aURL.substr(0, 6) == "https:" ? PRIVACY_ENCRYPTED : PRIVACY_FULL);
> +  },
> +
> 
> this needs to take an nsIURI and check aURI.schemeIs("https") instead.  Because
> bz says so.

fixed as gavin recommended - now takes bool aIsHTTPS as argument.

> I really don't like nesting xpcom calls in args like this, especially when its
> sort of half and half like this.  Just do the following:
> 
>   _getStringBundle: function sss_getStringBundle(aURI) {
>     var bundleService = Cc["@mozilla.org/intl/stringbundle;1"].
>                         getService(Ci.nsIStringBundleService);
>     var appLocale = Cc["@mozilla.org/intl/nslocaleservice;1"].
>                     getService(Ci.nsILocaleService).getApplicationLocale());
>     return bundleService.createBundle(aURI, appLocale);
>   },

fixed

> +/* ........ Storage API .............. */
> +
> +  /**
> +   * basic pref reader
> +   * @param aName
> +   * @param aDefault
> +   * @param aUseRootBranch
> +   */
> +  _getPref: function sss_getPref(aName, aDefault, aUseRootBranch) {
> +    if (!aUseRootBranch) {
> +      aName = this._prefBranchName + aName;
> +    }
> +
> +    var pb = this._prefBranch;
> +    switch (pb.getPrefType(aName)) {
> +    case pb.PREF_STRING:
> +      return pb.getCharPref(aName);
> +    case pb.PREF_BOOL:
> +      return pb.getBoolPref(aName);
> +    case pb.PREF_INT:
> +      return pb.getIntPref(aName);
> +    default:
> +      return aDefault;
> +    }
> +  },
> 
> this should wrap the switch statement in a try/catch, and return aDefault if
> the get*Pref call throws (so if the pref doesn't have a default we don't fail)

fixed

> +/* :::::::::: FileWriter :::::::::::::: */
> +
> +// a threaded file writer for somewhat better performance in the UI thread
> +function FileWriter(aFile, aData) {
> +  this._file = aFile;
> +  this._data = aData;
> +}
> +
> +FileWriter.prototype = {
> +  run: function FileWriter_run() {
> +    var stream = Cc["@mozilla.org/network/file-output-stream;1"].
> +                   createInstance(Ci.nsIFileOutputStream);
> +    stream.init(this._file, 0x02 | 0x08 | 0x20, 0600, 0);
> +    var cvstream = Cc["@mozilla.org/intl/converter-output-stream;1"].
> +                     createInstance(Ci.nsIConverterOutputStream);
> +    cvstream.init(stream, "UTF-8", 0,
> Ci.nsIConverterInputStream.DEFAULT_REPLACEMENT_CHARACTER);
> +    
> +    cvstream.writeString(this._data.replace(/\n/g, this._getEOL()));
> +    cvstream.flush();
> +    cvstream.close();
> +  },
> 
> this should really use nsISafeOutputStream like the update and search services

fixed

nsIConverterOutputStream doesn't support nsISafeOutputStream (it calls close(), but not finish() on the underlying stream), so instead i convert the string w/ nsIScriptableUnicodeConverter before writing.

> +  _getEOL: function FileWriter_getEOL() {
> +    var OS =
> Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime).OS;
> +    return /win|os[\/_]?2/i.test(OS) ? "\r\n":/mac/i.test(OS)?"\r" : "\n";
> +  }
> +};
> 
> do we need to use OS-appropriate line endings considering that I don't think
> this is going to touched by people who don't have good editors?  if we do, skip
> this complex junk and just use the XUL preprocessor to make sure run() does the
> right thing, instead of runtime checks.  if not, use Unix line endings

changed to use Unix line endings. regardless, this file is not really meant for editing manually. see 328162 for more discussion on this.

> +const IniObjectSerializer = {
> 
> var

fixed

> +    return aObj;
> +  },
> +
> +  _setValueForLine: function(aObj, aLine) {
> +    var ix = aLine.indexOf("=");
> +    if (ix < 1) {
> +      throw new Error("Invalid entry: " + aLine + "!");
> +    }
> +    
> +    var key = this._trim(aLine.substr(0, ix));
> +    var value = this._trim(aLine.substr(ix + 1));
> +    if (value == "true" || value == "false") {
> +      value = (value == "true");
> +    }
> +    else if (/^\d+$/.test(value)) {
> +      value = parseInt(value);
> +    }
> +    else if (value.indexOf("%") > -1) {
> +      value = decodeURI(value.replace(/%3B/gi, ";"));
> +    }
> 
> no brackets for single lines (fix this anywhere else I missed!)

I'd prefer to leave brackets around single lines. There's a bit of a discussion about this in the comments here:

http://dietrich.ganx4.com/blog/?p=226

The arguments for having brackets seem to always have more weight that the arguments against it (which are usually about aesthetics).

> +/* :::::::: Service Registration & Initialization ::::::::::::::: */
> +
> +const SessionStoreModuleAndFactory = {
> +
> +/* ........ nsIModule .............. */
> 
> +/* ........ nsIFactory .............. */
> +
> 
> gavin ran into leaks with nsIFactory and nsIModule implemented in the same js
> object with nsSearchService.js, please split these up

fixed

> Index: locales/en-US/chrome/browser/sessionstore.properties
> ===================================================================
> RCS file: locales/en-US/chrome/browser/sessionstore.properties
> diff -N locales/en-US/chrome/browser/sessionstore.properties
> --- /dev/null   1 Jan 1970 00:00:00 -0000
> +++ locales/en-US/chrome/browser/sessionstore.properties        18 Apr 2006
> 05:17:08 -0000
> @@ -0,0 +1,2 @@
> +restore_title= - Crash Recovery
> 
> So I think this is the wrong string, for a placeholder to be revised later lets
> just call this Firefox - Session Restore (%s - Session Restore as noted in the
> getFormattedString comment

Fixed. I had intended that to be a placeholder, but I've updated my placeholder with your placeholder :)

> +restore_prompt=Restore previous session?
> 
> I think we want to provide something a bit more descriptive, depending on
> whether we do this automatically the first time after a crash or not.  Maybe
> something like this (probably too wordy, its 3 AM).
> 
> +----------------------------------------------------------------------+
> | Firefox has recovered from a serious error.  One of the pages you    |
> | were viewing may have caused the problem, so you may experience      |
> | further problems if you continue with the previous browsing session. |
> |                                                                      |
> |                          [Restore Previous Session] [Open Home Page] |
> +----------------------------------------------------------------------+
> 
> 
> 

Changed. I'll request a Beltznering prior to check-in.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 219677 [details] [diff] [review]
trunk patch w/ fixes for mconnor review

>+  _updateCookieHosts: function sss_updateCookieHosts(aWindow) {
...
>+    function extractHosts(aEntry) {
>+      if (/^https?:\/\/(?:[^@\/\s]+@)?([\w.-]+)/.test(aEntry.url) &&
>+        !hosts[RegExp.$1] && _this._checkPrivacyLevel(_this._getURIFromString(aEntry.url))) {

I think this is meant to be:
_this._checkPrivacyLevel(_this._getURIFromString(aEntry.url).schemeIs("https"))

Comment 34

[Dietrich Ayala (:dietrich)]

Attachment: with gavin's fix

Comment 35

[Mike Connor [:mconnor]]

Comment on attachment 219704 [details] [diff] [review]
with gavin's fix

ok, this is good to go, I believe.  There's probably perf hits here and there, and room to improve greatly in places, but I'd rather iterate on perf and get more testing on functionality.

Comment 36

[Mike Connor [:mconnor]]

Comment on attachment 219704 [details] [diff] [review]
with gavin's fix

Talked to Ben, we're going to get this on the branch to get some perf numbers, and we'll follow up on perf wins later as needed/available.

Comment 37

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Is there a wiki available that explains the scriptable API to this new service?

I noted that there is supposed to be a way for third-party code to hook into the session restore service to allow extension-provided attributes to be restored alongside core attributes, but so far I don't see any documentation on how extensions are meant to do this.

Comment 38

[John Mellor (Jomel)]

The API's bug 332774, I don't think work's started on it yet but now this is being approved it might come soon. I'm eagerly awaiting it too...

I've made that bug a dependency of bug 328154 to help people find it.

(In reply to comment #37)
> Is there a wiki available that explains the scriptable API to this new service?

Comment 39

[John Mellor (Jomel)]

Shucks, didn't mean to set a dependency on this one too :/
Sorry for bugspam.

Comment 40

[Dietrich Ayala (:dietrich)]

Attachment: enables the service, traps init errors

This patch enables the service, catching problems in initialization. I'm going to see if I can get Tp results locally before getting this checked in though.

Comment 41

[Dietrich Ayala (:dietrich)]

Attachment: enables service, no longer writes in a new thread

The Tp crashes/freezes were happening when the session data was written to disk on a new thread. I could reproduce the crash while running either the DHTML test or the Tp test. I've not been able to reproduce any symptoms when writing on the main thread, even when running the Tp and DHTML tests simultaneously on different tabs.

This patch re-enables the service, removes the thread code, and fixes a couple of nits (a typo and some unneccesary comments).

Comment 42

[Dietrich Ayala (:dietrich)]

Attachment: same patch, w/ a bugfix

Comment 43

[Dietrich Ayala (:dietrich)]

Attachment: enables the service, but does not prompt or resume post-crash

Whereas crash restoration was previously enabled by default, you must now set the pref browser.sessionstore.resume_from_crash to true to get this functionality.

This is a workaround until we can make the necessary changes to Tinderbox to handle the restore prompt. When the Tinderbox issues are worked out, we'll change the default for this pref, so that crash restoration is enabled.

Comment 44

[Mike Connor [:mconnor]]

Comment on attachment 220561 [details] [diff] [review]
enables the service, but does not prompt or resume post-crash


+    else {
+      // initialize the session-restore service
+      var ssEnabled = true;
+      var prefBranch = Components.classes["@mozilla.org/preferences-service;1"].
+                                  getService(Components.interfaces.nsIPrefBranch);

leading period (since there's a period to line up!)

+      try {
+        ssEnabled = prefBranch.getBoolPref("browser.sessionstore.enabled");
+      } catch (ex) {}
+
+      if (ssEnabled) {
+        try {
+          var ss = Components.classes["@mozilla.org/browser/sessionstore;1"].
+                              getService(Components.interfaces.nsISessionStore);

period nit!



   _doRecoverSession: function sss_doRecoverSession() {
+    // do not prompt or resume, post-crash
+    if (!this._getPref("sessionstore.resume_from_crash", DEFAULT_RESUME_FROM_CRASH)) {
+      return false;
+    }

no extra braces

Comment 45

[Dietrich Ayala (:dietrich)]

Attachment: nits fixed

Comment 46

[Dietrich Ayala (:dietrich)]

Attachment: removes threads, adds no-prompt-as-default

Comment 47

[Dietrich Ayala (:dietrich)]

Attachment: session-restore patch for branch

Comment 48

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 221044 [details] [diff] [review]
session-restore patch for branch

Landed this patch on the branch.

Comment 49

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This made balsa on http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox (where assertions are fatal) turn orange since this code instantiates the download manager eagerly, and the download manager had not been instantiated during pageload tests before, and currently (post bug 326491) causes assertions on shutdown (before bug 326491 it caused leaks, as described in bug 315421 comment 0).

I'm using bug 315421 for the orange, since it's the same underlying problem.  For the branch, taking the original patch in bug 315421 is likely to be desirable if you don't want the leak numbers to go up, but that patch doesn't make sense on the trunk anymore post-bug 326491.

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This checkin caused significant increases in leaks on both branch and trunk; you may want to try testing with http://dbaron.org/mozilla/leak-monitor/ .

Comment 51

[Dietrich Ayala (:dietrich)]

I installed the extension on both branch and trunk builds, and received no alerts (related to this code anyway). Does the extension catch scenarios such as adding an anonymous function as an event listener inside an XPCOM service?

Note that mconnor in comment #28 said that all the removeEventListener() calls at window-close were probably unnecessary, so I removed them. Maybe we should re-evaluate that?

Comment 52

[Nickolay_Ponomarev]

>>Comment #28 From Mike Connor  2006-04-19 01:38 PDT
>I'd like to get darin's opinion on the safety of restoring cache like this
>before we land

>>Comment #32 From dietrich ayala  2006-04-24 15:54 PDT
>The consistency of push-back from Darin over the almost 5 year lifespan of
>105843 would seem to indicate that this is a bad idea.
>
>However, as you said previously, Darin would probably be the most qualified to
>speak to the actual level of risk in doing this.

Have you actually talked to darin? As far as I can see, the cache "fixer" change has landed.

Comment 53

[Dietrich Ayala (:dietrich)]

Attachment: Ts and bug fixes

Fixes a few restoration issues, and delays the loading of the session file until the first window loads. In my tests, this change showed about a 3% decrease in Ts:

http://numsum.com/spreadsheet/show/20421

Comment 54

[Dietrich Ayala (:dietrich)]

Comment on attachment 221044 [details] [diff] [review]
session-restore patch for branch

landed by gavin on 5/5

Comment 55

[Mike Connor [:mconnor]]

Comment on attachment 221277 [details] [diff] [review]
Ts and bug fixes


>   /**
>    * Called on application shutdown, after notifications:
>    * quit-application-granted, quit-application
>    */
>   _uninit: function sss_uninit() {
>+    if(this._saveTimer) {
>+      this._saveTimer.cancel();
>+      this._saveTimer = null;
>+    }

space after if



>       // give the tabbrowsers a chance to clear their histories first
>-      setTimeout(function() { _this.saveState(true); }, 0);
>+      var win = this._getMostRecentBrowserWindow();
>+      if (win) {
>+        win.setTimeout(function() { _this.saveState(true); }, 0);
>+      }
>+      else {
>+        this.saveState(true);
>+      }

no braces around single lines (I will break you of this sooner or later)

>     // perform additional initialization when the first window is loading
>     if (this._loadState == STATE_STOPPED) {
>+      // get file references
>+      this._sessionFile =
>+        Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties).
>+                                                    get("ProfD", Ci.nsILocalFile);

line up the periods

>     
>     var _this = this;
>-    aWindow.setTimeout( function(){
>-      _this.restoreHistory_proxy.apply(_this,
>-      [aWindow, winData.Tab, (aOverwriteTabs ? (parseInt(winData.selected) || 1) : 0), 0, 0]); }, 0);
>+    aWindow.setTimeout( function() {
>+      _this.restoreHistory_proxy(aWindow, winData.Tab, (aOverwriteTabs ? (parseInt(winData.selected) || 1) : 0), 0, 0);
>+      }, 0);
>   },

so, we tend to write this differently

var restoreHistoryFunc = function(self) {
  self.restoreHistory_proxy(aWindow, winData.Tab, (aOverwriteTabs ? (parseInt(winData.selected) || 1) : 0), 0, 0);
};
aWindow.setTimeout(restoreHistoryFunc, 0, this);


>         if (aCount < 10) {
>-          this.setTimeout(this.restoreHistory_proxy, 100, aWindow, aTabs, aSelectTab, aIx, aCount + 1);
>+          var _this = this;
>+          aWindow.setTimeout(function() {
>+            _this.restoreHistory_proxy(aWindow, aTabs, aSelectTab, aIx, aCount + 1);
>+            }, 100);
>           return;

see previous comment

Comment 56

[Steffen Wilberg]

Is this supposed to work yet? I've set browser.sessionstore.enabled to true, and sessionstore.ini in my profile gets updated as I browse, but upon a crash and a subsequent restart of Firefox nothing happens, Firefox just loads my homepage.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060507 BonEcho/2.0a1

Comment 57

[Mike Connor [:mconnor]]

Steffen, you need to flip the browser.sessionstore.resume_from_crash pref until we get tinderbox fixed.

Comment 58

[Steffen Wilberg]

Grr, using prefbranches is not nice when you search for "browser.sessionstore". :-)

380 if (this._getPref("sessionstore.resume_session_once", DEFAULT_RESUME_SESSION_ONCE)) {
381         this._prefBranch.setBoolPref("resume_session_once", false);
This has to be sessionstore.resume_session_once in line 381 as well. Otherwise the real pref is not set to false and Firefox keeps resuming the session.

Comment 59

[Dietrich Ayala (:dietrich)]

Attachment: update of patch 221277 - fixes nits, and comment #58

Comment 60

[Dietrich Ayala (:dietrich)]

Comment on attachment 221315 [details] [diff] [review]
update of patch 221277 - fixes nits, and comment #58

hold up, might be adding leak fixes to this. testing now.

Comment 61

[Dietrich Ayala (:dietrich)]

Attachment: Update again of 221277 - Ts fix + nits + leak fixes

Comment 62

[Dietrich Ayala (:dietrich)]

Attachment: timeless' patch for fixing leaks

Putting leak and Ts fixes in 2 different patches. Here's the leak changes only.

Comment 63

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 221326 [details] [diff] [review]
timeless' patch for fixing leaks

r=[e-mail]

check it in!

Comment 64

[timeless]

Comment on attachment 221326 [details] [diff] [review]
timeless' patch for fixing leaks

mozilla/browser/components/sessionstore/src/nsSessionStore.js 	1.6

Comment 65

[Dietrich Ayala (:dietrich)]

Attachment: Ts + fixes + nits + removal of cache-fixing hack

Comment 66

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix timer leak

With this, I can run the "leak tests" (such as they are) on trunk without anything unexpected leaking.

Comment 67

[Mike Schroepfer]

Still noting about a 6% drop in Ts on trunk:

http://build-graphs.mozilla.org/graph/query.cgi?tbox=pacifica&testname=startup&autoscale=1&size=&units=ms&ltype=&points=&showpoint=2006%3A05%3A08%3A18%3A30%3A34%2C531&avg=1&days=9

Not sure if it is session restore but it is one of a few changes in the timerange that caused the Ts regression.

Should we do a cycle with SR disabled to get another Ts run to see if it still has issues?  Are there any other obvious Ts fixes/causes in the code?

Comment 68

[Dietrich Ayala (:dietrich)]

The fix earlier today seemed to bring it down on some boxes:

http://numsum.com/spreadsheet/show/20658

The other tinderboxes on trunk showed little or no difference. The windows builds in particular showed almost zero change.

I'm looking for other efficiencies in the code. One option is to roll back to initializing the component on app startup. These (outdated) numbers show that initializing the component at startup was up to 5% faster. (caveat: tinderbox loves to shred my numbers to bits)

Comment 69

[Dietrich Ayala (:dietrich)]

Marking this fixed, and filing follow-ups for the Ts and leak issues.

Comment 70

[Phil Ringnalda (:philor)]

*** Bug 310261 has been marked as a duplicate of this bug. ***

Comment 71

[Boris Zbarsky [:bzbarsky]]

As regards XPCNativeWrappers:

> but it appears that only chrome:// scripts are "protected"

JS components are protected at least in some cases; see bug 337095.  Is the claim from comment 9 still true?  If so, PLEASE file bugs with steps to reproduce (possibly involving editing code of existing components).  We want to fix security bugs, not just work around them...

Comment 72

[Nickolay_Ponomarev]

(In reply to comment #71)
> As regards XPCNativeWrappers:
> > but it appears that only chrome:// scripts are "protected"
> JS components are protected at least in some cases; see bug 337095.  Is the
> claim from comment 9 still true?

All I claimed was that *documentation* implied the code in JS components was not protected. If they are supposed to be protected, please fix the docs (I prefer not to touch the article, since I don't quite understand the code that implements xpcnw). Were JS components "protected" in 1.8.0.0 too?

Comment 73

[James Ross]

In terms of what the code actually does, it is true that only chrome://package/ is flagged for protection. The documentation is to that extent correct.

Comment 74

[Boris Zbarsky [:bzbarsky]]

> implied the code in JS components was not protected.

Whether they're protected or not depends on where they're loaded from, as I understand it.  So it might depend on the component and the exact loading system we use.  Sadly, I'm not familiar with that end of things.  Benjamin, care to comment here?

> Were JS components "protected" in 1.8.0.0 too?

Components loaded from chrome:// had protected return values from methods the component calls, but not in params to component-implemented functions.  That was bug 337095.

For future reference, if the documentation is unclear, please file bugs on the documentation authors (in this case that would clearly be me); otherwise how will it become clear?  ;)

Comment 75

[Benjamin Smedberg]

> Whether they're protected or not depends on where they're loaded from, as I
> understand it.  So it might depend on the component and the exact loading

I have no clue. I believe that all JS components are loaded the same way (they certainly are on trunk).

Comment 76

[Boris Zbarsky [:bzbarsky]]

OK, I see what the deal is with JS components.  I'll update the documentation accordingly.

Comment 77

[Boris Zbarsky [:bzbarsky]]

Er, and see bug 345991 for the components situation.

Comment 78

[Boris Zbarsky [:bzbarsky]]

Documentation updated.  Please let me know what you think and if anything's unclear.