Closed Bug 934967 Opened 11 years ago Closed 7 years ago

[Session Restore] Read/write data with lz4

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Type:
defect
Priority:
Not set
Severity:
normal
Points:
5

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: Yoric, Assigned: beekill)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 934967 - Part 1: Compress session store files using lz4.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 934967 - Part 2: Make test cases read lz4 compressed version of session store files and create a frequently use function in head.js.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 934967 - Part 3: Add test cases to check session store's migration to using lz4 compression.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 934967 - Part 4: Change session file extension in FirefoxProfileMigrator to jsonlz4 and make SessionMigration read and write functions use lz4 compression.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details 
Reading/writing data with lz4 should decrease the amount of time spent in I/O in session restore as well as battery usage, so this should be a nice win.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
It seems that this could be a problem with upgrades and downgrades. If somebody uses Nightly and then uses Release with the same profile, they will not be able to read their Session Store data. To support this, we would actually need to write out two versions of the data (compressed and uncompressed), until we feel that the migration period has lasted long enough to drop the uncompressed version.

Another question about this is if we need to worry about supporting the usecase of hand-editing these files. I don't think that this is something that we need to worry about, but it is worth bringing up.
We didn't really settle on a decision, yet. All the concerns brought up by you are totally valid and that's why we didn't really move forward here.

The hand-editing use case is probably nothing we should be too worried about. However, there are no tools like "unzip" or "tar" or anything that can read lz4 which makes it even worse should you once in a year want to take a look at its contents.
(In reply to Tim Taubert [:ttaubert] from comment #2)
> The hand-editing use case is probably nothing we should be too worried
> about. However, there are no tools like "unzip" or "tar" or anything that
> can read lz4 which makes it even worse should you once in a year want to
> take a look at its contents.

I suppose it wouldn't be hard to write an add-on that can do the extracting/editing/compressing through Firefox though.
That's true but it doesn't quite *feel* right :)
Whiteboard: p=5
(In reply to Tim Taubert [:ttaubert] from comment #2)
> The hand-editing use case is probably nothing we should be too worried
> about. However, there are no tools like "unzip" or "tar" or anything that
> can read lz4 which makes it even worse should you once in a year want to
> take a look at its contents.

I do e.g. see |lz4| executables linked here: https://code.google.com/p/lz4/
(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> I do e.g. see |lz4| executables linked here: https://code.google.com/p/lz4/

Also shipping with some Linux distros already apparently (either as lz4c or lz4).
(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> I do e.g. see |lz4| executables linked here: https://code.google.com/p/lz4/

I was under the impression that we're doing something nonstandard with our lz4, and this tool won't actually work with our files (I'm not sure we even have a command line tool which can handle our lz4).
(In reply to Steven MacLeod [:smacleod] from comment #7)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> > I do e.g. see |lz4| executables linked here: https://code.google.com/p/lz4/
> 
> I was under the impression that we're doing something nonstandard with our
> lz4, and this tool won't actually work with our files (I'm not sure we even
> have a command line tool which can handle our lz4).

Yoric, do you know about this?
Flags: needinfo?(dteller)
Yes, for historical reasons, we use the lz4 algorithm but with our own wrapper. There are plans to upgrade to something more standard, once we have adopted the updated upstream library to a more recent version.
Flags: needinfo?(dteller)
Points: --- → 5
Whiteboard: p=5
Bug 1304389 is newer, but has a patch from bkelly.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Blocks: 1304389
Status: REOPENED → NEW
Assignee: nobody → nnn_bikiu0707
Status: NEW → ASSIGNED
I tested the patch locally, there were some errors. But I want know whether I'm going the right track.
Comment on attachment 8872878 [details]
Bug 934967 - Part 1: Compress session store files using lz4.

https://reviewboard.mozilla.org/r/144388/#review148308

The SessionWorker changes are looking good to me. The SessionFile changes need some more work, I think ;-)

I've written down a few ideas below.

::: browser/components/sessionstore/SessionFile.jsm:101
(Diff revision 1)
>  
>  var SessionFileInternal = {
>    Paths: Object.freeze({
>      // The path to the latest version of sessionstore written during a clean
>      // shutdown. After startup, it is renamed `cleanBackup`.
> -    clean: Path.join(profileDir, "sessionstore.js"),
> +    clean: Path.join(profileDir, "sessionstore.jslz4"),

While we're here, let's standardize on '.jsonlz4' to conform to similar feature usage elsewhere.

::: browser/components/sessionstore/SessionFile.jsm:218
(Diff revision 1)
>  
>      let result;
>      let noFilesFound = true;
> +    let useOldExtension = false;
>      // Attempt to load by order of priority from the various backups
> +    do {

Instead of using a loop here, I'd recommend abstracting away the body of the for...of loop below into a separate method that can be called with a path and returns `result`. If `result` is null, you can call it again with the path containing the old extension.

::: browser/components/sessionstore/SessionFile.jsm:229
(Diff revision 1)
> -        let startMs = Date.now();
> +          let startMs = Date.now();
>  
> -        let source = await OS.File.read(path, { encoding: "utf-8" });
> -        let parsed = JSON.parse(source);
> +          let source;
> +          let parsed;
> +          if (useOldExtension) {
> +            path = this.Paths[key].replace("lz4", "");

So this'd become `.replace("jsonlz4", "js")`

::: browser/components/sessionstore/SessionFile.jsm:246
(Diff revision 1)
> -        }
> +          }
> -        result = {
> +          result = {
> -          origin: key,
> +            origin: key,
> -          source,
> +            source,
> -          parsed
> +            parsed,
> +            useOldExtension: useOldExtension

As you can see used above, a new JS feature is that you can replace `useOldExtension: useOldExtension` with just `useOldExtension` if the name of the key and variable that holds the key's value are the same.

::: browser/components/sessionstore/SessionFile.jsm:254
(Diff revision 1)
>  <span class="indent">>></span>          add(false);
>  <span class="indent">>></span>        Telemetry.getHistogramById("FX_SESSION_RESTORE_READ_FILE_MS").
>  <span class="indent">>></span>          add(Date.now() - startMs);
>  <span class="indent">>></span>        break;
>  <span class="indent">>></span>      } catch (ex) {
>  <span class="indent">>></span>          if (ex instanceof OS.File.Error && ex.becauseNoSuchFile) {

We also need to take care not to pollute our telemetry data with a huge amount of file not found errors when we migrate. Perhaps it's an idea to put the `useOldExtension` retry logic in here?

::: browser/components/sessionstore/SessionWorker.js:314
(Diff revision 1)
>      let exn = null;
>  
>      // Erase main session state file
>      try {
>        File.remove(this.Paths.clean);
> +      File.remove(this.Paths.clean.replace("lz4", ""), {ignoreAbsent: true}); // remove old extension one

nit: Please put comments on their own line and format them correctly: "Remove old extension ones."
Attachment #8872878 - Flags: review?(mdeboer)
Comment on attachment 8872879 [details]
Bug 934967 - Part 2: Make test cases read lz4 compressed version of session store files and create a frequently use function in head.js.

https://reviewboard.mozilla.org/r/144390/#review148318

Looks good! However, I would keep the sessionstore_invalid.json and sessionstore_valid.json files and compress them inside the test instead - that way we're keeping things readable and make sure that the tests can be changed easily when necessary.
Attachment #8872879 - Flags: review?(mdeboer)
Comment on attachment 8872880 [details]
Bug 934967 - Part 3: Add test cases to check session store's migration to using lz4 compression.

https://reviewboard.mozilla.org/r/144392/#review148320

::: browser/components/sessionstore/test/unit/xpcshell.ini:10
(Diff revision 1)
>  support-files =
>    data/sessionCheckpoints_all.json
>    data/sessionstore_invalid.js
>    data/sessionstore_valid.js
> +  data/sessionstore_invalid.jslz4
> +  data/sessionstore_valid.jslz4

This belongs in the previous patch.

::: browser/components/sessionstore/test/unit/xpcshell.ini:18
(Diff revision 1)
>  [test_histogram_corrupt_files.js]
>  [test_shutdown_cleanup.js]
>  [test_startup_nosession_async.js]
>  [test_startup_session_async.js]
>  [test_startup_invalid_session.js]
> +[test_migration.js]

Please use a more descriptive filename, like 'test_migration_lz4compression.js' for example.
Comment on attachment 8872880 [details]
Bug 934967 - Part 3: Add test cases to check session store's migration to using lz4 compression.

https://reviewboard.mozilla.org/r/144392/#review148322

Published too soon!
I think this is already looking really good and ready to review when you push your updates next time.
Attachment #8872880 - Flags: review?(mdeboer)
Comment on attachment 8872878 [details]
Bug 934967 - Part 1: Compress session store files using lz4.

https://reviewboard.mozilla.org/r/144388/#review150164

Almost there! Next version will most probably be an r+, so great work so far!

Please don't forget to mark open issues here in ReviewBoard as 'fixed' when you in fact fixed them. That helps your reviewer quite a bit as well. Thanks!

::: browser/components/sessionstore/SessionFile.jsm:210
(Diff revision 2)
>      } catch (ex) {
>        return undefined;
>      }
>    },
>  
> -  // Find the correct session file, read it and setup the worker.
> +  async loadSessionFile(useOldExtension) {

nit: please rename this to `_readInternal`

::: browser/components/sessionstore/SessionFile.jsm:235
(Diff revision 2)
> +          source = await OS.File.read(path, {encoding: "utf-8"});
> +        } else {
> +          path = this.Paths[key];
> +          source = await OS.File.read(path, {encoding: "utf-8", compression: "lz4"});
> +        }
> +        parsed = JSON.parse(source);

1. Please leave the `let parsed` (variable declaration) here.
2. Please keep the `source = ` line above here as well:

```js
let source = await OS.File.read(path, {encoding: "utf-8", compression: useOldExtension ? null : "lz4"});
let parsed = JSON.parse(source);
```

::: browser/components/sessionstore/SessionFile.jsm:269
(Diff revision 2)
>              corrupted = true;
>            }
>        } finally {
>          if (exists) {
>            noFilesFound = false;
> +          if (!useOldExtension) {

Please revert this change.

::: browser/components/sessionstore/SessionWorker.js:83
(Diff revision 2)
>     *   we have started by loading the corresponding file.
>     */
>    state: null,
>  
>    /**
> +   * A flag indicate we loaded a session file with ext .js

nit: `A flag that indicates that we loaded a session file with the derprecated .js extension.`

::: browser/components/sessionstore/SessionWorker.js:178
(Diff revision 2)
>          // Move $Path.clean out of the way, to avoid any ambiguity as
>          // to which file is more recent.
> +        if (!this.useOldExtension) {
> -        File.move(this.Paths.clean, this.Paths.cleanBackup);
> +          File.move(this.Paths.clean, this.Paths.cleanBackup);
> +        } else {
> +          let oldCleanPath = this.Paths.clean.replace("jsonlz4", "js");

nit: please add a comment here that explains what this code is doing and why it's there.

::: browser/components/sessionstore/SessionWorker.js:220
(Diff revision 2)
> +          compression: "lz4"
>          });
>        }
>  
>        telemetry.FX_SESSION_RESTORE_WRITE_FILE_MS = Date.now() - startWriteMs;
>        telemetry.FX_SESSION_RESTORE_FILE_SIZE_BYTES = data.byteLength;

This will now report incorrect numbers! Could you change this to record the bytes that were actually written to disk?

This number is returned by `File.writeAtomic()`, so there's actually quite little you need to change to correct it.

::: browser/components/sessionstore/SessionWorker.js:313
(Diff revision 2)
>      let exn = null;
>  
>      // Erase main session state file
>      try {
>        File.remove(this.Paths.clean);
> +      // Remove old extension ones

nit: missing dot.
Attachment #8872878 - Flags: review?(mdeboer) → review-
Comment on attachment 8872879 [details]
Bug 934967 - Part 2: Make test cases read lz4 compressed version of session store files and create a frequently use function in head.js.

https://reviewboard.mozilla.org/r/144390/#review150180

Also super close! Please don't forget to resolve the ReviewBoard issues when you push your next version. Thanks!

::: commit-message-f585e:1
(Diff revision 2)
> +Bug 934967 - Part 2: Change session store files extension from .js to .jsonlz4. r?mikedeboer

This is not describing what this patch changes properly.
Please add proper, descriptive commit messages. If it can't fit on a single line, feel free to write a longer message below it, separated from the first line by a newline.

::: browser/components/sessionstore/test/unit/test_backup_once.js:29
(Diff revision 2)
>    run_next_test();
>  }
>  
> +// Compress the source file using lz4 and put the result to destination file
> +// After that, source file is deleted
> +function promise_compress(source, destination) {

You can simplify this as:

```js
async function writeCompressedFile(source, destination) {
  let data = await OS.File.read(source);
  await OS.File.writeAtomic(destination, data, {compression: "lz4"});
  await OS.File.remove(source);
}
```

::: browser/components/sessionstore/test/unit/test_shutdown_cleanup.js:34
(Diff revision 2)
>    platformVersion: "",
>  });
>  
>  add_task(async function setup() {
> -  let source = do_get_file("data/sessionstore_valid.js");
> -  source.copyTo(profd, "sessionstore.js");
> +  let source = do_get_file("data/sessionstore_valid.jslz4");
> +  source.copyTo(profd, "sessionstore.jslz4");

"jslz4"? Is there something wrong in this test?

I can why it _might_ work - it's because you revert to the .js version if it can't find the compressed one.

So please do the same as in 'test_backup_once.js' and put the shared function in 'browser/components/sessionstore/test/unit/head.js'.

::: browser/components/sessionstore/test/unit/test_startup_invalid_session.js:19
(Diff revision 2)
>  
> -  do_test_pending();
> +  // Compress sessionstore.js to sessionstore.jsonlz4
> +  // and remove sessionstore.js
> +  let oldExtSessionFile = SessionFile.Paths.clean.replace("jsonlz4", "js");
> +  let readPromise = OS.File.read(oldExtSessionFile);
> +  readPromise.then(function onSucess(content) {

Please make the shared function you will put in head.js usable here too.

::: browser/components/sessionstore/test/unit/test_startup_session_async.js:25
(Diff revision 2)
>  
> -  do_test_pending();
> +  // Compress sessionstore.js to sessionstore.jsonlz4
> +  // and remove sessionstore.js
> +  let oldExtSessionFile = SessionFile.Paths.clean.replace("jsonlz4", "js");
> +  let readPromise = OS.File.read(oldExtSessionFile);
> +  readPromise.then(function onSucess(content) {

Same here! DRY is a good thing here I think.
Attachment #8872879 - Flags: review?(mdeboer) → review-
Comment on attachment 8872880 [details]
Bug 934967 - Part 3: Add test cases to check session store's migration to using lz4 compression.

https://reviewboard.mozilla.org/r/144392/#review150190

On the existing code I only have a few small comments. I'd like to see two more tests:
1. one that simulates reading the sessionfile(s) with the lz4 ones in the profile already (in other words: like a startup with already converted files in the profile dir)
2. and one that simulates an empty profile dir and making sure that empty, compressed session files are written to disk. (and `read()` doesn't fail).

::: browser/components/sessionstore/test/unit/test_migration_lz4compression.js:1
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.

The license block is not necessary anymore for test files, so you can remove it here.

::: browser/components/sessionstore/test/unit/test_migration_lz4compression.js:4
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/* Copied from test_backup_once.js */

This comment is not necessary. But good that you started from an existing test!

::: browser/components/sessionstore/test/unit/test_migration_lz4compression.js:10
(Diff revision 2)
> +
> +"use strict";
> +
> +var {OS} = Cu.import("resource://gre/modules/osfile.jsm", {});
> +var {XPCOMUtils} = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> +var {SessionWorker} = Cu.import("resource:///modules/sessionstore/SessionWorker.jsm", {});

s/var/const/ for variables that are unchanged throughout the scope of this file.

::: browser/components/sessionstore/test/unit/test_migration_lz4compression.js:12
(Diff revision 2)
> +
> +var {OS} = Cu.import("resource://gre/modules/osfile.jsm", {});
> +var {XPCOMUtils} = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> +var {SessionWorker} = Cu.import("resource:///modules/sessionstore/SessionWorker.jsm", {});
> +
> +var File = OS.File;

This is not really necessary, right? Just use OS.File everywhere.

::: browser/components/sessionstore/test/unit/test_migration_lz4compression.js:46
(Diff revision 2)
> +    let actual = await OS.File.read(path, { encoding: "utf-8", compression: "lz4" });
> +    Assert.deepEqual(JSON.parse(actual), expect, `File ${path} contains the expected data.`);
> +  })();
> +}
> +
> +// check whether the migration from .js to .jslz4 is correct

nit: Please start comment sentences with a capital and end with a dot.

::: browser/components/sessionstore/test/unit/test_migration_lz4compression.js:71
(Diff revision 2)
> +  Assert.deepEqual(result.parsed, parsed, "result.parse contains expected data");
> +
> +  // initiate a write to ensure we write the compressed version
> +  await SessionFile.write(parsed);
> +  await promise_check_exist(Paths.backups, true);
> +  await promise_check_exist(Paths.clean, false);

Why doesn't this file exist?
Attachment #8872880 - Flags: review?(mdeboer) → review-
Comment on attachment 8872880 [details]
Bug 934967 - Part 3: Add test cases to check session store's migration to using lz4 compression.

https://reviewboard.mozilla.org/r/144392/#review150190

> Why doesn't this file exist?

We performed a SessionFile.write, this write is not final write, so in [1] the Path.clean will be move to Path.cleanBackup

[1]: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionWorker.js#166-167
Comment on attachment 8872878 [details]
Bug 934967 - Part 1: Compress session store files using lz4.

https://reviewboard.mozilla.org/r/144388/#review151284

Cool! This is already looking very good. I'd like to see the final changes one more time. This also gives David a chance to look at this.

::: browser/components/sessionstore/SessionFile.jsm:233
(Diff revision 3)
> +          }
> +          source = await OS.File.read(path, {encoding: "utf-8"});
> +        } else {
> +          path = this.Paths[key];
> +          source = await OS.File.read(path, {encoding: "utf-8", compression: "lz4"});
> +        }

You forgot to make the changes here:
```js
let options = { encoding: "utf-8" };
if (useOldExtension) {
  //...
} else {
  path = this.Paths[key];
  options.compression = "lz4";
}
let source = await OS.File.read(path, options);
let parsed = JSON.parse(source);
```

::: browser/components/sessionstore/SessionWorker.js:197
(Diff revision 3)
>          // We are shutting down. At this stage, we know that
>          // $Paths.clean is either absent or corrupted. If it was
>          // originally present and valid, it has been moved to
>          // $Paths.cleanBackup a long time ago. We can therefore write
>          // with the guarantees that we erase no important data.
> -        File.writeAtomic(this.Paths.clean, data, {
> +        bytesWritten = File.writeAtomic(this.Paths.clean, data, {

Are you 300% sure that this API doesn't simply return a Promise because it's async? I don't know, because this is inside a worker and my gut tells me 'no', but flagging David to be sure...

::: browser/components/sessionstore/SessionWorker.js:224
(Diff revision 3)
> +          compression: "lz4"
>          });
>        }
>  
>        telemetry.FX_SESSION_RESTORE_WRITE_FILE_MS = Date.now() - startWriteMs;
> -      telemetry.FX_SESSION_RESTORE_FILE_SIZE_BYTES = data.byteLength;
> +      telemetry.FX_SESSION_RESTORE_FILE_SIZE_BYTES = bytesWritten;

When this lands, I'll flag Harald to add this telemetry metric to the Quantum Flow dashboard :-)
Attachment #8872878 - Flags: review?(mdeboer) → review-
Comment on attachment 8872879 [details]
Bug 934967 - Part 2: Make test cases read lz4 compressed version of session store files and create a frequently use function in head.js.

https://reviewboard.mozilla.org/r/144390/#review151288

Looks good to me!

::: commit-message-f585e:1
(Diff revision 3)
> +Bug 934967 - Part 2: Make test cases read lz4 compressed version of session store files and

Please keep this on a single line
Attachment #8872879 - Flags: review?(mdeboer) → review+
Comment on attachment 8872880 [details]
Bug 934967 - Part 3: Add test cases to check session store's migration to using lz4 compression.

https://reviewboard.mozilla.org/r/144392/#review151292

Yay! Thanks for the added tests - very re-assuring.
Attachment #8872880 - Flags: review?(mdeboer) → review+
Attachment #8872878 - Flags: feedback?(dteller)
Comment on attachment 8872878 [details]
Bug 934967 - Part 1: Compress session store files using lz4.

https://reviewboard.mozilla.org/r/144388/#review151284

> Are you 300% sure that this API doesn't simply return a Promise because it's async? I don't know, because this is inside a worker and my gut tells me 'no', but flagging David to be sure...

It's my mistake. The File.writeAtomic doesn't return anything [1], so we can't just retrieve the bytes written here. Therefore, I changed to using File.stat instead [2].

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_workers#OS.File.writeAtomic()
[2]: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_workers#OS.File.stat()
Comment on attachment 8872878 [details]
Bug 934967 - Part 1: Compress session store files using lz4.

https://reviewboard.mozilla.org/r/144388/#review152828

LGTM! It looks like you accidentally cleared the feedback flag for David, but I'd still like to have him look at this patch, just in case you or I missed something. Re-adding.
Good work so far! :)
Attachment #8872878 - Flags: review?(mdeboer) → review+
Attachment #8872878 - Flags: feedback?(dteller)
Comment on attachment 8872878 [details]
Bug 934967 - Part 1: Compress session store files using lz4.

https://reviewboard.mozilla.org/r/144388/#review153888

The patch looks mostly good. However, there is one subtle safety problem that needs to be addressed, as well as an open question.
Also, a few nitpicks.

::: browser/components/sessionstore/SessionFile.jsm:224
(Diff revision 4)
> -        let path = this.Paths[key];
> +        let path;
>          let startMs = Date.now();
>  
> -        let source = await OS.File.read(path, { encoding: "utf-8" });
> +        let options = {encoding: "utf-8"};
> +        if (useOldExtension) {
> +          if (key === "recoveryBackup") {

Nit: I'd rather have

```js
  this.Paths[key]
    .replace("jsonlz4", "js")
    .replace("baklz4", "bak");
```

than the hardcoded key.

::: browser/components/sessionstore/SessionFile.jsm:245
(Diff revision 4)
>          }
>          result = {
>            origin: key,
>            source,
> -          parsed
> +          parsed,
> +          useOldExtension

Side-note: my intuition (for a followup bug?) is that we would be interested in knowing how many uses of `useOldExtension` succeed.

::: browser/components/sessionstore/SessionFile.jsm:280
(Diff revision 4)
> +
> +  // Find the correct session file, read it and setup the worker.
> +  async read() {
> +    this._initializationStarted = true;
> +
> +    let {result, noFilesFound} = await this._readInternal(false);

Nit: It would be nice to have a comment explaining why we try once with `false` and once with `true`.

::: browser/components/sessionstore/SessionWorker.js:183
(Diff revision 4)
> +          // Since we are migrating from .js to .jsonlz4,
> +          // we need to compress the deprecated $Path.clean
> +          // and move it to $Path.cleanBackup.
> +          let oldCleanPath = this.Paths.clean.replace("jsonlz4", "js");
> +          let data = File.read(oldCleanPath);
> +          File.remove(oldCleanPath);

**Safety warning** We should remove *after* `writeAtomic` has succeeded. I'd suggest doing this in a `setTimeout`, at least 20 seconds after the success of `writeAtomic`, to avoid losing the data in case of a power outage just at that moment.

Mmmhhhh... come to think about it, I believe that we should **not remove the file at all** – if there is a bug, users will be pretty **** off if we can't recover something somewhat useful. So, I would add the cleanup as a separate step, in another version of Firefox, rather than removing this immediately upon startup. What do you think?
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #40)
> **Safety warning** We should remove *after* `writeAtomic` has succeeded. I'd
> suggest doing this in a `setTimeout`, at least 20 seconds after the success
> of `writeAtomic`, to avoid losing the data in case of a power outage just at
> that moment.
> 
> Mmmhhhh... come to think about it, I believe that we should **not remove the
> file at all** – if there is a bug, users will be pretty pissed off if we
> can't recover something somewhat useful. So, I would add the cleanup as a
> separate step, in another version of Firefox, rather than removing this
> immediately upon startup. What do you think?

Thanks for taking a look David!!

I totally agree! Let's not remove the old file at all.
Comment on attachment 8872878 [details]
Bug 934967 - Part 1: Compress session store files using lz4.

https://reviewboard.mozilla.org/r/144388/#review151182

::: browser/components/sessionstore/SessionFile.jsm:235
(Diff revision 2)
> +          source = await OS.File.read(path, {encoding: "utf-8"});
> +        } else {
> +          path = this.Paths[key];
> +          source = await OS.File.read(path, {encoding: "utf-8", compression: "lz4"});
> +        }
> +        parsed = JSON.parse(source);

There's probably something wrong with compression : useOldExtension ? null : "lz4". That expression makes some tests failed so I'll keep the old one.
(In reply to Bao Quan [:beekill] from comment #42)
> Comment on attachment 8872878 [details]
> Bug 934967 - Part 1: Compress session store files using lz4.
> 
> https://reviewboard.mozilla.org/r/144388/#review151182
> 
> ::: browser/components/sessionstore/SessionFile.jsm:235
> (Diff revision 2)
> > +          source = await OS.File.read(path, {encoding: "utf-8"});
> > +        } else {
> > +          path = this.Paths[key];
> > +          source = await OS.File.read(path, {encoding: "utf-8", compression: "lz4"});
> > +        }
> > +        parsed = JSON.parse(source);
> 
> There's probably something wrong with compression : useOldExtension ? null :
> "lz4". That expression makes some tests failed so I'll keep the old one.

Oops. My mistake, this should have been posted long ago.
Comment on attachment 8879465 [details]
Bug 934967 - Part 4: Change session file extension in FirefoxProfileMigrator to jsonlz4 and make SessionMigration read and write functions use lz4 compression.

https://reviewboard.mozilla.org/r/150770/#review156218
Attachment #8879465 - Flags: review?(mdeboer) → review+
Try server result looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15d50d9943b1. Mike, can you add check-in flag?
Flags: needinfo?(mdeboer)
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/733cfc278628
Part 1: Compress session store files using lz4. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/7de7f8f5903a
Part 2: Make test cases read lz4 compressed version of session store files and create a frequently use function in head.js. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/77d479c2426c
Part 3: Add test cases to check session store's migration to using lz4 compression. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/5e40f7e49396
Part 4: Change session file extension in FirefoxProfileMigrator to jsonlz4 and make SessionMigration read and write functions use lz4 compression. r=mikedeboer
Flags: needinfo?(mdeboer)
Depends on: 1376442
some users have reported lost sessions, this change may be responsible.
I think this bug should be made dependent on :
Bug 1376983 - Firefox 54.0 doesn't restore session of Nightly 2017-06-28, shows the Firefox start page

See comment #5 and comment #6 for a suggested plan to make the session restore backward/forward compatible.
Depends on: 1376983
Depends on: 934310
Depends on: 1427930
No longer depends on: 1427930
Depends on: 1429310
No longer depends on: 1429310
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: