876168 - [Session Restore] Find a way to backup after each upgrade

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

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

For some reason, upgrading my Nightly has killed my sessionstore.js and sessionstore.bak. No idea why.

I believe that we should perform a snapshot to avoid this kind of situation.

Possible algorithm:
- record in preferences the latest navigator.buildID for which we have performed a sessionstore snapshot copy;
- prior to restoring sessionstore.js, check current navigator.buildID - if navigator.buildID is greater than latest stored navigator.buildID, perform snapshot copy and update preference.

Comment 1

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

Might be useful for bug 838577, btw.

Comment 2

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

Attachment: Creating backups on first launch after each upgrade

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 766910 [details] [diff] [review]
Creating backups on first launch after each upgrade

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

::: browser/app/profile/firefox.js
@@ +836,5 @@
>  // When set to true, this pref overrides that behavior, and pinned tabs will only
>  // be restored when they are focused.
>  pref("browser.sessionstore.restore_pinned_tabs_on_demand", false);
> +// The version at which we performed the latest upgrade backup
> +pref("browser.sessionstore.upgrade.backup.buildID", "");

Maybe browser.sessionstore.backupOnUpgrade.lastBuildID? Or something more descriptive without that many levels?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +479,5 @@
> +
> +    let buildID = Services.appinfo.platformBuildID;
> +    let latestBackup = this._prefBranch.getCharPref(PREF_UPGRADE);
> +    return Task.spawn(function task() {
> +      if (latestBackup == buildID) {

We should return before even spawning a task.

@@ +483,5 @@
> +      if (latestBackup == buildID) {
> +        return;
> +      }
> +      try {
> +        let ext = "-" + buildID;

Nit: that variable seems superfluous.

::: browser/components/sessionstore/src/_SessionFile.jsm
@@ +86,5 @@
>    createBackupCopy: function SessionFile_createBackupCopy() {
>      return SessionFileInternal.createBackupCopy();
>    },
>    /**
> +   * Create a backup copy, asynchronously.

That comment should mention the upgrade case.

@@ +88,5 @@
>    },
>    /**
> +   * Create a backup copy, asynchronously.
> +   */
> +  createUpgradeBackupCopy: function SessionFile_createBackupCopy(ext) {

The function expression's name is wrong. We can also omit that nowadays.

@@ +92,5 @@
> +  createUpgradeBackupCopy: function SessionFile_createBackupCopy(ext) {
> +    return SessionFileInternal.createUpgradeBackupCopy(ext);
> +  },
> +  /**
> +   * Create a backup copy, asynchronously.

Wrong comment.

@@ +316,5 @@
>        }
>      });
>    },
>  
> +  createUpgradeBackupCopy: function ssfi_createBackupCopy(ext) {

Same wrong name for the function expression. We can omit it.

@@ +357,3 @@
>        }
>  
> +      // Erase any backup, any file named "sessionstore.bak"[-buildID].

Nit: ... name "sessionstore.bak[-buildID]".

@@ +361,4 @@
>        try {
> +        for (let promise of iterator) {
> +          let entry = yield promise;
> +          if (!entry.isDir && entry.name.startsWith("sessionstore.bak")) {

.startsWith(LEAF_NAME_BACKUP) or something? We should make "sessionstore.js" and "sessionstore.bak" consts.

@@ +371,5 @@
> +            }
> +          }
> +        }
> +      } finally {
> +        iterator.close();

Do we need to close that explicitly? We're catching all errors an even if we would not it should be closed when going out of scope, no?

::: browser/components/sessionstore/test/Makefile.in
@@ +137,5 @@
>  	browser_739531_sample.html \
>  	browser_739805.js \
>  	browser_819510_perwindowpb.js \
>  	browser_833286_atomic_backup.js \
> +	browser_upgrade_backup.js \

Please put this at the beginning of the list where all the other tests with better names live :)

::: browser/components/sessionstore/test/browser_838577_backup.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

That file should not have been included in the patch, right?

::: browser/components/sessionstore/test/browser_upgrade_backup.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Nit: public domain header.

@@ +17,5 @@
> +
> +      const PREF_UPGRADE = "browser.sessionstore.upgrade.backup.buildID";
> +      let buildID = Services.appinfo.platformBuildID;
> +
> +      is(Services.prefs.getCharPref(PREF_UPGRADE), buildID, "upgrade backup should be set");

I assume that this might not always be true if we run the test in single mode?

@@ +25,5 @@
> +      let contents = "browser_upgrade_backup.js";
> +      let pathStore = OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.js");
> +      yield OS.File.writeAtomic(pathStore, contents, { tmpPath: pathStore + ".tmp" });
> +      SessionStoreStuff.SessionStoreInternal._initPrefs();
> +      yield SessionStoreStuff.SessionStoreInternal._performUpgradeBackup();

That doesn't seem like the right thing to do. I understand that we can't really test the startup path but we shouldn't call into "private" stuff.

::: browser/components/sessionstore/test/unit/test_backup.js
@@ +33,5 @@
> +
> +  let data = yield OS.File.read(pathBackup(ext));
> +  do_check_eq((new TextDecoder()).decode(data), content);
> +
> +  yield OS.File.remove(pathBackup(ext));

You could also test _SessionFile.removeUpgradeBackup() here.

Comment 4

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

(In reply to Tim Taubert [:ttaubert] from comment #3)
> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +479,5 @@
> > +
> > +    let buildID = Services.appinfo.platformBuildID;
> > +    let latestBackup = this._prefBranch.getCharPref(PREF_UPGRADE);
> > +    return Task.spawn(function task() {
> > +      if (latestBackup == buildID) {
> 
> We should return before even spawning a task.

I don't understand what you mean. Do you want me to setTimeout() this?

> Do we need to close that explicitly? We're catching all errors an even if we
> would not it should be closed when going out of scope, no?

Good point.

> @@ +17,5 @@
> > +
> > +      const PREF_UPGRADE = "browser.sessionstore.upgrade.backup.buildID";
> > +      let buildID = Services.appinfo.platformBuildID;
> > +
> > +      is(Services.prefs.getCharPref(PREF_UPGRADE), buildID, "upgrade backup should be set");
> 
> I assume that this might not always be true if we run the test in single
> mode?

I don't see how. We always run session restore initialization before starting the test, don't we?

> @@ +25,5 @@
> > +      let contents = "browser_upgrade_backup.js";
> > +      let pathStore = OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.js");
> > +      yield OS.File.writeAtomic(pathStore, contents, { tmpPath: pathStore + ".tmp" });
> > +      SessionStoreStuff.SessionStoreInternal._initPrefs();
> > +      yield SessionStoreStuff.SessionStoreInternal._performUpgradeBackup();
> 
> That doesn't seem like the right thing to do. I understand that we can't
> really test the startup path but we shouldn't call into "private" stuff.

Well, this is meant as a unit test. I really can't think of any other way to do perform a meaningful test.

Comment 5

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

Attachment: Creating backups on first launch after each upgrade, v2

Applied your feedback, with the exception of the private call.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 767182 [details] [diff] [review]
Creating backups on first launch after each upgrade, v2

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +479,5 @@
> +
> +    let buildID = Services.appinfo.platformBuildID;
> +    let latestBackup = this._prefBranch.getCharPref(PREF_UPGRADE);
> +    return Task.spawn(function task() {
> +      if (latestBackup == buildID) {

I mean, we don't even need to call Task.spawn() if (latestBackup == buildID). We could not return a promise in that case but we don't need that anyway.

::: browser/components/sessionstore/src/_SessionFile.jsm
@@ +89,5 @@
>    /**
> +   * Create a backup copy, asynchronously.
> +   * This is designed to perform backup on upgrade.
> +   */
> +  createUpgradeBackupCopy: function SessionFile_createBackupCopy(ext) {

Wrong name for function expression.

@@ +329,5 @@
>        }
>      });
>    },
>  
> +  createUpgradeBackupCopy: function ssfi_createBackupCopy(ext) {

Wrong name for function expression :)

@@ +382,5 @@
> +            exn = exn || ex;
> +          }
> +        }
> +      }
> +      iterator.close();

.close() is called before StopIteration is thrown, no need to close explicitly.

::: browser/components/sessionstore/test/Makefile.in
@@ +28,5 @@
>  	browser_input.js \
>  	browser_input_sample.html \
>  	browser_pageshow.js \
>  	browser_windowRestore_perwindowpb.js \
> +	browser_upgrade_backup.js \

p > u > w

::: browser/components/sessionstore/test/browser_upgrade_backup.js
@@ +23,5 @@
> +      Services.prefs.setCharPref(PREF_UPGRADE, "");
> +      let contents = "browser_upgrade_backup.js";
> +      let pathStore = OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.js");
> +      yield OS.File.writeAtomic(pathStore, contents, { tmpPath: pathStore + ".tmp" });
> +      SessionStoreStuff.SessionStoreInternal._initPrefs();

I think we should get rid of that and wait for the service to be initialized. I'm okay with calling _performUpgradeBackup() as I don't see any other way of testing this.

::: browser/components/sessionstore/test/unit/test_backup.js
@@ +35,5 @@
> +  let data = yield OS.File.read(pathBackup(ext));
> +  do_check_eq((new TextDecoder()).decode(data), content);
> +
> +  do_print("Ensuring that we can remove the backup");
> +  yield _SessionFile.removeUpgradeBackup(ext);

You could also check the the file really is gone.

Comment 7

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

Attachment: Creating backups on first launch after each upgrade, v3

Applied feedback.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 767202 [details] [diff] [review]
Creating backups on first launch after each upgrade, v3

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

r=me with all comments addressed.

::: browser/components/sessionstore/src/_SessionFile.jsm
@@ +89,5 @@
>    /**
> +   * Create a backup copy, asynchronously.
> +   * This is designed to perform backup on upgrade.
> +   */
> +  createUpgradeBackupCopy: function SessionFile_createBackupCopy(ext) {

Please correct the wrong function name.

@@ +318,5 @@
>        }
>      });
>    },
>  
> +  createUpgradeBackupCopy: function ssfi_createBackupCopy(ext) {

Please correct the wrong function name.

@@ +371,5 @@
> +            exn = exn || ex;
> +          }
> +        }
> +      }
> +      iterator.close();

We don't need to close the iterator explicitly.

::: browser/components/sessionstore/test/Makefile.in
@@ +28,5 @@
>  	browser_input.js \
>  	browser_input_sample.html \
>  	browser_pageshow.js \
>  	browser_windowRestore_perwindowpb.js \
> +	browser_upgrade_backup.js \

p > u > w

::: browser/components/sessionstore/test/unit/test_backup.js
@@ +35,5 @@
> +  let data = yield OS.File.read(pathBackup(ext));
> +  do_check_eq((new TextDecoder()).decode(data), content);
> +
> +  do_print("Ensuring that we can remove the backup");
> +  yield _SessionFile.removeUpgradeBackup(ext);

Please check that the file is gone.

Comment 9

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

Attachment: Creating backups on first launch after each upgrade, v4

Applied.
Try: https://tbpl.mozilla.org/?tree=Try&rev=1d3ef72a12bd

Comment 10

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

Attachment: Creating backups on first launch after each upgrade, v5

Changed the test to avoid having two instances of SessionStore.
Try: https://tbpl.mozilla.org/?tree=Try&rev=b0998b0ca4f8

Comment 11

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

Sorry, wrong try: https://tbpl.mozilla.org/?tree=Try&rev=2fc76a32661c

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 767432 [details] [diff] [review]
Creating backups on first launch after each upgrade, v5

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

Please make sure to include browser_upgrade_backup.js in the tests' Makefile again :)

r=me with comments addressed

::: browser/app/profile/firefox.js
@@ +838,5 @@
>  pref("browser.sessionstore.restore_pinned_tabs_on_demand", false);
> +// The version at which we performed the latest upgrade backup
> +pref("browser.sessionstore.upgradeBackup.latestBuildID", "");
> +// End-users should not run sessionstore in test mode
> +pref("browser.sessionstore.testmode", false);

How about calling it 'browser.sessionstore.debug'? We could then also enable log messages automatically (in future patches), that's always helpful in testing mode.

::: browser/components/sessionstore/test/browser_upgrade_backup.js
@@ +11,5 @@
> +  Task.spawn(function task() {
> +    try {
> +
> +      // Obtain access to internals
> +      Services.prefs.setBoolPref("browser.sessionstore.testmode", true);

I think this can be moved to head.js.

@@ +14,5 @@
> +      // Obtain access to internals
> +      Services.prefs.setBoolPref("browser.sessionstore.testmode", true);
> +
> +      // Wait until initialization is complete
> +      SessionStore.init();

We should not call init() explicitly. It's called bei nsISessionStartup anyway.

Comment 13

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

Attachment: Creating backups on first launch after each upgrade, v6

Comment 14

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

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

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4f7341619f51