Bug 1066992 - Bookmarks backup should not exceed max backups a=standard8
authorSiddhant <dpsrkp.sid@gmail.com>
Thu, 30 Aug 2018 23:41:37 +0530
changeset 482973 69199edacdf637e4a93ce77066da9b6e51671377
parent 482972 e32a8fc346e197e24180dd046329dab4cfc1ff73
child 482974 c248b33135af97fc2fcb4224cd4e50ac84783788
push id232
push userfmarier@mozilla.com
push dateWed, 05 Sep 2018 20:45:54 +0000
reviewersstandard8
bugs1066992
milestone63.0a1
Bug 1066992 - Bookmarks backup should not exceed max backups a=standard8
toolkit/components/places/PlacesBackups.jsm
toolkit/components/places/tests/unit/test_utils_backups_create.js
--- a/toolkit/components/places/PlacesBackups.jsm
+++ b/toolkit/components/places/PlacesBackups.jsm
@@ -14,16 +14,27 @@ XPCOMUtils.defineLazyModuleGetters(this,
   BookmarkJSONUtils: "resource://gre/modules/BookmarkJSONUtils.jsm",
   OS: "resource://gre/modules/osfile.jsm",
 });
 
 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.
  */
 function appendMetaDataToFilename(aFilename, aMetaData) {
   let matches = aFilename.match(filenamesRegex);
   return "bookmarks-" + matches[1] +
                   "_" + aMetaData.count +
                   "_" + aMetaData.hash +
@@ -235,16 +246,23 @@ var PlacesBackups = {
     if (OS.Path.dirname(aFilePath) == backupFolderPath) {
       // We are creating a backup in the default backups folder,
       // so just update the internal cache.
       if (!this._backupFiles) {
         await this.getBackupFiles();
       }
       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
       // user.  See bug 424389.
       let mostRecentBackupFile = await this.getMostRecentBackup();
       if (!mostRecentBackupFile ||
           hash != getHashFromFilename(OS.Path.basename(mostRecentBackupFile))) {
         let name = this.getFilenameForDate(undefined, true);
@@ -264,19 +282,19 @@ var PlacesBackups = {
         } else {
           // There is no backup for today, add the new one.
           if (!this._backupFiles)
             await this.getBackupFiles();
           this._backupFiles.unshift(newFilePath);
         }
         let jsonString = await OS.File.read(aFilePath);
         await OS.File.writeAtomic(newFilePath, jsonString, { compression: "lz4" });
+        await limitBackups(aMaxBackup, this._backupFiles);
       }
     }
-
     return nodeCount;
   },
 
   /**
    * Creates a dated backup in <profile>/bookmarkbackups.
    * Stores the bookmarks using a lz4 compressed JSON file.
    *
    * @param [optional] int aMaxBackups
@@ -284,32 +302,22 @@ var PlacesBackups = {
    *                       all existing backups are removed and aForceBackup is
    *                       ignored, so a new one won't be created.
    * @param [optional] bool aForceBackup
    *                        Forces creating a backup even if one was already
    *                        created that day (overwrites).
    * @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;
       }
 
       // Ensure to initialize _backupFiles
       if (!this._backupFiles)
         await this.getBackupFiles();
       let newBackupFilename = this.getFilenameForDate(undefined, true);
       // If we already have a backup for today we should do nothing, unless we
@@ -361,17 +369,17 @@ var PlacesBackups = {
       }
 
       // Append metadata to the backup filename.
       let newBackupFileWithMetadata = OS.Path.join(backupFolder, newFilenameWithMetaData);
       await OS.File.move(newBackupFile, newBackupFileWithMetadata);
       this._backupFiles.unshift(newBackupFileWithMetadata);
 
       // Limit the number of backups.
-      await limitBackups(aMaxBackups);
+      await limitBackups(aMaxBackups, this._backupFiles);
     })();
   },
 
   /**
    * Gets the bookmark count for backup file.
    *
    * @param aFilePath
    *        File path The backup file.
--- a/toolkit/components/places/tests/unit/test_utils_backups_create.js
+++ b/toolkit/components/places/tests/unit/test_utils_backups_create.js
@@ -5,52 +5,47 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
  /**
   * Check for correct functionality of bookmarks backups
   */
 
 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()),
                               Math.floor(28 * Math.random()));
     if (!dates.includes(randomDate.getTime()))
       dates.push(randomDate.getTime());
   }
   // 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.
   for (let i = dates.length - 1; i >= 0; i--) {
     let backupFilename = PlacesBackups.getFilenameForDate(new Date(dates[i]));
     let backupFile = bookmarksBackupDir.clone();
     backupFile.append(backupFilename);
     backupFile.create(Ci.nsIFile.NORMAL_FILE_TYPE, parseInt("0666", 8));
     info("Creating fake backup " + backupFile.leafName);
     if (!backupFile.exists())
       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++) {
     let backupFilename;
     let shouldExist;
     let backupFile;
     if (i > 0) {
       let files = bookmarksBackupDir.directoryEntries;
@@ -67,19 +62,69 @@ add_task(async function() {
       backupFilename = PlacesBackups.getFilenameForDate(new Date(dates[i]));
       backupFile = bookmarksBackupDir.clone();
       backupFile.append(backupFilename);
       shouldExist = false;
     }
     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.
   let files = bookmarksBackupDir.directoryEntries;
   while (files.hasMoreElements()) {
     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);
+});