Bug 1066992 - Bookmarks backup should not exceed max backups a=standard8

--HG--
extra : rebase_source : 2b94cb6ef6cba2966e44897dca7e2d0090dcf6fc

toolkit/components/places/PlacesBackups.jsm

@@ -19,6 +19,17 @@ XPCOMUtils.defineLazyGetter(this, "filenamesRegex",
   () => /^bookmarks-([0-9-]+)(?:_([0-9]+)){0,1}(?:_([a-z0-9=+-]{24})){0,1}\.(json(lz4)?)$/i
 );
 
+async function limitBackups(aMaxBackups, backupFiles) {
+  if (typeof aMaxBackups == "number" && aMaxBackups > -1 &&
+      backupFiles.length >= aMaxBackups) {
+    let numberOfBackupsToDelete = backupFiles.length - aMaxBackups;
+    while (numberOfBackupsToDelete--) {
+      let oldestBackup = backupFiles.pop();
+      await OS.File.remove(oldestBackup);
+    }
+  }
+}
+
 /**
  * Appends meta-data information to a given filename.
  */
@@ -240,6 +251,13 @@ var PlacesBackups = {
       }
       this._backupFiles.unshift(aFilePath);
     } else {
+      let aMaxBackup = Services.prefs.getIntPref("browser.bookmarks.max_backups");
+      if (aMaxBackup === 0) {
+        if (!this._backupFiles)
+          await this.getBackupFiles();
+        limitBackups(aMaxBackup, this._backupFiles);
+        return nodeCount;
+      }
       // If we are saving to a folder different than our backups folder, then
       // we also want to create a new compressed version in it.
       // This way we ensure the latest valid backup is the same saved by the
@@ -269,9 +287,9 @@ var PlacesBackups = {
         }
         let jsonString = await OS.File.read(aFilePath);
         await OS.File.writeAtomic(newFilePath, jsonString, { compression: "lz4" });
+        await limitBackups(aMaxBackup, this._backupFiles);
       }
     }
-
     return nodeCount;
   },
 
@@ -289,22 +307,12 @@ var PlacesBackups = {
    * @return {Promise}
    */
   create: function PB_create(aMaxBackups, aForceBackup) {
-    let limitBackups = async () => {
-      let backupFiles = await this.getBackupFiles();
-      if (typeof aMaxBackups == "number" && aMaxBackups > -1 &&
-          backupFiles.length >= aMaxBackups) {
-        let numberOfBackupsToDelete = backupFiles.length - aMaxBackups;
-        while (numberOfBackupsToDelete--) {
-          let oldestBackup = this._backupFiles.pop();
-          await OS.File.remove(oldestBackup);
-        }
-      }
-    };
-
     return (async () => {
       if (aMaxBackups === 0) {
         // Backups are disabled, delete any existing one and bail out.
-        await limitBackups(0);
+        if (!this._backupFiles)
+          await this.getBackupFiles();
+        await limitBackups(0, this._backupFiles);
         return;
       }
 
@@ -366,7 +374,7 @@ var PlacesBackups = {
       this._backupFiles.unshift(newBackupFileWithMetadata);
 
       // Limit the number of backups.
-      await limitBackups(aMaxBackups);
+      await limitBackups(aMaxBackups, this._backupFiles);
     })();
   },
 

toolkit/components/places/tests/unit/test_utils_backups_create.js

@@ -10,11 +10,10 @@
 
 const NUMBER_OF_BACKUPS = 10;
 
-add_task(async function() {
+async function createBackups(nBackups, dateObj, bookmarksBackupDir) {
   // Generate random dates.
-  let dateObj = new Date();
   let dates = [];
-  while (dates.length < NUMBER_OF_BACKUPS) {
+  while (dates.length < nBackups) {
     // Use last year to ensure today's backup is the newest.
     let randomDate = new Date(dateObj.getFullYear() - 1,
                               Math.floor(12 * Math.random()),
@@ -25,10 +24,6 @@ add_task(async function() {
   // Sort dates from oldest to newest.
   dates.sort();
 
-  // Get and cleanup the backups folder.
-  let backupFolderPath = await PlacesBackups.getBackupFolder();
-  let bookmarksBackupDir = new FileUtils.File(backupFolderPath);
-
   // Fake backups are created backwards to ensure we won't consider file
   // creation time.
   // Create fake backups for the newest dates.
@@ -42,10 +37,10 @@ add_task(async function() {
       do_throw("Unable to create fake backup " + backupFile.leafName);
   }
 
-  await PlacesBackups.create(NUMBER_OF_BACKUPS);
-  // Add today's backup.
-  dates.push(dateObj.getTime());
+  return dates;
+}
 
+async function checkBackups(dates, bookmarksBackupDir) {
   // Check backups.  We have 11 dates but we the max number is 10 so the
   // oldest backup should have been removed.
   for (let i = 0; i < dates.length; i++) {
@@ -72,7 +67,9 @@ add_task(async function() {
     if (backupFile.exists() != shouldExist)
       do_throw("Backup should " + (shouldExist ? "" : "not") + " exist: " + backupFilename);
   }
+}
 
+async function cleanupFiles(bookmarksBackupDir) {
   // Cleanup backups folder.
   // XXX: Can't use bookmarksBackupDir.remove(true) because file lock happens
   // on WIN XP.
@@ -81,5 +78,53 @@ add_task(async function() {
     let entry = files.nextFile;
     entry.remove(false);
   }
+  // Clear cache to match the manual removing of files
+  delete PlacesBackups._backupFiles;
   Assert.ok(!bookmarksBackupDir.directoryEntries.hasMoreElements());
+}
+
+add_task(async function test_create_backups() {
+  let backupFolderPath = await PlacesBackups.getBackupFolder();
+  let bookmarksBackupDir = new FileUtils.File(backupFolderPath);
+
+  let dateObj = new Date();
+  let dates = await createBackups(NUMBER_OF_BACKUPS, dateObj, bookmarksBackupDir);
+  // Add today's backup.
+  await PlacesBackups.create(NUMBER_OF_BACKUPS);
+  dates.push(dateObj.getTime());
+  await checkBackups(dates, bookmarksBackupDir);
+  await cleanupFiles(bookmarksBackupDir);
+});
+
+add_task(async function test_saveBookmarks_with_no_backups() {
+  let backupFolderPath = await PlacesBackups.getBackupFolder();
+  let bookmarksBackupDir = new FileUtils.File(backupFolderPath);
+
+  Services.prefs.setIntPref("browser.bookmarks.max_backups", 0);
+
+  let filePath = do_get_tempdir().path + "/backup.json";
+  await PlacesBackups.saveBookmarksToJSONFile(filePath);
+  let files = bookmarksBackupDir.directoryEntries;
+  Assert.ok(!files.hasMoreElements(), "Should have no backup files.");
+  await OS.File.remove(filePath);
+  // We don't need to call cleanupFiles as we are not creating any
+  // backups but need to reset the cache.
+  delete PlacesBackups._backupFiles;
+});
+
+add_task(async function test_saveBookmarks_with_backups() {
+  let backupFolderPath = await PlacesBackups.getBackupFolder();
+  let bookmarksBackupDir = new FileUtils.File(backupFolderPath);
+
+  Services.prefs.setIntPref("browser.bookmarks.max_backups", NUMBER_OF_BACKUPS);
+
+  let filePath = do_get_tempdir().path + "/backup.json";
+  let dateObj = new Date();
+  let dates = await createBackups(NUMBER_OF_BACKUPS, dateObj, bookmarksBackupDir);
+
+  await PlacesBackups.saveBookmarksToJSONFile(filePath);
+  dates.push(dateObj.getTime());
+  await checkBackups(dates, bookmarksBackupDir);
+  await OS.File.remove(filePath);
+  await cleanupFiles(bookmarksBackupDir);
 });