Closed Bug 876168 Opened 10 years ago Closed 10 years ago

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

Tracking

()

Status:

RESOLVED FIXED

Milestone:

Firefox 25

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file, 5 obsolete files)

Creating backups on first launch after each upgrade 10 years ago David Teller [:Yoric] - still alive but not very active 15.27 KB, patch		Details \| Diff \| Splinter Review
Creating backups on first launch after each upgrade, v2 10 years ago David Teller [:Yoric] - still alive but not very active 14.74 KB, patch	ttaubert : feedback+	Details \| Diff \| Splinter Review
Creating backups on first launch after each upgrade, v3 10 years ago David Teller [:Yoric] - still alive but not very active 13.67 KB, patch	ttaubert : review+	Details \| Diff \| Splinter Review
Creating backups on first launch after each upgrade, v4 10 years ago David Teller [:Yoric] - still alive but not very active 13.66 KB, patch	Yoric : review+	Details \| Diff \| Splinter Review
Creating backups on first launch after each upgrade, v5 10 years ago David Teller [:Yoric] - still alive but not very active 13.75 KB, patch	ttaubert : review+	Details \| Diff \| Splinter Review
Creating backups on first launch after each upgrade, v6 10 years ago David Teller [:Yoric] - still alive but not very active 60.84 KB, patch	Yoric : review+	Details \| Diff \| Splinter Review

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

Assignee

Description

•

10 years ago

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.

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

Assignee

Comment 1

•

10 years ago

Might be useful for bug 838577, btw.

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

Assignee

Comment 2

•

10 years ago

Attached patch Creating backups on first launch after each upgrade (obsolete) — Details — Splinter Review

Assignee: nobody → dteller

Attachment #766910 - Flags: review?(ttaubert)

Tim Taubert [:ttaubert] (inactive)

Updated

•

10 years ago

Attachment #766910 - Attachment is patch: true

Tim Taubert [:ttaubert] (inactive)

Comment 3

•

10 years ago

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.

Attachment #766910 - Flags: review?(ttaubert)

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

Assignee

Comment 4

•

10 years ago

(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.

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

Assignee

Comment 5

•

10 years ago

Attached patch Creating backups on first launch after each upgrade, v2 (obsolete) — Details — Splinter Review

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

Attachment #766910 - Attachment is obsolete: true

Attachment #767182 - Flags: review?(ttaubert)

Tim Taubert [:ttaubert] (inactive)

Comment 6

•

10 years ago

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.

Attachment #767182 - Flags: review?(ttaubert) → feedback+

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

Assignee

Comment 7

•

10 years ago

Attached patch Creating backups on first launch after each upgrade, v3 (obsolete) — Details — Splinter Review

Applied feedback.

Attachment #767182 - Attachment is obsolete: true

Attachment #767202 - Flags: review?(ttaubert)

Tim Taubert [:ttaubert] (inactive)

Comment 8

•

10 years ago

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.

Attachment #767202 - Flags: review?(ttaubert) → review+

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

Assignee

Comment 9

•

10 years ago

Attached patch Creating backups on first launch after each upgrade, v4 (obsolete) — Details — Splinter Review

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

Attachment #767202 - Attachment is obsolete: true

Attachment #767222 - Flags: review+

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

Assignee

Comment 10

•

10 years ago

Attached patch Creating backups on first launch after each upgrade, v5 (obsolete) — Details — Splinter Review

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

Attachment #767222 - Attachment is obsolete: true

Attachment #767432 - Flags: review?(ttaubert)

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

Assignee

Comment 11

•

10 years ago

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

Tim Taubert [:ttaubert] (inactive)

Comment 12

•

10 years ago

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.

Attachment #767432 - Flags: review?(ttaubert) → review+

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

Assignee

Comment 13

•

10 years ago

Attached patch Creating backups on first launch after each upgrade, v6 — Details — Splinter Review

Attachment #767432 - Attachment is obsolete: true

Attachment #768344 - Flags: review+

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

Assignee

Comment 14

•

10 years ago

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

Phil Ringnalda (:philor)

Comment 15

•

10 years ago

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

Status: NEW → RESOLVED

Closed: 10 years ago

Resolution: --- → FIXED

Target Milestone: --- → Firefox 25

Tim Taubert [:ttaubert] (inactive)

Updated

•

10 years ago

Depends on: 890112

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

Bugzilla

Quick Search

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

Categories

(Firefox :: Session Restore, defect)

Tracking

()

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(1 file, 5 obsolete files)

Description

Comment 1

Comment 2

Updated

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Comment 13

Comment 14

Comment 15

Updated