From b2d08e195f8ffec83f7c3c0c742148bddd66e22d Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Thu, 13 Feb 2014 23:26:40 +0100 Subject: [PATCH] Bug 972434 - Fix a dumb loop that mistakenly deletes all of the bookmarks backups. r=MattN --- toolkit/components/places/PlacesBackups.jsm | 28 ++- .../test_466303-json-remove-backups.js | 181 ++++++++++++------ 2 files changed, 137 insertions(+), 72 deletions(-) diff --git a/toolkit/components/places/PlacesBackups.jsm b/toolkit/components/places/PlacesBackups.jsm index 2cfa6749005c..957a5ee705db 100644 --- a/toolkit/components/places/PlacesBackups.jsm +++ b/toolkit/components/places/PlacesBackups.jsm @@ -309,27 +309,25 @@ this.PlacesBackups = { 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(); } - let oldestBackup = this._backupFiles.pop(); - yield OS.File.remove(oldestBackup); + + while (numberOfBackupsToDelete--) { + this._entries.pop(); + let oldestBackup = this._backupFiles.pop(); + yield OS.File.remove(oldestBackup); + } } // Do nothing if we already have this backup or we don't want backups. diff --git a/toolkit/components/places/tests/bookmarks/test_466303-json-remove-backups.js b/toolkit/components/places/tests/bookmarks/test_466303-json-remove-backups.js index a9b220ea3908..4af56d6a1c32 100644 --- a/toolkit/components/places/tests/bookmarks/test_466303-json-remove-backups.js +++ b/toolkit/components/places/tests/bookmarks/test_466303-json-remove-backups.js @@ -4,63 +4,130 @@ * 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(); }