Last Comment Bug 893009 - Move _backupSessionFileOnce() to the SessionWorker
: Move _backupSessionFileOnce() to the SessionWorker
Status: RESOLVED FIXED
[mentor=ttaubert]
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 25
Assigned To: Steven MacLeod [:smacleod]
:
:
Mentors:
Depends on:
Blocks: 886447
  Show dependency treegraph
 
Reported: 2013-07-12 08:15 PDT by Tim Taubert [:ttaubert]
Modified: 2013-07-29 17:19 PDT (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch - Move logic into worker (6.67 KB, patch)
2013-07-18 10:15 PDT, Steven MacLeod [:smacleod]
ttaubert: feedback+
Details | Diff | Splinter Review
Patch 1 - Move logic into worker v2 (8.17 KB, patch)
2013-07-18 12:35 PDT, Steven MacLeod [:smacleod]
ttaubert: feedback+
Details | Diff | Splinter Review
Patch - Move logic into worker v3 (12.58 KB, patch)
2013-07-22 10:32 PDT, Steven MacLeod [:smacleod]
no flags Details | Diff | Splinter Review
Patch - Move logic into worker v4 (16.11 KB, patch)
2013-07-22 13:30 PDT, Steven MacLeod [:smacleod]
no flags Details | Diff | Splinter Review
Patch - Move logic into worker v5 (14.31 KB, patch)
2013-07-23 11:40 PDT, Steven MacLeod [:smacleod]
ttaubert: review+
Details | Diff | Splinter Review
Patch - Move logic into worker v6 (14.32 KB, patch)
2013-07-23 12:02 PDT, Steven MacLeod [:smacleod]
no flags Details | Diff | Splinter Review
Patch - Move logic into worker v7 (16.61 KB, patch)
2013-07-25 08:58 PDT, Steven MacLeod [:smacleod]
ttaubert: review+
Details | Diff | Splinter Review
Patch - Move logic into worker v8 (19.33 KB, patch)
2013-07-26 10:48 PDT, Steven MacLeod [:smacleod]
no flags Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2013-07-12 08:15:37 PDT
SessionStoreInternal._backupSessionFileOnce() is currently used to create a backup of sessionstore.js exactly once, just before we write the session for the first time.

This can happen automatically in the SessionWorker without that SessionStore.jsm needs to be aware of it. Just like 'hasWrittenLoadStateOnce' we should keep a flag the tracks whether we already created a sessionstore.js backup.

SessionWorker.write() is the function that needs to be modified. It needs to incorporate the functionality of SessionWorker.moveToBackupPath(). The latter can then be removed as we don't need it be part of the API anymore.

For clarification: the backup does not create a copy, it moves sessionstore.js to sessionstore.bak just before writing to sessionstore.js the first time. That's way faster than creating a copy if we're going to overwrite the original anyway.

This bug moves more logic to the Worker so that SessionStore doesn't need to care about it. It also further reduces the amount of SessionStore <-> Worker communication.
Comment 1 Tim Taubert [:ttaubert] 2013-07-12 08:16:40 PDT
This seems like a perfect bug for Steven :)
Comment 2 Steven MacLeod [:smacleod] 2013-07-18 10:15:12 PDT
Created attachment 777906 [details] [diff] [review]
Patch - Move logic into worker
Comment 3 Tim Taubert [:ttaubert] 2013-07-18 10:45:23 PDT
Comment on attachment 777906 [details] [diff] [review]
Patch - Move logic into worker

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

The patch looks great to me! We need to remove the call site that is left over, though.

The (really) superb thing I totally forgot is that we can now actually test this code as the magic happens in _SessionFile and the SessionWorker. Here is a good example of what we can do:
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/unit/test_backup.js

Basically this means we can write an xpcshell test that calls _SessionFile.write() directly and checks the outcome. The first test should just have the sessionstore.js file and make sure that it's properly moved to sessionstore.bak. The second test can call .write() once again and check that sessionstore.js has the new content and sessionstore.bak didn't change and we didn't throw any errors. The third test can then call .write() again and check that only sessionstore.js has changed because we only backup once. That should be a nice test setup, I hope :)

::: browser/components/sessionstore/src/SessionStore.jsm
@@ -458,5 @@
>        catch (ex) { debug("The session file is invalid: " + ex); }
>      }
>  
> -    // A Lazy getter for the sessionstore.js backup promise.
> -    XPCOMUtils.defineLazyGetter(this, "_backupSessionFileOnce", function () {

We also need to remove the call site of _backupSessionFileOnce in _saveStateObject().
Comment 4 Steven MacLeod [:smacleod] 2013-07-18 12:35:14 PDT
Created attachment 778005 [details] [diff] [review]
Patch 1 - Move logic into worker v2

Added the removal of the calling code to the patch (Somehow missed it in the last commit). Tests to come in additional patch.
Comment 5 Tim Taubert [:ttaubert] 2013-07-18 12:46:39 PDT
Comment on attachment 778005 [details] [diff] [review]
Patch 1 - Move logic into worker v2

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ -3801,5 @@
>  
> -    let promise;
> -    // If "sessionstore.resume_from_crash" is true, attempt to backup the
> -    // session file first, before writing to it.
> -    if (this._resume_from_crash) {

Sorry, I totally forgot about the resume_from_crash pref. If this is false we shouldn't create a backup on startup. I think the only way we can keep this is by adding a second argument like _SessionFile.write(data, {backupOnFirstWrite: this._resume_from_crash}).

The SessionWorker should then not create a backup if backupOnFirstWrite=false. This needs to be only checked once when hasMovedToBackupPathOnce=false.
Comment 6 Steven MacLeod [:smacleod] 2013-07-22 10:32:07 PDT
Created attachment 779272 [details] [diff] [review]
Patch - Move logic into worker v3
Comment 7 Tim Taubert [:ttaubert] 2013-07-22 11:06:15 PDT
Comment on attachment 779272 [details] [diff] [review]
Patch - Move logic into worker v3

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

::: browser/components/sessionstore/src/SessionWorker.js
@@ +113,5 @@
>     * Write the session to disk.
>     */
> +  write: function (stateString, options) {
> +    if (!this.hasMovedToBackupPathOnce) {
> +      //if ('backupOnFirstWrite' in options && options.backupOnFirstWrite) {

This is commented out?

@@ +121,5 @@
> +          // Ignore exceptions about non-existent files.
> +        }
> +      //}
> +
> +      this.hasMovedToBackupPathOnce = true;

The name is not really ideal anymore because the meaning has changed now when we never actually do the backup. Is 'hasWrittenStateOnce' better? hasWrittenOnce? isFirstWrite?

::: browser/components/sessionstore/src/_SessionFile.jsm
@@ +67,5 @@
>    /**
>     * Write the contents of the session file, asynchronously.
>     */
> +  write: function (aData, options = {}) {
> +    return SessionFileInternal.write(aData, options);

Nit: Just to stick with the conventions of this file, the argument should be named 'aOptions'.

@@ +205,5 @@
>    read: function () {
>      return SessionWorker.post("read").then(msg => msg.ok);
>    },
>  
> +  write: function (aData, options) {

Nit: (see above)

::: browser/components/sessionstore/test/unit/test_backup_once.js
@@ +37,5 @@
> +  let content = "test_2";
> +  let initial_content = decoder.decode(yield OS.File.read(pathStore));
> +  let initial_backup_content = decoder.decode(yield OS.File.read(pathBackup));
> +
> +  yield _SessionFile.write(content, {backupOnFirstWrite: true});

Can you please add another test that calls write(backupOnFirstWrite:false) to check that no backup has been created. And then calls write(backupOnFirstWrite:true) to make sure we don't create a backup on the second write either? I think this needs to be in a new file because we need another instance to test this.
Comment 8 Steven MacLeod [:smacleod] 2013-07-22 13:30:18 PDT
Created attachment 779372 [details] [diff] [review]
Patch - Move logic into worker v4

Updated based on feedback from Tim.

Also, added an optional |aOptions| parameter to |writeLoadStateOnceAfterStartup()| as well. This method ends up calling |write()| on the worker, so it was needed to pass in |{backupOnFirstWrite: this._resume_from_crash}|
Comment 9 Tim Taubert [:ttaubert] 2013-07-22 14:24:53 PDT
Comment on attachment 779372 [details] [diff] [review]
Patch - Move logic into worker v4

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

I don't think that writeLoadStateOnceAfterStartup() should create a backup. This is done very early after startup only to detect startup crashes and doesn't actually change session data - it just flips a flag. You're totally right that it uses .write() as well so we should probably just copy the two lines from write() into writeLoadStateOnceAfterStartup() for now. Bug 888373 will make it obsolete soon.

::: browser/components/sessionstore/src/SessionWorker.js
@@ +114,5 @@
>     * Write the session to disk.
>     */
> +  write: function (stateString, options = {}) {
> +    if (!this.hasWrittenState) {
> +      if ('backupOnFirstWrite' in options && options.backupOnFirstWrite) {

Better: if (options && options.backupOnFirstWrite). That doesn't fail when options is null.
Comment 10 Steven MacLeod [:smacleod] 2013-07-23 11:40:31 PDT
Created attachment 779913 [details] [diff] [review]
Patch - Move logic into worker v5

Updated the patch based on Tim's review.
Comment 11 Tim Taubert [:ttaubert] 2013-07-23 11:48:50 PDT
Comment on attachment 779913 [details] [diff] [review]
Patch - Move logic into worker v5

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

Cool, thank you! Did you push this to try, yet?

::: browser/components/sessionstore/src/SessionWorker.js
@@ +57,5 @@
>    // the loadState to disk once after startup.
>    hasWrittenLoadStateOnce: false,
>  
> +  // Boolean that tells whether we already made a
> +  // call to write. We will only attempt to move

Nit: 'call to write()'. Makes it clearer that you're talking about the method.
Comment 12 Steven MacLeod [:smacleod] 2013-07-23 12:02:28 PDT
Created attachment 779934 [details] [diff] [review]
Patch - Move logic into worker v6

Updated comment, from Tim's review.
Comment 13 Steven MacLeod [:smacleod] 2013-07-23 12:12:18 PDT
Try push: https://tbpl.mozilla.org/?tree=Try&rev=e07ef3ee8325
Comment 14 Steven MacLeod [:smacleod] 2013-07-25 08:58:22 PDT
Created attachment 781024 [details] [diff] [review]
Patch - Move logic into worker v7

Fixed and simplified the broken test.

Try: https://tbpl.mozilla.org/?tree=Try&rev=025a918fffc5
Comment 15 Tim Taubert [:ttaubert] 2013-07-25 12:32:48 PDT
Comment on attachment 781024 [details] [diff] [review]
Patch - Move logic into worker v7

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

This looks great. I think we can also remove the last 'testNoWriteBackup' test. AFAICT it doesn't actually test anything as the atomic backup test never actually writes, does it?

r=me with the minor cleanup. Try looks good as well, let's land this!

::: browser/components/sessionstore/test/browser_833286_atomic_backup.js
@@ +56,4 @@
>    nextTest(testWriteNoBackup);
>  }
>  
>  function testWriteNoBackup() {

We could maybe rename that to testInitialState()? Up to you.

@@ +58,5 @@
>  
>  function testWriteNoBackup() {
> +  // Ensure sessionstore.bak is not created. We start with a clean
> +  // profile so there was nothing to move to sessionstore.bak before
> +  // initially writing sessionstore.js

Thank you for clarifying this.

@@ +76,5 @@
>  
>    nextTest(testWriteBackup);
>  }
>  
>  function testWriteBackup() {

That should probably be named testReadBackup(). All we ever do is read stuff.
Comment 16 Steven MacLeod [:smacleod] 2013-07-26 10:48:03 PDT
Created attachment 781808 [details] [diff] [review]
Patch - Move logic into worker v8

(In reply to Tim Taubert [:ttaubert] from comment #15)
> This looks great. I think we can also remove the last 'testNoWriteBackup'
> test. AFAICT it doesn't actually test anything as the atomic backup test
> never actually writes, does it?

The way this test was written was a little confusing. I've renamed a number of the
functions as you've suggested, and added comments to make it more clear when writes to the session store happen.
Comment 17 Tim Taubert [:ttaubert] 2013-07-26 11:16:46 PDT
https://hg.mozilla.org/integration/fx-team/rev/6fba91b781ee
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-07-27 18:59:48 PDT
https://hg.mozilla.org/mozilla-central/rev/6fba91b781ee

Note You need to log in before you can comment on or make changes to this bug.