session file should be read on a background thread

RESOLVED FIXED in Firefox 20

Status

()

Firefox
Session Restore
P2
normal
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: dietrich, Assigned: Yoric)

Tracking

(Blocks: 3 bugs, {perf})

Trunk
Firefox 20
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ts][Snappy:P3])

Attachments

(2 attachments, 10 obsolete attachments)

38.87 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
8.12 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Comment 1

8 years ago
will need some progress UI in case browser comes up before the session file read has completed.
Depends on: 532147
Whiteboard: [ts]
(Reporter)

Updated

8 years ago
Keywords: perf
Priority: -- → P2
(Reporter)

Updated

8 years ago
Blocks: 447581
Whiteboard: [ts] → [ts][Snappy]

Updated

6 years ago
Whiteboard: [ts][Snappy] → [ts][Snappy:P3]
(Reporter)

Updated

6 years ago
Blocks: 669034
(Reporter)

Comment 2

6 years ago
Created attachment 602135 [details] [diff] [review]
session file should be read on a background thread
(Reporter)

Comment 3

6 years ago
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.
Attachment #602135 - Flags: feedback?(paul)
(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 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).
Attachment #602135 - Flags: feedback?(paul) → feedback+
Created attachment 631332 [details] [diff] [review]
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... :)
Assignee: nobody → ttaubert
Attachment #602135 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #631332 - Flags: review?(paul)
Attachment #631332 - Flags: review?(dietrich)
Created attachment 631335 [details] [diff] [review]
patch v2

Small fix.
Attachment #631332 - Attachment is obsolete: true
Attachment #631332 - Flags: review?(paul)
Attachment #631332 - Flags: review?(dietrich)
Attachment #631335 - Flags: review?(paul)
Attachment #631335 - Flags: review?(dietrich)
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.
Attachment #631335 - Flags: feedback+
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.
Created attachment 634421 [details] [diff] [review]
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.
Attachment #631335 - Attachment is obsolete: true
Attachment #631335 - Flags: review?(paul)
Attachment #631335 - Flags: review?(dietrich)
Attachment #634421 - Flags: review?(paul)
(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?
(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 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.
Attachment #634421 - Flags: review?(paul)
Attachment #634421 - Flags: review?(gavin.sharp)
Attachment #634421 - Flags: review+
Blocks: 762867
gentle ping
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
Attachment #634421 - Flags: review?(gavin.sharp)
I will take a stab at porting this to OS.File.
Created attachment 671053 [details] [diff] [review]
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.
Attachment #671053 - Flags: review?(ttaubert)
Blocks: 810932
Depends on: 794091
Assignee: ttaubert → dteller
Created attachment 681642 [details] [diff] [review]
patch v4

Attaching a version with a synchronous fallback for doRestore, sessionType and state. I haven't properly tested yet.
Attachment #671053 - Attachment is obsolete: true
Attachment #671053 - Flags: review?(ttaubert)
Attachment #681642 - Flags: feedback?(ttaubert)
Created attachment 683609 [details] [diff] [review]
patch v5
Attachment #634421 - Attachment is obsolete: true
Attachment #681642 - Attachment is obsolete: true
Attachment #681642 - Flags: feedback?(ttaubert)
Created attachment 683610 [details] [diff] [review]
Additional tests
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.
Attachment #683609 - Flags: review?(paul)
Comment on attachment 683610 [details] [diff] [review]
Additional tests

+ tests
Attachment #683610 - Flags: review?(paul)

Comment 23

5 years ago
Comment on attachment 683610 [details] [diff] [review]
Additional tests

wrong paul
Attachment #683610 - Flags: review?(paul) → review?(paul)

Updated

5 years ago
Attachment #683609 - Flags: review?(paul) → review?(paul)
Attachment #683609 - Flags: review?(ttaubert)
Attachment #683609 - Flags: review?(paul)
Attachment #683609 - Flags: review?(felipc)
Attachment #683610 - Flags: review?(ttaubert)
Attachment #683610 - Flags: review?(paul)
Attachment #683610 - Flags: review?(felipc)
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.
Attachment #683609 - Flags: review?(felipc) → feedback+
Blocks: 716174
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?)
Attachment #683610 - Flags: review?(felipc) → feedback+
(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.
(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?
Created attachment 689865 [details] [diff] [review]
patch v6

Applied your feedback.
Attachment #683609 - Attachment is obsolete: true
Attachment #683609 - Flags: review?(ttaubert)
Attachment #689865 - Flags: review?(ttaubert)
Attachment #689865 - Flags: review?(felipc)
(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.
Created attachment 689890 [details] [diff] [review]
Additional tests v2
Attachment #683610 - Attachment is obsolete: true
Attachment #683610 - Flags: review?(ttaubert)
Attachment #689890 - Flags: review?(ttaubert)
Attachment #689890 - Flags: review?(felipc)
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
Attachment #689865 - Flags: review?(felipc) → review+
Attachment #689890 - Flags: review?(felipc) → review+
Created attachment 691730 [details] [diff] [review]
patch v7

Applied feedback.
Attachment #689865 - Attachment is obsolete: true
Attachment #689865 - Flags: review?(ttaubert)
Attachment #691730 - Flags: review+
Try: https://tbpl.mozilla.org/?tree=Try&rev=5b9484f6e474
Keywords: checkin-needed
This seems to be pending review still...
Keywords: checkin-needed
Comment on attachment 689890 [details] [diff] [review]
Additional tests v2

Actually, that's a bookkeeping error. I only needed ttaubert or felipc's review.
Attachment #689890 - Flags: review?(ttaubert)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cab33f5bfbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/57bbbda0dedd
Keywords: checkin-needed
Backed out for test failure: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=57bbbda0dedd

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d81cc19ff3f
That's strange, I cannot reproduce the test failure on TryServer. I will keep investigating.
Try: https://tbpl.mozilla.org/?tree=Try&rev=0aabd8ae26e2
Created attachment 693818 [details] [diff] [review]
Additional tests v3

Ah, got it. Trivial issue in a test, combined with a change of semantics in bug 810543.
Attachment #689890 - Attachment is obsolete: true
Attachment #693818 - Flags: review+
Try: https://tbpl.mozilla.org/?tree=Try&rev=2ce227ad9e36
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c99c3ab37f95
https://hg.mozilla.org/integration/mozilla-inbound/rev/1181ba5295b9
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c99c3ab37f95
https://hg.mozilla.org/mozilla-central/rev/1181ba5295b9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
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?
(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?
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.

Updated

5 years ago
Depends on: 824107
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

5 years ago
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]
No ideas offhand, but that is plenty meaningful enough to file a new bug - please do, and mark it blocking this one? Thank you!
@Gavin: Perhaps we should revert this bug until we have investigated bug 824107?
FWIW also breaks treeStyleTab https://github.com/piroor/treestyletab/issues/425
(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.
Depends on: 838552
You need to log in before you can comment on or make changes to this bug.