532150 - session file should be read on a background thread

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect, P2)

Comment 1

[Dietrich Ayala (:dietrich)]

will need some progress UI in case browser comes up before the session file read has completed.

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: session file should be read on a background thread

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 602135 [details] [diff] [review]
session file should be read on a background thread

read file async. fwiw, this is very similar to how mobile sessionstore reads the file in.

need to spit out a sessionstore-state-finalized event, and have nsSessionStore listen for that before attempting to read the string. otherwise if we have super large files, it's possible that nsSessionStore could try restoring before the session state data is available.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Dietrich Ayala (:dietrich) from comment #3)
> Comment on attachment 602135 [details] [diff] [review]
> session file should be read on a background thread
> 
> read file async. fwiw, this is very similar to how mobile sessionstore reads
> the file in.
> 
> need to spit out a sessionstore-state-finalized event, and have
> nsSessionStore listen for that before attempting to read the string.
> otherwise if we have super large files, it's possible that nsSessionStore
> could try restoring before the session state data is available.

Would we need to add something to nsSessionStartup indicating it's ready?
if (ss.ready) state = ss.state
else addObserver(...)

Or we can add another sessionType to default to (SESSION_UNKNOWN?) instead of an attribute.

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 602135 [details] [diff] [review]
session file should be read on a background thread

Review of attachment 602135 [details] [diff] [review]:
-----------------------------------------------------------------

As I mentioned above, I think we'll need some public indicator that we haven't finish reading the file yet.

::: browser/components/sessionstore/src/nsSessionStartup.js
@@ +294,2 @@
>      TelemetryStopwatch.start("FX_SESSION_RESTORE_READ_FILE_MS");
> +    // TODO: throw if callback not a function

It's internal, so I'm too worried about throwing, but I wouldn't tell you to take it out. I would add a comment saying that it must be a function though.

@@ -329,5 @@
> -                     createInstance(Ci.nsIConverterInputStream);
> -
> -      var fileSize = stream.available();
> -      if (fileSize > MAX_FILE_SIZE)
> -        throw "SessionStartup: sessionstore.js was not processed because it was too large.";

Don't forget to get rid of MAX_FILE_SIZE if you're not using it anymore (I don't remember there being any terribly compelling reason... I think we just needed _something_ there).

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Took Dietrich's patch, addressed Paul's comments and finished it. Seized the chance to refactor some session file handling code.

I'd actually like to ask Paul for review but I figured he might not really have time for that at the moment? Adding Dietrich as well for some feedback or maybe you're ok with reviewing the patch? (It's hard to find SR reviewers these days... :)

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Small fix.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 631335 [details] [diff] [review]
patch v2

Review of attachment 631335 [details] [diff] [review]:
-----------------------------------------------------------------

I just gave this a quick look while I had the chance. Overall I think it looks good - hopefully I'll get to give it a proper review soon.

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Before I can review, have you looked at other consumers of nsISessionStartup.doRestore/sessionType? There are a couple places where they're used and expected to reflect the truth but might get into a funky state if we haven't finished reading the file yet.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

Small but important correction to v2. We need to queue all windows passed to ss.init() and initialize them when the session is actually ready. Also, we shouldn't save the session on uninit if it hasn't been read, yet.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Paul O'Shannessy [:zpao] from comment #9)
> Before I can review, have you looked at other consumers of
> nsISessionStartup.doRestore/sessionType? There are a couple places where
> they're used and expected to reflect the truth but might get into a funky
> state if we haven't finished reading the file yet.

Very good point. So these are the consumers of sessionType:

http://dxr.lanedo.com/mozilla-central/browser/components/downloads/src/DownloadsStartup.js.html#l100
http://dxr.lanedo.com/mozilla-central/browser/components/nsBrowserGlue.js.html#l424

Both of them are called by "sessionstore-windows-restored" observers, which is emitted by ss.onLoad(aWindow), which happens after the session was loaded.

The only thing here is that nsBrowserGlue waits for "sessionstore-windows-restored" to open the default browser dialog. With really big sessions there might be some delay. Not sure if that's troubling?

Also, I remembered that the addon-sdk uses "sessionstore-windows-restored" to start initializing add-ons. This might be a tad delayed now that we read the file asynchronously.


The only consumer of doRestore is:

http://dxr.lanedo.com/mozilla-central/browser/components/nsBrowserContentHandler.js.html#l597

That code path seems to be entered when the users starts Firefox for the first time after an application update. Not sure when exactly nsBrowserContentHandler is initialized but seems like the session has probably not been read at this point?

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Tim Taubert [:ttaubert] from comment #11)
> (In reply to Paul O'Shannessy [:zpao] from comment #9)
> > Before I can review, have you looked at other consumers of
> > nsISessionStartup.doRestore/sessionType? There are a couple places where
> > they're used and expected to reflect the truth but might get into a funky
> > state if we haven't finished reading the file yet.
> 
> Very good point. So these are the consumers of sessionType:
> 
> http://dxr.lanedo.com/mozilla-central/browser/components/downloads/src/
> DownloadsStartup.js.html#l100
> http://dxr.lanedo.com/mozilla-central/browser/components/nsBrowserGlue.js.
> html#l424
> 
> Both of them are called by "sessionstore-windows-restored" observers, which
> is emitted by ss.onLoad(aWindow), which happens after the session was loaded.
> 
> The only thing here is that nsBrowserGlue waits for
> "sessionstore-windows-restored" to open the default browser dialog. With
> really big sessions there might be some delay. Not sure if that's troubling?

I don't think so.

> Also, I remembered that the addon-sdk uses "sessionstore-windows-restored"
> to start initializing add-ons. This might be a tad delayed now that we read
> the file asynchronously.

I think those are all probably ok. The meaning is still the same and for most real cases, it's going to be indiscernible.

> The only consumer of doRestore is:
> 
> http://dxr.lanedo.com/mozilla-central/browser/components/
> nsBrowserContentHandler.js.html#l597
> 
> That code path seems to be entered when the users starts Firefox for the
> first time after an application update. Not sure when exactly
> nsBrowserContentHandler is initialized but seems like the session has
> probably not been read at this point?

It's definitely possible that it hasn't been read yet. We could make that check wait for the new topic to fire if STATE_UNKNOWN.

Comment 13

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 634421 [details] [diff] [review]
patch v3

Review of attachment 634421 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, but I'd like Gavin (or another peer) to give it a quick look if possible.

Comment 14

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

gentle ping

Comment 15

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

Comment on attachment 634421 [details] [diff] [review]
patch v3

Sorry it took me so long to get to this. Now that we have OS.File, we should probably use that instead, and also get rid of the nsIFile use here? Or maybe it's best to land this as-is and file that as a followup, up to you.

It looks like this will break the caller of doRestore() in nsBrowserContentHandler.js? AFAICT it will always return false now, because when it is called (early in startup, before we open the first window), the session file (probably?) won't have been read yet, and so sessionType will be UNKNOWN_SESSION. I don't think we can easily just make that code wait - it needs to return defaultArgs synchronously. I have some local patches that remove that use of doRestore, but they depend on bug 699573 and probably bit rotten :(

There are add-on users of .sessionType and .doRestore() - we should audit them to see how they'll be impacted by this:
https://mxr.mozilla.org/addons/search?string=.doRestore
https://mxr.mozilla.org/addons/search?string=.sessionType

Comment 16

[David Teller [:Yoric] - still alive but not very active]

I will take a stab at porting this to OS.File.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Attachment: patch v3 ported to OS.File

Here is a version of the patch using OS.File instead of nsIFile/FileUtils/NetUtils. If we decide to go forward with this code, this will make bug 801137 and bug 794091 as duplicates of this bug.

I have not attempted to address Gavin's remark yet. Not sure exactly how we should proceed.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Attachment: patch v4

Attaching a version with a synchronous fallback for doRestore, sessionType and state. I haven't properly tested yet.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Attachment: patch v5

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Attachment: Additional tests

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 683609 [details] [diff] [review]
patch v5

Paul, since you reviewed the previous version, could you also review this one?

Let me list the main changes since the version you reviewed:
- everything uses OS.File and Task.jsm;
- the UNKNOWN startup state has been replaced by a synchronous fallback.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 683610 [details] [diff] [review]
Additional tests

+ tests

Comment 23

[Paul Rouget [:paul]]

Comment on attachment 683610 [details] [diff] [review]
Additional tests

wrong paul

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 683609 [details] [diff] [review]
patch v5

Review of attachment 683609 [details] [diff] [review]:
-----------------------------------------------------------------

This looks very cool. I'm giving feedback+ because there are a number of nits and questions as usual for big patches like this, and also because I want to give a second pass through the code but that doesn't need to block you on making the changes or replying to them.

--------

You'll need to change your two .jsm files to the new exporting style whereas the EXPORTED_SYMBOLS var itself and the exported objects need to be explicitly set on |this|


Not necessary to change now, but something to keep in mind as a possible follow-up, the _SessionFile and TaskUtils objects don't seem to have anything specific to sessionstore, so it'd be better to have them in a more generic location that can be used by other consumers (specially the TaskUtils one)

::: browser/components/sessionstore/src/Makefile.in
@@ +21,5 @@
>  EXTRA_JS_MODULES := \
>    DocumentUtils.jsm \
>    SessionStorage.jsm \
>    XPathGenerator.jsm \
> +  _SessionFile.jsm \

This naming scheme of having an underscore for an "internal" file is not commonly used. I don't mind it, but I wanna ask some folks who have more history around this code if that's fine with them

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +120,1 @@
>      SessionStoreInternal.init(aWindow);

it seems that SSI.init(aWin) already guarantees that initService will be called before, what's the advantage of calling it here too?

@@ +357,5 @@
>        this._prefBranch.getBoolPref("sessionstore.restore_pinned_tabs_on_demand");
>      this._prefBranch.addObserver("sessionstore.restore_pinned_tabs_on_demand", this, true);
>  
> +    // The session startup service might still be reading the file.
> +    if (gSessionStartup.sessionType == Ci.nsISessionStartup.UNKNOWN_SESSION)

nsISessionStartup.UNKNOWN_SESSION doesn't exist.. is it from a different patch?

but it's unfortunate that we need to use this pattern where the function may be either called sync or async. It can always lead to subtle bugs. Aren't there any of the promise patterns that could be used to unify this?

@@ -350,5 @@
> -    // get file references
> -    this._sessionFile = Services.dirsvc.get("ProfD", Ci.nsILocalFile);
> -    this._sessionFileBackup = this._sessionFile.clone();
> -    this._sessionFile.append("sessionstore.js");
> -    this._sessionFileBackup.append("sessionstore.bak");

it would be good to keep the file definitions in this module instead of _SessionFile if you can think of a clean way for that

@@ +522,5 @@
> +  init: function ssi_init(aWindow) {
> +    let self = this;
> +    if (!this._sessionInitialized) {
> +      this.initService();
> +    }

it would be better if initService() itself checked for _sessionInitialized and bailed out instead of counting on init to be aware of that

(I realize you didn't change this code, just moved it around, but taking the opportunity to clean that up would be good. Let me know if you've seen any reason not to do this)

@@ -3718,3 @@
>      TelemetryStopwatch.finish("FX_SESSION_RESTORE_SERIALIZE_DATA_MS");
>  
> -    Services.obs.notifyObservers(stateString, "sessionstore-state-write", "");

please keep these notificatione here in the public interface rather than moving them to the "internal, might change at any time" one.

The telemetry probes that you've moved there are fine to go there in order to make the finish() vs. cancel() handling straightforward

::: browser/components/sessionstore/src/_SessionFile.jsm
@@ +10,5 @@
> + * Implementation of all the disk I/O required by the session store.
> + * This is a private API, meant to be used only by the session store.
> + * It will change. Do not use it for any other purpose.
> + *
> + * Note that this module implicitly depends on one of two things:

This disclaimer is a bit scary, can you detail some more in the comment why does this module implicitly depend on that? Is there any way to write a reliable test for this, or is it race-condition dependent?
Certainly we cannot guarantee #2 because add-ons might hook into SessionStore and change how things work; so we'll need to rely on #1, and I'm a bit afraid that a future change in implementation here would drop property 1.

@@ +147,5 @@
> +      }
> +      let chan = NetUtil.newChannel(file);
> +      let stream = chan.open();
> +      text = NetUtil.readInputStreamToString(stream, stream.available(), {charset: "utf-8"});
> +    } catch(ex) {

in a similar fashion to the read() function, should we cancel the telemetry probe here for an unknown error?

::: browser/components/sessionstore/src/nsSessionStartup.js
@@ -43,2 @@
>  const STATE_RUNNING_STR = "running";
> -const MAX_FILE_SIZE = 100 * 1024 * 1024; // 100 megabytes

is the max file size enforcement gone?

@@ +47,5 @@
>  
>  function debug(aMsg) {
>    aMsg = ("SessionStartup: " + aMsg).replace(/\S{80}/g, "$&\n");
>    Services.console.logStringMessage(aMsg);
> +  dump(aMsg + "\n");

leftover dump() here and quite a few debug statements left through the file

@@ +125,5 @@
>  
> +    let prefBranch = Cc["@mozilla.org/preferences-service;1"].
> +                     getService(Ci.nsIPrefService).getBranch("browser.");
> +
> +    let doResumeSessionOnce = prefBranch.getBoolPref("sessionstore.resume_session_once");

while you're here, pleasee get rid of the prefBranch and use Services.prefs.getBoolPref with the full name of the pref on the same string. I want to kill all other cases like that..

@@ +290,5 @@
> +    debug("_ensureInitialized: " + this._initialState);
> +    if (this._initialized) {
> +      debug("ensureInitialized: Initialization is already complete");
> +      // Initialization is complete, nothing else to do
> +      return;

bad indentation

::: toolkit/components/telemetry/Histograms.json
@@ +2024,3 @@
>      "description": "Session restore: Time to JSON serialize session data (ms)"
>    },
>    "FX_SESSION_RESTORE_READ_FILE_MS": {

out of curiosity, do you expect this value to go up with this bug, for doing this work async? obviously it's a good change for getting rid of main-thread IO and improving snappy, just curious what you think the effect on the measured number will be.

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 683610 [details] [diff] [review]
Additional tests

Review of attachment 683610 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/test/unit/head.js
@@ +6,5 @@
> +
> +// Call a function once initialization of SessionStartup is complete
> +let afterSessionStartupInitialization =
> +  function afterSessionStartupInitialization(cb) {
> +  do_print("Waiting for session startup initialization");

missing one indent level

@@ +19,5 @@
> +  };
> +  let startup = Cc["@mozilla.org/browser/sessionstartup;1"].
> +    getService(Ci.nsIObserver);
> +  Services.obs.addObserver(startup, "final-ui-startup", false);
> +  Services.obs.addObserver(startup, "quit-application", false);

should you have a tail.js to remove these observers?

::: browser/components/sessionstore/test/unit/test_startup_nosession_fallback.js
@@ +4,5 @@
> +
> +// Test nsISessionStartup.sessionType in the following scenario:
> +// - no sessionstore.js;
> +// - the session store has not been loaded yet, so we have to trigger
> +//    synchronous fallback

what does fallback vs nofallback mean? (e.g. why does the fallback test have do_get_profile and the nofallback one doesnt?)

Comment 26

[David Teller [:Yoric] - still alive but not very active]

(In reply to :Felipe Gomes from comment #24)
> --------
> 
> You'll need to change your two .jsm files to the new exporting style whereas
> the EXPORTED_SYMBOLS var itself and the exported objects need to be
> explicitly set on |this|

done

> Not necessary to change now, but something to keep in mind as a possible
> follow-up, the _SessionFile and TaskUtils objects don't seem to have
> anything specific to sessionstore, so it'd be better to have them in a more
> generic location that can be used by other consumers (specially the
> TaskUtils one)

Yes, TaskUtils is very much on my todo list. _SessionFile not really, though. What do you think would deserve reuse?

> ::: browser/components/sessionstore/src/Makefile.in
> @@ +21,5 @@
> >  EXTRA_JS_MODULES := \
> >    DocumentUtils.jsm \
> >    SessionStorage.jsm \
> >    XPathGenerator.jsm \
> > +  _SessionFile.jsm \
> 
> This naming scheme of having an underscore for an "internal" file is not
> commonly used. I don't mind it, but I wanna ask some folks who have more
> history around this code if that's fine with them

I admit I picked it from devtools. It seems as good an idea as any.

> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +120,1 @@
> >      SessionStoreInternal.init(aWindow);
> 
> it seems that SSI.init(aWin) already guarantees that initService will be
> called before, what's the advantage of calling it here too?

Good point, removed.

> @@ +357,5 @@
> >        this._prefBranch.getBoolPref("sessionstore.restore_pinned_tabs_on_demand");
> >      this._prefBranch.addObserver("sessionstore.restore_pinned_tabs_on_demand", this, true);
> >  
> > +    // The session startup service might still be reading the file.
> > +    if (gSessionStartup.sessionType == Ci.nsISessionStartup.UNKNOWN_SESSION)
> 
> nsISessionStartup.UNKNOWN_SESSION doesn't exist.. is it from a different
> patch?
> 
> but it's unfortunate that we need to use this pattern where the function may
> be either called sync or async. It can always lead to subtle bugs. Aren't
> there any of the promise patterns that could be used to unify this?

Good spot. I was working from a pre-promise version of the patch by ttaubert and I had not noticed that we could simplify this. Working on it.

> @@ -350,5 @@
> > -    // get file references
> > -    this._sessionFile = Services.dirsvc.get("ProfD", Ci.nsILocalFile);
> > -    this._sessionFileBackup = this._sessionFile.clone();
> > -    this._sessionFile.append("sessionstore.js");
> > -    this._sessionFileBackup.append("sessionstore.bak");
> 
> it would be good to keep the file definitions in this module instead of
> _SessionFile if you can think of a clean way for that

Right now, I cannot think of anything clean.

> @@ +522,5 @@
> > +  init: function ssi_init(aWindow) {
> > +    let self = this;
> > +    if (!this._sessionInitialized) {
> > +      this.initService();
> > +    }
> 
> it would be better if initService() itself checked for _sessionInitialized
> and bailed out instead of counting on init to be aware of that
> 
> (I realize you didn't change this code, just moved it around, but taking the
> opportunity to clean that up would be good. Let me know if you've seen any
> reason not to do this)

Fixing.

> @@ -3718,3 @@
> >      TelemetryStopwatch.finish("FX_SESSION_RESTORE_SERIALIZE_DATA_MS");
> >  
> > -    Services.obs.notifyObservers(stateString, "sessionstore-state-write", "");
> 
> please keep these notificatione here in the public interface rather than
> moving them to the "internal, might change at any time" one.

Fixing.

> The telemetry probes that you've moved there are fine to go there in order
> to make the finish() vs. cancel() handling straightforward
> 
> ::: browser/components/sessionstore/src/_SessionFile.jsm
> @@ +10,5 @@
> > + * Implementation of all the disk I/O required by the session store.
> > + * This is a private API, meant to be used only by the session store.
> > + * It will change. Do not use it for any other purpose.
> > + *
> > + * Note that this module implicitly depends on one of two things:
> 
> This disclaimer is a bit scary, can you detail some more in the comment why
> does this module implicitly depend on that? Is there any way to write a
> reliable test for this, or is it race-condition dependent?
> Certainly we cannot guarantee #2 because add-ons might hook into
> SessionStore and change how things work; so we'll need to rely on #1, and
> I'm a bit afraid that a future change in implementation here would drop
> property 1.
> 
> @@ +147,5 @@
> > +      }
> > +      let chan = NetUtil.newChannel(file);
> > +      let stream = chan.open();
> > +      text = NetUtil.readInputStreamToString(stream, stream.available(), {charset: "utf-8"});
> > +    } catch(ex) {
> 
> in a similar fashion to the read() function, should we cancel the telemetry
> probe here for an unknown error?

I am not sure. The idea for this probe is also to determine how much time is wasted with the synchronous initializer, regardless of whether it succeeds or fails.

> 
> ::: browser/components/sessionstore/src/nsSessionStartup.js
> @@ -43,2 @@
> >  const STATE_RUNNING_STR = "running";
> > -const MAX_FILE_SIZE = 100 * 1024 * 1024; // 100 megabytes
> 
> is the max file size enforcement gone?

As per comment 5 above.

> 
> @@ +47,5 @@
> >  
> >  function debug(aMsg) {
> >    aMsg = ("SessionStartup: " + aMsg).replace(/\S{80}/g, "$&\n");
> >    Services.console.logStringMessage(aMsg);
> > +  dump(aMsg + "\n");
> 
> leftover dump() here and quite a few debug statements left through the file

Fixing.

> @@ +125,5 @@
> >  
> > +    let prefBranch = Cc["@mozilla.org/preferences-service;1"].
> > +                     getService(Ci.nsIPrefService).getBranch("browser.");
> > +
> > +    let doResumeSessionOnce = prefBranch.getBoolPref("sessionstore.resume_session_once");
> 
> while you're here, pleasee get rid of the prefBranch and use
> Services.prefs.getBoolPref with the full name of the pref on the same
> string. I want to kill all other cases like that..

Fixing.

> 
> @@ +290,5 @@
> > +    debug("_ensureInitialized: " + this._initialState);
> > +    if (this._initialized) {
> > +      debug("ensureInitialized: Initialization is already complete");
> > +      // Initialization is complete, nothing else to do
> > +      return;
> 
> bad indentation

Fixing.

> ::: toolkit/components/telemetry/Histograms.json
> @@ +2024,3 @@
> >      "description": "Session restore: Time to JSON serialize session data (ms)"
> >    },
> >    "FX_SESSION_RESTORE_READ_FILE_MS": {
> 
> out of curiosity, do you expect this value to go up with this bug, for doing
> this work async? obviously it's a good change for getting rid of main-thread
> IO and improving snappy, just curious what you think the effect on the
> measured number will be.

Actually, I am curious. Since the session store is quite large, the cost of synchronization between threads should be limited wrt the cost of actually writing to disk. By opposition to main thread I/O or to partially main thread I/O, we are decreasing the number of actual reads, so we might actually decrease the speed. Pending real-world benchmarks.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

(In reply to :Felipe Gomes from comment #24)
> ::: browser/components/sessionstore/src/_SessionFile.jsm
> @@ +10,5 @@
> > + * Implementation of all the disk I/O required by the session store.
> > + * This is a private API, meant to be used only by the session store.
> > + * It will change. Do not use it for any other purpose.
> > + *
> > + * Note that this module implicitly depends on one of two things:
> 
> This disclaimer is a bit scary, can you detail some more in the comment why
> does this module implicitly depend on that? Is there any way to write a
> reliable test for this, or is it race-condition dependent?
> Certainly we cannot guarantee #2 because add-ons might hook into
> SessionStore and change how things work; so we'll need to rely on #1, and
> I'm a bit afraid that a future change in implementation here would drop
> property 1.

(forgot to reply to this one)

Well, if necessary, I can enforce sequential I/O from here. There will be a small penalty, but that shouldn't be too much. Do you mind if we do this in a followup bug?

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Attachment: patch v6

Applied your feedback.

Comment 29

[David Teller [:Yoric] - still alive but not very active]

(In reply to :Felipe Gomes from comment #25)
> Comment on attachment 683610 [details] [diff] [review]
> Additional tests
> 
> Review of attachment 683610 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/sessionstore/test/unit/head.js
> @@ +6,5 @@
> > +
> > +// Call a function once initialization of SessionStartup is complete
> > +let afterSessionStartupInitialization =
> > +  function afterSessionStartupInitialization(cb) {
> > +  do_print("Waiting for session startup initialization");
> 
> missing one indent level

Ah, I have seen both styles on m-c. Adding 2 chars.

> 
> @@ +19,5 @@
> > +  };
> > +  let startup = Cc["@mozilla.org/browser/sessionstartup;1"].
> > +    getService(Ci.nsIObserver);
> > +  Services.obs.addObserver(startup, "final-ui-startup", false);
> > +  Services.obs.addObserver(startup, "quit-application", false);
> 
> should you have a tail.js to remove these observers?

Actually, startup will do that without my help. Adding cleanup will just get the test to fail.

> :::
> browser/components/sessionstore/test/unit/test_startup_nosession_fallback.js
> @@ +4,5 @@
> > +
> > +// Test nsISessionStartup.sessionType in the following scenario:
> > +// - no sessionstore.js;
> > +// - the session store has not been loaded yet, so we have to trigger
> > +//    synchronous fallback
> 
> what does fallback vs nofallback mean? (e.g. why does the fallback test have
> do_get_profile and the nofallback one doesnt?)

I have just renamed them to "sync" (formerly "fallback") and "async" (formerly "nofallback"). The async version waits until session startup init is complete, the async version doesn't and immediately checks sessionType, which forces the session startup to use the fallback synchronous initializer.

session_ tests have a do_get_profile (we need to install a sessionstore.js to the profile directory, for testing purposes), while nosession_ tests do not have a do_get_profile.

Comment 30

[David Teller [:Yoric] - still alive but not very active]

Attachment: Additional tests v2

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 689865 [details] [diff] [review]
patch v6

Review of attachment 689865 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/src/SessionStorage.jsm
@@ +6,5 @@
>  
>  const Cu = Components.utils;
>  
> +Cu.import("resource://gre/modules/Services.jsm", this);
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);

I don't think you need to change this, it's needed only for the objects that you need to export. (likewise in the two other .jsm)

::: browser/components/sessionstore/src/_SessionFile.jsm
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +let EXPORTED_SYMBOLS = ["_SessionFile"];

forgot the change in this file to this.EXPORTED_SYMBOLS

@@ +47,5 @@
> +XPCOMUtils.defineLazyGetter(this, "gEncoder", function () {
> +  return new TextEncoder();
> +});
> +
> +let _SessionFile = {

and this._SessionFile

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Attachment: patch v7

Applied feedback.

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=5b9484f6e474

Comment 34

[(no longer active)]

This seems to be pending review still...

Comment 35

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 689890 [details] [diff] [review]
Additional tests v2

Actually, that's a bookkeeping error. I only needed ttaubert or felipc's review.

Comment 36

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cab33f5bfbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/57bbbda0dedd

Comment 37

[(no longer active)]

Backed out for test failure: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=57bbbda0dedd

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d81cc19ff3f

Comment 38

[David Teller [:Yoric] - still alive but not very active]

That's strange, I cannot reproduce the test failure on TryServer. I will keep investigating.

Comment 39

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=0aabd8ae26e2

Comment 40

[David Teller [:Yoric] - still alive but not very active]

Attachment: Additional tests v3

Ah, got it. Trivial issue in a test, combined with a change of semantics in bug 810543.

Comment 41

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=2ce227ad9e36

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c99c3ab37f95
https://hg.mozilla.org/integration/mozilla-inbound/rev/1181ba5295b9

Comment 43

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c99c3ab37f95
https://hg.mozilla.org/mozilla-central/rev/1181ba5295b9

Comment 44

[Michael Kraft [:morac]]

I'm running into a problem with this patch.  I read the nsISessionStartup.sessionType variable during the "final-ui-startup" notification in my Session Manager addon.

Doing so breaks SessionStore's restore functionality since it calls _ensureInitialized() and apparently kills the async initialization.  This causes the previous session to be lost.

I'm not actually doing anything session related at that point, I just want to know what the sessionType is.

Is there some way around this?

Comment 45

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

(In reply to Michael Kraft [:morac] from comment #44)
> Doing so breaks SessionStore's restore functionality since it calls
> _ensureInitialized() and apparently kills the async initialization.  This
> causes the previous session to be lost.

That shouldn't be happening! Can you file a separate bug, and mark it as blocking this one?

Comment 46

[David Teller [:Yoric] - still alive but not very active]

Indeed, accessing nsISessionStartup.sessionType before the async init is complete kills the async init (that's by design, although of course this should be avoided for performance reasons). Now, this should not cause the previous session to be lost. If it does, this is a very serious bug.

Comment 47

[Michael Kraft [:morac]]

I filed bug 824107.  I'd look into this myself to find the cause, but I don't have the time at the moment.

Comment 48

[IU]

Ever since this landed, I'm getting the following in Error Console with my primary profile (even with all extensions disabled); however, I cannot find a way to reproduce so as to open a meaningful bug.  Does the following mean anything to anyone?:

SessionStore: [Exception... "Component returned failure code: 0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED) [nsIObjectInputStream.readObject]"  nsresult: "0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED)"  location: "JS frame :: resource:///modules/sessionstore/SessionStore.jsm :: ssi_deserializeHistoryEntry :: line 3352"  data: no]

Comment 49

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

No ideas offhand, but that is plenty meaningful enough to file a new bug - please do, and mark it blocking this one? Thank you!

Comment 50

[David Teller [:Yoric] - still alive but not very active]

@Gavin: Perhaps we should revert this bug until we have investigated bug 824107?

Comment 51

[Wayne Mery (:wsmwk)]

FWIW also breaks treeStyleTab https://github.com/piroor/treestyletab/issues/425

Comment 52

[Wayne Mery (:wsmwk)]

(In reply to Wayne Mery (:wsmwk) from comment #51)
> FWIW also breaks treeStyleTab
> https://github.com/piroor/treestyletab/issues/425

please ignore this erroneous comment. Failure is with 20121219 build.