Bug 972434 - Fix a dumb loop that mistakenly deletes all of the bookmarks backups. r=MattN
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 13 Feb 2014 23:26:40 +0100
changeset 168704 b9cff8846380759236aae0f13f4eb132eb707d9d
parent 168703 1816d2cbd18bc54af494db297d8435e8dfd81dd7
child 168705 dc542bb8011ce0e6f7ff29518682fbc98a94d22a
push id26215
push userryanvm@gmail.com
push dateFri, 14 Feb 2014 13:54:11 +0000
treeherdermozilla-central@5d7caa093f4f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs972434
milestone30.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 972434 - Fix a dumb loop that mistakenly deletes all of the bookmarks backups. r=MattN
toolkit/components/places/PlacesBackups.jsm
toolkit/components/places/tests/bookmarks/test_466303-json-remove-backups.js
--- a/toolkit/components/places/PlacesBackups.jsm
+++ b/toolkit/components/places/PlacesBackups.jsm
@@ -304,37 +304,35 @@ this.PlacesBackups = {
    */
   create: function PB_create(aMaxBackups, aForceBackup) {
     return Task.spawn(function* () {
       // Construct the new leafname.
       let newBackupFilename = this.getFilenameForDate();
       let mostRecentBackupFile = yield this.getMostRecentBackup();
 
       if (!aForceBackup) {
-        let numberOfBackupsToDelete = 0;
-        if (aMaxBackups !== undefined && aMaxBackups > -1) {
-          let backupFiles = yield this.getBackupFiles();
-          numberOfBackupsToDelete = backupFiles.length - aMaxBackups;
+        let backupFiles = yield this.getBackupFiles();
+        // If there are backups, limit them to aMaxBackups, if requested.
+        if (backupFiles.length > 0 && typeof aMaxBackups == "number" &&
+            aMaxBackups > -1 && backupFiles.length >= aMaxBackups) {
+          let numberOfBackupsToDelete = backupFiles.length - aMaxBackups;
 
           // If we don't have today's backup, remove one more so that
           // the total backups after this operation does not exceed the
           // number specified in the pref.
-          if (!mostRecentBackupFile ||
-              !this._isFilenameWithSameDate(OS.Path.basename(mostRecentBackupFile),
-                                            newBackupFilename))
+          if (!this._isFilenameWithSameDate(OS.Path.basename(mostRecentBackupFile),
+                                            newBackupFilename)) {
             numberOfBackupsToDelete++;
-        }
+          }
 
-        while (numberOfBackupsToDelete--) {
-          this._entries.pop();
-          if (!this._backupFiles) {
-            yield this.getBackupFiles();
+          while (numberOfBackupsToDelete--) {
+            this._entries.pop();
+            let oldestBackup = this._backupFiles.pop();
+            yield OS.File.remove(oldestBackup);
           }
-          let oldestBackup = this._backupFiles.pop();
-          yield OS.File.remove(oldestBackup);
         }
 
         // Do nothing if we already have this backup or we don't want backups.
         if (aMaxBackups === 0 ||
             (mostRecentBackupFile &&
              this._isFilenameWithSameDate(OS.Path.basename(mostRecentBackupFile),
                                           newBackupFilename)))
           return;
--- a/toolkit/components/places/tests/bookmarks/test_466303-json-remove-backups.js
+++ b/toolkit/components/places/tests/bookmarks/test_466303-json-remove-backups.js
@@ -1,66 +1,133 @@
 /* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim:set ts=2 sw=2 sts=2 et: */
 /* 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/. */
 
-const NUMBER_OF_BACKUPS = 1;
+// Since PlacesBackups.getbackupFiles() is a lazy getter, these tests must
+// run in the given order, to avoid making it out-of-sync.
+
+add_task(function check_max_backups_is_respected() {
+  // Get bookmarkBackups directory
+  let backupFolder = yield PlacesBackups.getBackupFolder();
+
+  // Create an html dummy backup in the past.
+  let htmlPath = OS.Path.join(backupFolder, "bookmarks-2008-01-01.html");
+  let htmlFile = yield OS.File.open(htmlPath, { truncate: true });
+  htmlFile.close();
+  do_check_true(yield OS.File.exists(htmlPath));
+
+  // Create a json dummy backup in the past.
+  let jsonPath = OS.Path.join(backupFolder, "bookmarks-2008-01-31.json");
+  let jsonFile = yield OS.File.open(jsonPath, { truncate: true });
+  jsonFile.close();
+  do_check_true(yield OS.File.exists(jsonPath));
+
+  // Export bookmarks to JSON.
+  // Allow 2 backups, the older one should be removed.
+  yield PlacesBackups.create(2);
+  let backupFilename = PlacesBackups.getFilenameForDate();
+  let re = new RegExp("^" + backupFilename.replace(/\.json/, "") + "(_[0-9]+){0,1}\.json$");
+
+  let count = 0;
+  let lastBackupPath = null;
+  let iterator = new OS.File.DirectoryIterator(backupFolder);
+  try {
+    yield iterator.forEach(aEntry => {
+      count++;
+      if (aEntry.name.match(re))
+        lastBackupPath = aEntry.path;
+    });
+  } finally {
+    iterator.close();
+  }
+
+  do_check_eq(count, 2);
+  do_check_neq(lastBackupPath, null);
+  do_check_false(yield OS.File.exists(htmlPath));
+  do_check_true(yield OS.File.exists(jsonPath));
+});
+
+add_task(function check_max_backups_greater_than_backups() {
+  // Get bookmarkBackups directory
+  let backupFolder = yield PlacesBackups.getBackupFolder();
+
+  // Export bookmarks to JSON.
+  // Allow 3 backups, none should be removed.
+  yield PlacesBackups.create(3);
+  let backupFilename = PlacesBackups.getFilenameForDate();
+  let re = new RegExp("^" + backupFilename.replace(/\.json/, "") + "(_[0-9]+){0,1}\.json$");
+
+  let count = 0;
+  let lastBackupPath = null;
+  let iterator = new OS.File.DirectoryIterator(backupFolder);
+  try {
+    yield iterator.forEach(aEntry => {
+      count++;
+      if (aEntry.name.match(re))
+        lastBackupPath = aEntry.path;
+    });
+  } finally {
+    iterator.close();
+  }
+  do_check_eq(count, 2);
+  do_check_neq(lastBackupPath, null);
+});
+
+add_task(function check_max_backups_null() {
+  // Get bookmarkBackups directory
+  let backupFolder = yield PlacesBackups.getBackupFolder();
+
+  // Export bookmarks to JSON.
+  // Allow infinite backups, none should be removed, a new one is not created
+  // since one for today already exists.
+  yield PlacesBackups.create(null);
+  let backupFilename = PlacesBackups.getFilenameForDate();
+  let re = new RegExp("^" + backupFilename.replace(/\.json/, "") + "(_[0-9]+){0,1}\.json$");
+
+  let count = 0;
+  let lastBackupPath = null;
+  let iterator = new OS.File.DirectoryIterator(backupFolder);
+  try {
+    yield iterator.forEach(aEntry => {
+      count++;
+      if (aEntry.name.match(re))
+        lastBackupPath = aEntry.path;
+    });
+  } finally {
+    iterator.close();
+  }
+  do_check_eq(count, 2);
+  do_check_neq(lastBackupPath, null);
+});
+
+add_task(function check_max_backups_undefined() {
+  // Get bookmarkBackups directory
+  let backupFolder = yield PlacesBackups.getBackupFolder();
+
+  // Export bookmarks to JSON.
+  // Allow infinite backups, none should be removed, a new one is not created
+  // since one for today already exists.
+  yield PlacesBackups.create();
+  let backupFilename = PlacesBackups.getFilenameForDate();
+  let re = new RegExp("^" + backupFilename.replace(/\.json/, "") + "(_[0-9]+){0,1}\.json$");
+
+  let count = 0;
+  let lastBackupPath = null;
+  let iterator = new OS.File.DirectoryIterator(backupFolder);
+  try {
+    yield iterator.forEach(aEntry => {
+      count++;
+      if (aEntry.name.match(re))
+        lastBackupPath = aEntry.path;
+    });
+  } finally {
+    iterator.close();
+  }
+  do_check_eq(count, 2);
+  do_check_neq(lastBackupPath, null);
+});
 
 function run_test() {
-  do_test_pending();
-
-  Task.spawn(function() {
-    // Get bookmarkBackups directory
-    let backupFolder = yield PlacesBackups.getBackupFolder();
-    let bookmarksBackupDir = new FileUtils.File(backupFolder);
-
-    // Create an html dummy backup in the past
-    var htmlBackupFile = bookmarksBackupDir.clone();
-    htmlBackupFile.append("bookmarks-2008-01-01.html");
-    if (htmlBackupFile.exists())
-      htmlBackupFile.remove(false);
-    do_check_false(htmlBackupFile.exists());
-    htmlBackupFile.create(Ci.nsILocalFile.NORMAL_FILE_TYPE, 0600);
-    do_check_true(htmlBackupFile.exists());
-
-    // Create a json dummy backup in the past
-    var jsonBackupFile = bookmarksBackupDir.clone();
-    jsonBackupFile.append("bookmarks-2008-01-31.json");
-    if (jsonBackupFile.exists())
-      jsonBackupFile.remove(false);
-    do_check_false(jsonBackupFile.exists());
-    jsonBackupFile.create(Ci.nsILocalFile.NORMAL_FILE_TYPE, 0600);
-    do_check_true(jsonBackupFile.exists());
-
-    // Export bookmarks to JSON.
-    var backupFilename = PlacesBackups.getFilenameForDate();
-    var rx = new RegExp("^" + backupFilename.replace(/\.json/, "") + "(_[0-9]+){0,1}\.json$");
-    var files = bookmarksBackupDir.directoryEntries;
-    var entry;
-    while (files.hasMoreElements()) {
-      entry = files.getNext().QueryInterface(Ci.nsIFile);
-      if (entry.leafName.match(rx))
-        entry.remove(false);
-    }
-
-    yield PlacesBackups.create(NUMBER_OF_BACKUPS);
-    files = bookmarksBackupDir.directoryEntries;
-    while (files.hasMoreElements()) {
-      entry = files.getNext().QueryInterface(Ci.nsIFile);
-      if (entry.leafName.match(rx))
-        lastBackupFile = entry;
-    }
-    do_check_true(lastBackupFile.exists());
-
-    // Check that last backup has been retained
-    do_check_false(htmlBackupFile.exists());
-    do_check_false(jsonBackupFile.exists());
-    do_check_true(lastBackupFile.exists());
-
-    // cleanup
-    lastBackupFile.remove(false);
-    do_check_false(lastBackupFile.exists());
-
-    do_test_finished();
-  });
+  run_next_test();
 }