1147822 - [Session Restore] Add a format version number for sessionstore.js

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

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

We recently had issues when changing the format of sessionstore.js. These issues would have been less annoying if we had had a version number that would have let Firefox know beforehand that it cannot load a sessionstore.js using a format too recent, and that it should have fallen back to loading a backup.

Comment 1

[Fabian Furger]

I'm happy to work on this bug. Could you please provide some more information about what you had in mind?
- Is this a version number for the sessionstore format that is independent from the Firefox version number?
- So the issue appears by changing the sessionstore format without changing Firefox to handle the new format. I assume that would only arise during development? What about backward-compatibility (is a newer Firefox version able to load old sessionstore files)?
- How would Firefox know which sessionstore version(s) it can process?

Comment 2

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

For the moment, there is no numbering scheme for Session Restore. Most versions of Firefox do not change the format, so we do not want to tie the format version number to the version of Gecko/Firefox. Rather, we will want to change the number manually whenever we change the actual format.

The issue appears when downgrading Firefox, or when a change to Session Restore has been landed then backed out (backing things out is the kind of stuff that happens on a regular basis with Nightly), or possibly in the future when attempting to synchronize distinct versions of Firefox through Firefox Sync.

Newer versions of Firefox are expected to be able to load files from older versions. We may eventually give up on compatibility after several years.

The code of SessionRestore would need to be patched manually whenever it learns/unlearns a format.

Comment 3

[Fabian Furger]

So if I understand correctly, it would require the following changes:

- add a version number when writing the session to sessionstore.js and to upgrade backups
- add a preference(?) like `session_store_version`
- when loading sessionstore.js, compare the sessionstore.js version to the `session_store_version` and if `session_store_version` is lower, repeat the process with the upgrade backups

Is that correct? Feel free to assign the bug to me btw.

Comment 4

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

1. add a version number when writing the session to sessionstore.js and to upgrade backups

When writing the session, yes. You don't need to change anything to the backups.

2. add a preference(?) like `session_store_version`

Actually, a hardcoded constant in (I believe) SessionStore.jsm

3. when loading sessionstore.js, compare the sessionstore.js version to the `session_store_version` and if `session_store_version` is lower, repeat the process with the upgrade backups

`SessionFile.read` already has a mechanism for trying the next file if a file doesn't parse correctly. We just need to add "I don't understand this version" as an error case and use it.

Comment 5

[Fabian Furger]

Attachment: first-draft.patch

I've created a first draft but there are several things I'm not happy about. First, what I've done:

- SessionStore.jsm got a constant version number
- SessionStore.jsm got a function `getSessionStoreVersion()` that returns this number
- SessionStore.jsm function `getCurrentState(...)` returns an additional property `version` with the value of the constant version number

- SessionFile.jsm loads the SessionStore module
- SessionFile.jsm, after parsing a sessionstore file, checks whether the file is a sessionstore.js file rather than an upgrade backup (is that correct?), and whether the version number is too high. If it is, the file is skipped.

What I'm unhappy about:

- SessionFile.jsm probably shouldn't load the SessionStore module but how else does it find out whether it understands the version of the file?
- I'm not sure that the check I added in the loop that processes the load order is correct
- I'm not sure that I handle invalid versions well. Maybe we should remove the file with the invalid version?
- How can I test this?

Comment 6

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

Comment on attachment 8590165 [details] [diff] [review]
first-draft.patch

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

Good start. I'll answer the rest of your comments a bit later.

::: browser/components/sessionstore/SessionFile.jsm
@@ +209,5 @@
>          let path = this.Paths[key];
>          let startMs = Date.now();
>          let source = yield OS.File.read(path, { encoding: "utf-8" });
>          let parsed = JSON.parse(source);
> +        let parsedVersion = parseInt(parsed.version, 10);

let version = `parsed.version || 0` should be sufficient.

@@ +211,5 @@
>          let source = yield OS.File.read(path, { encoding: "utf-8" });
>          let parsed = JSON.parse(source);
> +        let parsedVersion = parseInt(parsed.version, 10);
> +        // Check whether version of the file can be read
> +        if (key === "clean" && (isNaN(parsedVersion) || parsedVersion > SessionStore.getSessionStoreVersion())) {

I don't understand why you check whether `key` is `"clean"`.

::: browser/components/sessionstore/SessionStore.jsm
@@ +10,5 @@
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  const Cr = Components.results;
>  
> +// Current version of the session store

// Current version of the format used by Session Restore.

@@ +317,5 @@
>    },
> +
> +  getSessionStoreVersion() {
> +    return SESSION_STORE_VERSION;
> +  },

get formatVersion() {
  ...
}

You'll probably also want

`true` if we are compatible with this format version, `false` otherwise
isFormatVersionUnderstood(version) {
  // ...
}

Comment 7

[Fabian Furger]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #6)
> @@ +211,5 @@
> >          let source = yield OS.File.read(path, { encoding: "utf-8" });
> >          let parsed = JSON.parse(source);
> > +        let parsedVersion = parseInt(parsed.version, 10);
> > +        // Check whether version of the file can be read
> > +        if (key === "clean" && (isNaN(parsedVersion) || parsedVersion > SessionStore.getSessionStoreVersion())) {
> 
> I don't understand why you check whether `key` is `"clean"`.
> 

You mentioned that we only apply the versioning to the sessionstore.js file and not the upgrade backups. `SessionFileInternal.read` loops over `SessionFileInternal.Paths.loadOrder`, checking files in descending priority. The entry "clean" in the loadOrder refers to "sessionstore.js", hence I check for key "clean".

Comment 8

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

(In reply to Fabian Furger from comment #5)
> I've created a first draft but there are several things I'm not happy about.
> First, what I've done:
> 
> - SessionStore.jsm got a constant version number
> - SessionStore.jsm got a function `getSessionStoreVersion()` that returns
> this number
> - SessionStore.jsm function `getCurrentState(...)` returns an additional
> property `version` with the value of the constant version number

Sounds good to me (with a few nits expressed above).

> - SessionFile.jsm loads the SessionStore module

I can't think of a better solution, as only SessionStore is expected to know its format.

> - SessionFile.jsm, after parsing a sessionstore file, checks whether the
> file is a sessionstore.js file rather than an upgrade backup (is that
> correct?), and whether the version number is too high. If it is, the file is
> skipped.

Unless I'm missing something, we don't care about it being an upgrade backup.

> What I'm unhappy about:
> 
> - SessionFile.jsm probably shouldn't load the SessionStore module but how
> else does it find out whether it understands the version of the file?

I agree it's not very nice, but I can't think of a better idea at the moment.

> - I'm not sure that the check I added in the loop that processes the load
> order is correct
> - I'm not sure that I handle invalid versions well. Maybe we should remove
> the file with the invalid version?

No, let's not take the initiative to remove files. They will be considered invalid and overwritten anyway.

> - How can I test this?

Comment 9

[Fabian Furger]

Attachment: second-version.patch

Thanks for your feedback. I've included this in an updated patch. I've removed access to the version number since the function to check version compatibility should be sufficient.

Comment 10

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

Comment on attachment 8590256 [details] [diff] [review]
second-version.patch

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

Looks good to me.

For testing purposes, you'll want to extend one of the Session Restore tests that checks we can fallback to a backup. In sessionstore.js, put a valid JSON file with a very large number as a version, then in previous.js put a valid JSON file with different content and a version of 1, check that it uses the second one. Repeat with a few variations (e.g. when previous.js also has a very large version number, or when it doesn't have a version number at all, etc.)

::: browser/components/sessionstore/SessionFile.jsm
@@ +212,5 @@
>          let parsed = JSON.parse(source);
> +        let parsedVersion = parsed.version || 0;
> +        // Check whether version of the file can be read
> +        if (!SessionStore.isFormatVersionCompatible(parsedVersion)) {
> +          // Skip sessionstore files that we don't understand

I believe that it would be a good idea to print something so that users have at least a small chance of understanding what's going on.

Something along the lines of:

Cu.reportError(`Cannot extract data from Session Restore file ${path}: it uses an invalid format ${parsedVersion}.`);

::: browser/components/sessionstore/SessionStore.jsm
@@ +11,5 @@
>  const Ci = Components.interfaces;
>  const Cr = Components.results;
>  
> +// Current version of the format used by Session Restore.
> +const SESSION_STORE_VERSION = 1;

Let's call this `FORMAT_VERSION`.

@@ +2216,5 @@
>        recentCrashes: this._recentCrashes
>      };
>  
>      let state = {
> +      version: SESSION_STORE_VERSION,

Let's call this field "sessionrestore:version". I suspect that putting "sessionrestore" in the name will eventually prove useful for debugging/forensics purposes.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #10)
> Let's call this field "sessionrestore:version". I suspect that putting
> "sessionrestore" in the name will eventually prove useful for
> debugging/forensics purposes.

Why? I think this field being stored in sessionstore.js is a pretty clear indicator it belongs to sessionstore. Starting a new naming convention now doesn't make sense to me.

Comment 12

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

Just a hunch that someday, we will have to debug a file in which someone else (some add-on?) will have injected a version number called "version".

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

We can't prepare for every thing add-on authors are or might be doing in the future, I wouldn't like to adjust the format we want to have with unnecessary annotations to guard against weird add-ons. We provide no official API anymore to tamper with the session data that's stored to disk so I don't think this will be an issue.

Comment 14

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

What's wrong with putting a clearer name?

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Well first, having property names with a colon isn't something we usually do. And second, why would we add a "sessionstore" namespace to a property that exists in the sessionstore data file? To me that is overly cautious and I'd rather limit the possibilities of tampering with our data even further than guarding against hypothetical misuses by add-ons.

Comment 16

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

Feel free to replace the colon with anything else. What's important here is to attach not just the format *version*, but also the *format* itself, to avoid accidents, help with debugging, help finding out answering the question "wth is that file I just opened", help with recovery, etc. Just as in just about every single file format on Earth.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Could call it "format_version" then.

Comment 18

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

So `format_version` and `format_name`?

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

I don't understand why we would need format_name? What else should a property in sessionstore.js refer to than the version of the file itself? It would be in sync with the FORMAT_VERSION constant in the JSM.

Comment 20

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

Let me put it this way: can you find any file format in existence that doesn't offer a header (or some other mechanism) to identify files that belong to this format?

Avi, Doc, Odf, Xml, Bash, Core Dump, Sqlite, Elf, Exe, etc. all have a magic number, shebang or equivalent that specifies the file *format*. This is useful as documentation, for debugging, recovery, building third-party tools, etc. Source code typically doesn't have that, but at least relies on a meaningful extension. Session Restore, on the other hand, has nothing. No meaningful extension, no header. You need expert knowledge to identify a Session Restore file from its contents, and I suspect that there are less than 10 of us who can do that. I want to remedy this with a trivial field.

Any of the following is fine for me:
- "sessionstore:version": 1
- format: "sessionstore", version: 1,
- format: {kind: "sessionstore", version: 1},
- ...

Comment 21

[Fabian Furger]

Attachment: third-version.patch

Sorry for the delay, I've been busy finishing up my bachelor thesis during the last days and weeks.

As you guys don't seem to really have reached a consensus on the naming, I'm leaving it as is at the moment ("sessionstore-version").

I've tried adding tests as outlined in comment 10 but I'm unable to create `previous.js`:

> 47 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_backup_recovery.js | Uncaught exception - at (unknown module):0 - Unix error 2 during operation open on file /tmp/tmpIAcQQm.mozrunner/sessionstore-backups/previous.js (No such file or directory)

This exception is raised when calling

> yield File.writeAtomic(Paths.cleanBackup, BACKUP_SOURCE);

I don't understand why, as the file's directory is created earlier by calling

> yield File.makeDir(Paths.backups);

Any ideas? Also, can you please confirm that I'm operating on the correct 2 files, referenced by `Paths.clean` and `Paths.cleanBackup`. The complete test output is here: http://pastebin.com/d6ukMSAk

PS: Can I have `mach test` run individual tasks (rather than entire test files)?

Comment 22

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

> PS: Can I have `mach test` run individual tasks (rather than entire test files)?

I'm afraid not.
(I'm trying to debug your error right now)

Comment 23

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

Comment on attachment 8599824 [details] [diff] [review]
third-version.patch

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

I found your issue: `promiseSource` calls `wipe`, which destroys the directory. You need to recreate it *after* calling `promiseSource`.

::: browser/components/sessionstore/test/browser_backup_recovery.js
@@ +186,5 @@
> +  yield File.writeAtomic(Paths.cleanBackup, BACKUP_SOURCE);
> +  info("writing done");
> +
> +  // Ensure that we can recover from Paths.recovery
> +  is((yield SessionFile.read()).source, SOURCE, "Recovered the correct source from the recovery file");

Wait, why should we recover SOURCE and not BACKUP_SOURCE?

@@ +193,5 @@
> +  yield SessionFile.wipe();
> +
> +  // Create Paths.recoveryBackup and invalid Paths.recovery file
> +  info("Corrupting recovery file, attempting to recover from recovery backup");
> +  SOURCE = yield promiseSource("Paths.recoveryBackup");

Please use another variable name.

Comment 24

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

By the way, I assume that
{
  version: ["sessionstore", 1]
}

will keep everybody happy. Could you make that change, Fabian?

Comment 25

[Fabian Furger]

Attachment: four.patch

Alright, I took your comments into consideration and updated my changes on the flight last weekend. Unfortunately, I lost my notes so I might be missing something...

Thanks David, that did indeed solve my problem. I created 2 separate tests, one that checks the scenario where the recovery file has a valid format version number and one where the recovery file has a high format version number to check the fallback to the backup file.

There is a similar test (`test_recovery`) that checks fallback in case of a corrupt file but I'd say my tests are sufficiently different to keep both.

What about the case where both recovery and recovery backup have a higher format version though? I assume the expected behavior is to start a new, clean session, is that correct? How can I test that?

David, I used your proposed naming but I can't claim I'm happy with it. I'd rather have
>{
>  version: {
>    sessionstore: 1
>  }
>}
so that the version can be accessed like that: `format = foo.version.sessionstore` (rather than something like `format = foo.version[0] === "sessionstore" ? foo.version[1] : -1`. I'd say with your proposal and the array, there's much more room for error. Am I missing something?

Comment 26

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

The idea of making it `["sessionstore", 1]` is that:
- the format name is "sessionstore";
- the format version is 1.

I kind of like this. However, I realize that `version` is a bad name, so
 format: ["sessionstore", 1]
would certainly be better.

How does that sound?

Comment 27

[Fabian Furger]

I understand what you mean, my point I is that I'd prefer to store it as an object rather than an array. The reason being that this allows more secure access.

Comment 28

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

Comment on attachment 8604608 [details] [diff] [review]
four.patch

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

Looks good to me with trivial nits.

::: browser/components/sessionstore/test/browser_backup_recovery.js
@@ +73,5 @@
>    yield promiseBrowserLoaded(tab.linkedBrowser);
>    TabState.flush(tab.linkedBrowser);
>    yield SessionSaver.run();
>  
> +  // Ensure the exepected files (don't) exist

Nit: "expected"

@@ +168,5 @@
> +
> +  // Check there's a format version number
> +  is(JSON.parse(SOURCE).version[0], "sessionstore", "Found sessionstore format version");
> +
> +  // Create Paths.recovery file

Isn't this Paths.source?

@@ +198,5 @@
> +  let parsedSource = JSON.parse(SOURCE);
> +  parsedSource.version[1] = Number.MAX_VALUE;
> +  SOURCE = JSON.stringify(parsedSource);
> +
> +  yield File.writeAtomic(Paths.clean, SOURCE);

I'd prefer if you wrote directly `JSON.stringify(parsedSource)` without first doing `SOURCE = JSON.stringify(...)`.

Comment 29

[Fabian Furger]

Attachment: five.patch

> Isn't this Paths.source?

I didn't check that, this is copied from the previous test that someone else wrote. Looking at the code I'd probably prefer "// Create Paths.clean file" or something like that. "Paths.source" implies that the commonly used "Paths" object has a "source" property, which, as far as I can recall, it doesn't.

Comment 30

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

Comment on attachment 8611282 [details] [diff] [review]
five.patch

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

r=me with trivial changes

(and forget my comment on `Paths.clean`, that was a typo)

::: browser/components/sessionstore/SessionFile.jsm
@@ +213,5 @@
> +        let versions = parsed.version;
> +        let parsedVersion = undefined;
> +        if (versions[0] == "sessionstore") {
> +          parsedVersion = versions[1];
> +        }

Actually, the following might be a little easier to read:

  let [format, version] = parsed.version || ["sessionstore", 0]; // Older versions of the file didn't have a format name/number.
  if (!SessionStore.isFormatVersionCompatible(version)) {
    ...
  }

::: browser/components/sessionstore/test/browser_backup_recovery.js
@@ +173,5 @@
> +  yield File.makeDir(Paths.backups);
> +  yield File.writeAtomic(Paths.clean, SOURCE);
> +
> +  info("Attempting to recover from the recovery file");
> +  // Ensure that we can recover from Paths.recovery

Actually, you wrote to Paths.clean, not Paths.recovery.

@@ +192,5 @@
> +
> +  // Create Paths.clean and Paths.cleanBackup files
> +  yield File.makeDir(Paths.backups);
> +
> +  // Change version number so we'll attempt to load the backup

"to something that we don't understand yet"

Comment 31

[Fabian Furger]

Attachment: six.patch

There you go!

Comment 32

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

Comment on attachment 8616133 [details] [diff] [review]
six.patch

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

Looks good to me, thanks for the patch.
Remind me: do you have Try access or do you need me to push the patch to Try?

Comment 33

[Fabian Furger]

I had asked for and was given L1 access after finishing the previous bug (session store backup cleanup). 

I've had a look at this page: https://wiki.mozilla.org/ReleaseEngineering/TryServer and tried to access the Try server via SSH but it failed due to permissions. However, I saw no option to add my SSH key to my Try account. How exactly should I submit to Try?


Then I'll also need to create a proper patch (rather than just redirecting the `hg diff` output).

Comment 34

[Florian Bender]

Have you resolved this issue, Fabian?

David, may you help him?

Comment 35

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

Sorry about that, I missed the question.
In the future, Fabian, don't hesitate to mark me as "need info", otherwise I risk missing your message.

Normally, you provided your ssh when you got your L1 access. So you should not need to do anything special for the Try server.

If you have run `./mach mercurial-setup` recently, the only thing you need to do to push to try is
`hg push-to-try -m "try: ..."`, where the ... are given by http://trychooser.pub.build.mozilla.org/ or equivalent.

Comment 36

[Fabian Furger]

Thanks for the reply. I ran the mercurial-setup before submitting the patch for the previous bug so I tried push-to-try which failed ("hg: unknown command 'push-to-try'"). I re-ran the mercurial-setup which suggested a few changes to my hgrc (which I accepted and applied) but that didn't fix it. Google didn't help me find a way to get the mercurial push-to-try. Is that an alias that I could add manually?

Comment 37

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

Have you pulled recently? You may be using an old version of mercurial-setup. Command `hg push-to-try` is provided by extension `push-to-try`, which should be installed by `./mach mercurial-setup`.

Comment 38

[Fabian Furger]

Hm that's strange, I ran `./mach mercurial-setup` two days ago, several times, without being queried to install push-to-try. Now I re-ran the setup and could install push-to-try. After some more configurating, I actually managed to run the push-to-try, I used the arguments that you supplied for my last patch! Thanks for your patience! The results should end up here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ab792a7358chttps://treeherder.mozilla.org/#/jobs?repo=try&revision=9ab792a7358c

My current changes are more extensive than they were when I was writing them because I ran `hg pull` in the meantime and several other changes were made to the 3 files that I modified. Is this a problem?

Comment 39

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

Well, according to the test results, there seems to be a problem. My guess is that there is a syntax error somewhere in your changes. Could you try and run a test manually, to see if you can pinpoint the error?

./mach mochitest browser/components/sessionstore/test/browser_backup_recovery.js

Comment 40

[Fabian Furger]

Sorry for the delay, I finally got around to investigating the matter. Since submitting my latest patch (the sixth), I added no more changes. The only thing that I did was run `hg pull`.

I did a complete re-build and ran the tests which again failed. It appears that, before an actual test is run, an exception in a promise resolution callback is reported. Then, I get a few TypeError exceptions: "SessionSaver.clearLastSaveTime is not a function" in the SessionSaver and "TabState.flush is not a function" in the actual test (which is thrown a couple of times). The full test output is here: http://pastebin.com/NW6B5u9T

I don't know what that's about, I'm quite sure it's not something I did, back in June.

Comment 41

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

With the latest Nightly, the patch doesn't apply.
Did you run `hg pull --rebase`?

Comment 42

[Fabian Furger]

I tried again removing all local changes, pulling (with rebase), and applying the patch. As you say (and as I noticed earlier), there are conflicts. SessionStore.jsm seems to have changed around my addition of the `isFormatVersionCompatible(version)` function  and the browser_backup_recovery.js seems to have changed as well. There are two chunks in my diff of the test, the first is the addition of several comments to existing code and the second is the addition of two new tests, `test_version` and `test_version_fallback`.

Now, only one test fails (the contents of a backup recovery file don't match the expected contents). I can't see why it doesn't match, the diff shows quite a lot of differences so I assume something significant is done incorrectly. However, I'm now too far away from the code to understand what's wrong so I'll just show you the test output if that's ok: pastebin.com/hd1BFKKN

Comment 43

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

I'll try and find some time next week to help you.

Comment 44

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

I actually don't encounter any error.

Comment 45

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

Attachment: MozReview Request: Bug 1147822 - Add a format to Session Restore files;r?yoric

Bug 1147822 - Add a format to Session Restore files;r?yoric

Comment 46

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

Comment on attachment 8670810 [details]
MozReview Request: Bug 1147822 - Add a format to Session Restore files;r?yoric

Bug 1147822 - Add a format to Session Restore files;r?yoric

Comment 47

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

https://reviewboard.mozilla.org/r/21459/#review19451

Comment 48

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

Comment on attachment 8670810 [details]
MozReview Request: Bug 1147822 - Add a format to Session Restore files;r?yoric

https://reviewboard.mozilla.org/r/21461/#review19453

Comment 49

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dae51949b26

Comment 50

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c1b4c02d7b38

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c1b4c02d7b38

Comment 52

[Wayne Mery (:wsmwk)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #0)
> We recently had issues when changing the format of sessionstore.js. These
> issues would have been less annoying if we had had a version number that
> would have let Firefox know beforehand that it cannot load a sessionstore.js
> using a format too recent, and that it should have fallen back to loading a
> backup.

I'm guessing the immediate impetus for this was bug 1143720.  It's a good change.

If format changes are extremely rare it perhaps might not matter in the sense that most users will never encounter a situation where their session restore will not be built from the most recent store. But for testers such as myself who bounce around random old versions for testing, the risk is much higher, so I wonder 
 a) is the user informed when session recover does not use the last session file and informed why?
 b) alternatively, or in addition, are session format revs documented in a place other than just the source code that could be monitored - for example wiki?

Comment 53

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

(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #52)
> (In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment
> #0)
> > We recently had issues when changing the format of sessionstore.js. These
> > issues would have been less annoying if we had had a version number that
> > would have let Firefox know beforehand that it cannot load a sessionstore.js
> > using a format too recent, and that it should have fallen back to loading a
> > backup.
> 
> I'm guessing the immediate impetus for this was bug 1143720.  It's a good
> change.
> 
> If format changes are extremely rare it perhaps might not matter in the
> sense that most users will never encounter a situation where their session
> restore will not be built from the most recent store. But for testers such
> as myself who bounce around random old versions for testing, the risk is
> much higher, so I wonder 
>  a) is the user informed when session recover does not use the last session
> file and informed why?

No such information is available, no. How would you suggest we do that?

>  b) alternatively, or in addition, are session format revs documented in a
> place other than just the source code that could be monitored - for example
> wiki?

Not yet, but at least, now, we have support for actually documenting stuff.