diff --git a/toolkit/components/places/BookmarkJSONUtils.jsm b/toolkit/components/places/BookmarkJSONUtils.jsm index f2bb9863f6f4..b44bcc05386f 100644 --- a/toolkit/components/places/BookmarkJSONUtils.jsm +++ b/toolkit/components/places/BookmarkJSONUtils.jsm @@ -25,6 +25,24 @@ XPCOMUtils.defineLazyModuleGetter(this, "Deprecated", XPCOMUtils.defineLazyGetter(this, "gTextDecoder", () => new TextDecoder()); XPCOMUtils.defineLazyGetter(this, "gTextEncoder", () => new TextEncoder()); +/** + * Generates an hash for the given string. + * + * @note The generated hash is returned in base64 form. Mind the fact base64 + * is case-sensitive if you are going to reuse this code. + */ +function generateHash(aString) { + let cryptoHash = Cc["@mozilla.org/security/hash;1"] + .createInstance(Ci.nsICryptoHash); + cryptoHash.init(Ci.nsICryptoHash.MD5); + let stringStream = Cc["@mozilla.org/io/string-input-stream;1"] + .createInstance(Ci.nsIStringInputStream); + stringStream.data = aString; + cryptoHash.updateFromStream(stringStream, -1); + // base64 allows the '/' char, but we can't use it for filenames. + return cryptoHash.finish(true).replace("/", "-"); +} + this.BookmarkJSONUtils = Object.freeze({ /** * Import bookmarks from a url. @@ -99,13 +117,19 @@ this.BookmarkJSONUtils = Object.freeze({ * * @param aFilePath * OS.File path string for the "bookmarks.json" file to be created. - * + * @param [optional] aOptions + * Object containing options for the export: + * - failIfHashIs: if the generated file would have the same hash + * defined here, will reject with ex.becauseSameHash * @return {Promise} - * @resolves To the exported bookmarks count when the file has been created. + * @resolves once the file has been created, to an object with the + * following properties: + * - count: number of exported bookmarks + * - hash: file hash for contents comparison * @rejects JavaScript exception. * @deprecated passing an nsIFile is deprecated */ - exportToFile: function BJU_exportToFile(aFilePath) { + exportToFile: function BJU_exportToFile(aFilePath, aOptions={}) { if (aFilePath instanceof Ci.nsIFile) { Deprecated.warning("Passing an nsIFile to BookmarksJSONUtils.exportToFile " + "is deprecated. Please use an OS.File path string instead.", @@ -125,12 +149,29 @@ this.BookmarkJSONUtils = Object.freeze({ Components.utils.reportError("Unable to report telemetry."); } + startTime = Date.now(); + let hash = generateHash(jsonString); + // Report the time taken to generate the hash. + try { + Services.telemetry + .getHistogramById("PLACES_BACKUPS_HASHING_MS") + .add(Date.now() - startTime); + } catch (ex) { + Components.utils.reportError("Unable to report telemetry."); + } + + if (hash === aOptions.failIfHashIs) { + let e = new Error("Hash conflict"); + e.becauseSameHash = true; + throw e; + } + // Do not write to the tmp folder, otherwise if it has a different // filesystem writeAtomic will fail. Eventual dangling .tmp files should // be cleaned up by the caller. yield OS.File.writeAtomic(aFilePath, jsonString, { tmpPath: OS.Path.join(aFilePath + ".tmp") }); - return count; + return { count: count, hash: hash }; }); }, diff --git a/toolkit/components/places/PlacesBackups.jsm b/toolkit/components/places/PlacesBackups.jsm index afaacfa6ae1c..4900bad8012e 100644 --- a/toolkit/components/places/PlacesBackups.jsm +++ b/toolkit/components/places/PlacesBackups.jsm @@ -28,12 +28,71 @@ XPCOMUtils.defineLazyGetter(this, "localFileCtor", () => Components.Constructor("@mozilla.org/file/local;1", "nsILocalFile", "initWithPath")); +XPCOMUtils.defineLazyGetter(this, "filenamesRegex", + () => new RegExp("^bookmarks-([0-9\-]+)(?:_([0-9]+)){0,1}(?:_([a-z0-9=\+\-]{24})){0,1}\.(json|html)", "i") +); + +/** + * 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 + + "." + matches[4]; +} + +/** + * Gets the hash from a backup filename. + * + * @return the extracted hash or null. + */ +function getHashFromFilename(aFilename) { + let matches = aFilename.match(filenamesRegex); + if (matches && matches[3]) + return matches[3]; + return null; +} + +/** + * Given two filenames, checks if they contain the same date. + */ +function isFilenameWithSameDate(aSourceName, aTargetName) { + let sourceMatches = aSourceName.match(filenamesRegex); + let targetMatches = aTargetName.match(filenamesRegex); + + return sourceMatches && targetMatches && + sourceMatches[1] == targetMatches[1] && + sourceMatches[4] == targetMatches[4]; +} + +/** + * Given a filename, searches for another backup with the same date. + * + * @return OS.File path string or null. + */ +function getBackupFileForSameDate(aFilename) { + return Task.spawn(function* () { + let backupFiles = yield PlacesBackups.getBackupFiles(); + for (let backupFile of backupFiles) { + if (isFilenameWithSameDate(OS.Path.basename(backupFile), aFilename)) + return backupFile; + } + return null; + }); +} + this.PlacesBackups = { - get _filenamesRegex() { - delete this._filenamesRegex; - return this._filenamesRegex = - new RegExp("^(bookmarks)-([0-9-]+)(_[0-9]+)*\.(json|html)"); - }, + /** + * Matches the backup filename: + * 0: file name + * 1: date in form Y-m-d + * 2: bookmarks count + * 3: contents hash + * 4: file extension + */ + get filenamesRegex() filenamesRegex, get folder() { Deprecated.warning( @@ -99,8 +158,7 @@ this.PlacesBackups = { let entry = files.getNext().QueryInterface(Ci.nsIFile); // A valid backup is any file that matches either the localized or // not-localized filename (bug 445704). - let matches = entry.leafName.match(this._filenamesRegex); - if (!entry.isHidden() && matches) { + if (!entry.isHidden() && filenamesRegex.test(entry.leafName)) { // Remove bogus backups in future dates. if (this.getDateForFile(entry) > new Date()) { entry.remove(false); @@ -139,8 +197,7 @@ this.PlacesBackups = { return; } - let matches = aEntry.name.match(this._filenamesRegex); - if (matches) { + if (filenamesRegex.test(aEntry.name)) { // Remove bogus backups in future dates. let filePath = aEntry.path; if (this.getDateForFile(filePath) > new Date()) { @@ -188,10 +245,10 @@ this.PlacesBackups = { getDateForFile: function PB_getDateForFile(aBackupFile) { let filename = (aBackupFile instanceof Ci.nsIFile) ? aBackupFile.leafName : OS.Path.basename(aBackupFile); - let matches = filename.match(this._filenamesRegex); + let matches = filename.match(filenamesRegex); if (!matches) throw("Invalid backup file name: " + filename); - return new Date(matches[2].replace(/-/g, "/")); + return new Date(matches[1].replace(/-/g, "/")); }, /** @@ -258,7 +315,8 @@ this.PlacesBackups = { aFilePath = aFilePath.path; } return Task.spawn(function* () { - let nodeCount = yield BookmarkJSONUtils.exportToFile(aFilePath); + let { count: nodeCount, hash: hash } = + yield BookmarkJSONUtils.exportToFile(aFilePath); let backupFolderPath = yield this.getBackupFolder(); if (OS.Path.dirname(aFilePath) == backupFolderPath) { @@ -274,22 +332,33 @@ this.PlacesBackups = { // we also want to copy this new backup to it. // This way we ensure the latest valid backup is the same saved by the // user. See bug 424389. - let name = this.getFilenameForDate(); - let newFilename = this._appendMetaDataToFilename(name, - { nodeCount: nodeCount }); - let newFilePath = OS.Path.join(backupFolderPath, newFilename); - let backupFile = yield this._getBackupFileForSameDate(name); - if (!backupFile) { - // Update internal cache if we are not replacing an existing - // backup file. - this._entries.unshift(new localFileCtor(newFilePath)); - if (!this._backupFiles) { - yield this.getBackupFiles(); + let mostRecentBackupFile = yield this.getMostRecentBackup("json"); + if (!mostRecentBackupFile || + hash != getHashFromFilename(OS.Path.basename(mostRecentBackupFile))) { + let name = this.getFilenameForDate(); + let newFilename = appendMetaDataToFilename(name, + { count: nodeCount, + hash: hash }); + let newFilePath = OS.Path.join(backupFolderPath, newFilename); + let backupFile = yield getBackupFileForSameDate(name); + if (backupFile) { + // There is already a backup for today, replace it. + yield OS.File.remove(backupFile, { ignoreAbsent: true }); + if (!this._backupFiles) + yield this.getBackupFiles(); + else + this._backupFiles.shift(); + this._backupFiles.unshift(newFilePath); + } else { + // There is no backup for today, add the new one. + this._entries.unshift(new localFileCtor(newFilePath)); + if (!this._backupFiles) + yield this.getBackupFiles(); + this._backupFiles.unshift(newFilePath); } - this._backupFiles.unshift(newFilePath); - } - yield OS.File.copy(aFilePath, newFilePath); + yield OS.File.copy(aFilePath, newFilePath); + } } return nodeCount; @@ -303,7 +372,9 @@ this.PlacesBackups = { * "places/excludeFromBackup". * * @param [optional] int aMaxBackups - * The maximum number of backups to keep. + * The maximum number of backups to keep. If set to 0 + * 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). @@ -311,75 +382,80 @@ 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 limitBackups = function* () { 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) { + if (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 (!this._isFilenameWithSameDate(OS.Path.basename(mostRecentBackupFile), - newBackupFilename)) { - numberOfBackupsToDelete++; - } - while (numberOfBackupsToDelete--) { this._entries.pop(); let oldestBackup = this._backupFiles.pop(); yield OS.File.remove(oldestBackup); } } + }.bind(this); - // 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; + if (aMaxBackups === 0) { + // Backups are disabled, delete any existing one and bail out. + yield limitBackups(0); + return; } - let backupFile = yield this._getBackupFileForSameDate(newBackupFilename); + // Ensure to initialize _backupFiles + if (!this._backupFiles) + yield this.getBackupFiles(); + let newBackupFilename = this.getFilenameForDate(); + // If we already have a backup for today we should do nothing, unless we + // were required to enforce a new backup. + let backupFile = yield getBackupFileForSameDate(newBackupFilename); + if (backupFile && !aForceBackup) + return; + if (backupFile) { - if (aForceBackup) { - yield OS.File.remove(backupFile, { ignoreAbsent: true }); - } else { - return; - } + // In case there is a backup for today we should recreate it. + this._backupFiles.shift(); + this._entries.shift(); + yield OS.File.remove(backupFile, { ignoreAbsent: true }); } + // Now check the hash of the most recent backup, and try to create a new + // backup, if that fails due to hash conflict, just rename the old backup. + let mostRecentBackupFile = yield this.getMostRecentBackup(); + let mostRecentHash = mostRecentBackupFile && + getHashFromFilename(OS.Path.basename(mostRecentBackupFile)); + // Save bookmarks to a backup file. let backupFolder = yield this.getBackupFolder(); let newBackupFile = OS.Path.join(backupFolder, newBackupFilename); - let nodeCount = yield this.saveBookmarksToJSONFile(newBackupFile); - // Rename the filename with metadata. - let newFilenameWithMetaData = this._appendMetaDataToFilename( - newBackupFilename, - { nodeCount: nodeCount }); + let newFilenameWithMetaData; + try { + let { count: nodeCount, hash: hash } = + yield BookmarkJSONUtils.exportToFile(newBackupFile, + { failIfHashIs: mostRecentHash }); + newFilenameWithMetaData = appendMetaDataToFilename(newBackupFilename, + { count: nodeCount, + hash: hash }); + } catch (ex if ex.becauseSameHash) { + // The last backup already contained up-to-date information, just + // rename it as if it was today's backup. + this._backupFiles.shift(); + this._entries.shift(); + newBackupFile = mostRecentBackupFile; + newFilenameWithMetaData = appendMetaDataToFilename( + newBackupFilename, + { count: this.getBookmarkCountForFile(mostRecentBackupFile), + hash: mostRecentHash }); + } + + // Append metadata to the backup filename. let newBackupFileWithMetadata = OS.Path.join(backupFolder, newFilenameWithMetaData); yield OS.File.move(newBackupFile, newBackupFileWithMetadata); - - // Update internal cache. - let newFileWithMetaData = new localFileCtor(newBackupFileWithMetadata); - this._entries.pop(); - this._entries.unshift(newFileWithMetaData); - this._backupFiles.pop(); + this._entries.unshift(new localFileCtor(newBackupFileWithMetadata)); this._backupFiles.unshift(newBackupFileWithMetadata); - }.bind(this)); - }, - _appendMetaDataToFilename: - function PB__appendMetaDataToFilename(aFilename, aMetaData) { - let matches = aFilename.match(this._filenamesRegex); - let newFilename = matches[1] + "-" + matches[2] + "_" + - aMetaData.nodeCount + "." + matches[4]; - return newFilename; + // Limit the number of backups. + yield limitBackups(aMaxBackups); + }.bind(this)); }, /** @@ -393,43 +469,12 @@ this.PlacesBackups = { getBookmarkCountForFile: function PB_getBookmarkCountForFile(aFilePath) { let count = null; let filename = OS.Path.basename(aFilePath); - let matches = filename.match(this._filenamesRegex); - - if (matches && matches[3]) - count = matches[3].replace(/_/g, ""); + let matches = filename.match(filenamesRegex); + if (matches && matches[2]) + count = matches[2]; return count; }, - _isFilenameWithSameDate: - function PB__isFilenameWithSameDate(aSourceName, aTargetName) { - let sourceMatches = aSourceName.match(this._filenamesRegex); - let targetMatches = aTargetName.match(this._filenamesRegex); - - return (sourceMatches && targetMatches && - sourceMatches[1] == targetMatches[1] && - sourceMatches[2] == targetMatches[2] && - sourceMatches[4] == targetMatches[4]); - }, - - _getBackupFileForSameDate: - function PB__getBackupFileForSameDate(aFilename) { - return Task.spawn(function* () { - let backupFolderPath = yield this.getBackupFolder(); - let iterator = new OS.File.DirectoryIterator(backupFolderPath); - let backupFile; - - yield iterator.forEach(function(aEntry) { - if (this._isFilenameWithSameDate(aEntry.name, aFilename)) { - backupFile = aEntry.path; - return iterator.close(); - } - }.bind(this)); - yield iterator.close(); - - return backupFile; - }.bind(this)); - }, - /** * Gets a bookmarks tree representation usable to create backups in different * file formats. The root or the tree is PlacesUtils.placesRootId. 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 4af56d6a1c32..312f01e4a290 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 @@ -27,7 +27,6 @@ add_task(function check_max_backups_is_respected() { // 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; @@ -35,7 +34,7 @@ add_task(function check_max_backups_is_respected() { try { yield iterator.forEach(aEntry => { count++; - if (aEntry.name.match(re)) + if (PlacesBackups.filenamesRegex.test(aEntry.name)) lastBackupPath = aEntry.path; }); } finally { @@ -56,7 +55,6 @@ add_task(function check_max_backups_greater_than_backups() { // 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; @@ -64,7 +62,7 @@ add_task(function check_max_backups_greater_than_backups() { try { yield iterator.forEach(aEntry => { count++; - if (aEntry.name.match(re)) + if (PlacesBackups.filenamesRegex.test(aEntry.name)) lastBackupPath = aEntry.path; }); } finally { @@ -83,7 +81,6 @@ add_task(function check_max_backups_null() { // 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; @@ -91,7 +88,7 @@ add_task(function check_max_backups_null() { try { yield iterator.forEach(aEntry => { count++; - if (aEntry.name.match(re)) + if (PlacesBackups.filenamesRegex.test(aEntry.name)) lastBackupPath = aEntry.path; }); } finally { @@ -110,7 +107,6 @@ add_task(function check_max_backups_undefined() { // 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; @@ -118,7 +114,7 @@ add_task(function check_max_backups_undefined() { try { yield iterator.forEach(aEntry => { count++; - if (aEntry.name.match(re)) + if (PlacesBackups.filenamesRegex.test(aEntry.name)) lastBackupPath = aEntry.path; }); } finally { diff --git a/toolkit/components/places/tests/bookmarks/test_477583_json-backup-in-future.js b/toolkit/components/places/tests/bookmarks/test_477583_json-backup-in-future.js index 9ba7b848c724..e162952762ca 100644 --- a/toolkit/components/places/tests/bookmarks/test_477583_json-backup-in-future.js +++ b/toolkit/components/places/tests/bookmarks/test_477583_json-backup-in-future.js @@ -22,11 +22,10 @@ function run_test() { dateObj.setYear(dateObj.getFullYear() + 1); let name = PlacesBackups.getFilenameForDate(dateObj); do_check_eq(name, "bookmarks-" + dateObj.toLocaleFormat("%Y-%m-%d") + ".json"); - let rx = new RegExp("^" + name.replace(/\.json/, "") + "(_[0-9]+){0,1}\.json$"); let files = bookmarksBackupDir.directoryEntries; while (files.hasMoreElements()) { let entry = files.getNext().QueryInterface(Ci.nsIFile); - if (entry.leafName.match(rx)) + if (PlacesBackups.filenamesRegex.test(entry.leafName)) entry.remove(false); } @@ -43,8 +42,7 @@ function run_test() { let mostRecentBackupFile = yield PlacesBackups.getMostRecentBackup(); do_check_neq(mostRecentBackupFile, null); let todayFilename = PlacesBackups.getFilenameForDate(); - rx = new RegExp("^" + todayFilename.replace(/\.json/, "") + "(_[0-9]+){0,1}\.json$"); - do_check_true(OS.Path.basename(mostRecentBackupFile).match(rx).length > 0); + do_check_true(PlacesBackups.filenamesRegex.test(OS.Path.basename(mostRecentBackupFile))); // Check that future backup has been removed. do_check_false(futureBackupFile.exists()); diff --git a/toolkit/components/places/tests/bookmarks/test_818584-discard-duplicate-backups.js b/toolkit/components/places/tests/bookmarks/test_818584-discard-duplicate-backups.js new file mode 100644 index 000000000000..14145e276be7 --- /dev/null +++ b/toolkit/components/places/tests/bookmarks/test_818584-discard-duplicate-backups.js @@ -0,0 +1,59 @@ +/* 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/. */ + +/** + * Checks that automatically created bookmark backups are discarded if they are + * duplicate of an existing ones. + */ +function run_test() { + run_next_test(); +} + +add_task(function() { + // Create a backup for yesterday in the backups folder. + let backupFolder = yield PlacesBackups.getBackupFolder(); + let dateObj = new Date(); + dateObj.setDate(dateObj.getDate() - 1); + let oldBackupName = PlacesBackups.getFilenameForDate(dateObj); + let oldBackup = OS.Path.join(backupFolder, oldBackupName); + let {count: count, hash: hash} = yield BookmarkJSONUtils.exportToFile(oldBackup); + do_check_true(count > 0); + do_check_eq(hash.length, 24); + oldBackupName = oldBackupName.replace(/\.json/, "_" + count + "_" + hash + ".json"); + yield OS.File.move(oldBackup, OS.Path.join(backupFolder, oldBackupName)); + + // Create a backup. + // This should just rename the existing backup, so in the end there should be + // only one backup with today's date. + yield PlacesBackups.create(); + + // Get the hash of the generated backup + let backupFiles = yield PlacesBackups.getBackupFiles(); + do_check_eq(backupFiles.length, 1); + + let matches = OS.Path.basename(backupFiles[0]).match(PlacesBackups.filenamesRegex); + do_check_eq(matches[1], new Date().toLocaleFormat("%Y-%m-%d")); + do_check_eq(matches[2], count); + do_check_eq(matches[3], hash); + + // Add a bookmark and create another backup. + let bookmarkId = PlacesUtils.bookmarks.insertBookmark(PlacesUtils.bookmarks.bookmarksMenuFolder, + uri("http://foo.com"), + PlacesUtils.bookmarks.DEFAULT_INDEX, + "foo"); + // We must enforce a backup since one for today already exists. The forced + // backup will replace the existing one. + yield PlacesBackups.create(undefined, true); + do_check_eq(backupFiles.length, 1); + recentBackup = yield PlacesBackups.getMostRecentBackup(); + do_check_neq(recentBackup, OS.Path.join(backupFolder, oldBackupName)); + matches = OS.Path.basename(recentBackup).match(PlacesBackups.filenamesRegex); + do_check_eq(matches[1], new Date().toLocaleFormat("%Y-%m-%d")); + do_check_eq(matches[2], count + 1); + do_check_neq(matches[3], hash); + + // Clean up + PlacesUtils.bookmarks.removeItem(bookmarkId); + yield PlacesBackups.create(0); +}); diff --git a/toolkit/components/places/tests/bookmarks/test_818593-store-backup-metadata.js b/toolkit/components/places/tests/bookmarks/test_818593-store-backup-metadata.js index 18323756e0b8..914d82a74bae 100644 --- a/toolkit/components/places/tests/bookmarks/test_818593-store-backup-metadata.js +++ b/toolkit/components/places/tests/bookmarks/test_818593-store-backup-metadata.js @@ -31,9 +31,9 @@ add_task(function test_saveBookmarksToJSONFile_and_create() // Ensure the backup would be copied to our backups folder when the original // backup is saved somewhere else. let recentBackup = yield PlacesBackups.getMostRecentBackup(); - let todayFilename = PlacesBackups.getFilenameForDate(); - do_check_eq(OS.Path.basename(recentBackup), - todayFilename.replace(/\.json/, "_" + nodeCount + ".json")); + let matches = OS.Path.basename(recentBackup).match(PlacesBackups.filenamesRegex); + do_check_eq(matches[2], nodeCount); + do_check_eq(matches[3].length, 24); // Clear all backups in our backups folder. yield PlacesBackups.create(0); @@ -45,9 +45,9 @@ add_task(function test_saveBookmarksToJSONFile_and_create() mostRecentBackupFile = yield PlacesBackups.getMostRecentBackup(); do_check_neq(mostRecentBackupFile, null); - let rx = new RegExp("^" + todayFilename.replace(/\.json/, "") + "_([0-9]+)\.json$"); - let matches = OS.Path.basename(recentBackup).match(rx); - do_check_true(matches.length > 0 && parseInt(matches[1]) == nodeCount); + matches = OS.Path.basename(recentBackup).match(PlacesBackups.filenamesRegex); + do_check_eq(matches[2], nodeCount); + do_check_eq(matches[3].length, 24); // Cleanup backupFile.remove(false); diff --git a/toolkit/components/places/tests/bookmarks/xpcshell.ini b/toolkit/components/places/tests/bookmarks/xpcshell.ini index 55563d8142ad..3f5d6641bb7e 100644 --- a/toolkit/components/places/tests/bookmarks/xpcshell.ini +++ b/toolkit/components/places/tests/bookmarks/xpcshell.ini @@ -29,3 +29,4 @@ tail = [test_711914.js] [test_protectRoots.js] [test_818593-store-backup-metadata.js] +[test_818584-discard-duplicate-backups.js] diff --git a/toolkit/components/places/tests/head_common.js b/toolkit/components/places/tests/head_common.js index 366b926b1b58..53ce716d9fc9 100644 --- a/toolkit/components/places/tests/head_common.js +++ b/toolkit/components/places/tests/head_common.js @@ -522,10 +522,9 @@ function check_JSON_backup(aIsAutomaticBackup) { bookmarksBackupDir.append("bookmarkbackups"); let files = bookmarksBackupDir.directoryEntries; let backup_date = new Date().toLocaleFormat("%Y-%m-%d"); - let rx = new RegExp("^bookmarks-" + backup_date + "_[0-9]+\.json$"); while (files.hasMoreElements()) { let entry = files.getNext().QueryInterface(Ci.nsIFile); - if (entry.leafName.match(rx)) { + if (PlacesBackups.filenamesRegex.test(entry.leafName)) { profileBookmarksJSONFile = entry; break; } diff --git a/toolkit/components/places/tests/unit/test_utils_backups_create.js b/toolkit/components/places/tests/unit/test_utils_backups_create.js index 820fcfa24fef..5844b181aa9f 100644 --- a/toolkit/components/places/tests/unit/test_utils_backups_create.js +++ b/toolkit/components/places/tests/unit/test_utils_backups_create.js @@ -8,8 +8,6 @@ * Check for correct functionality of bookmarks backups */ -const PREFIX = "bookmarks-"; -const SUFFIX = ".json"; const NUMBER_OF_BACKUPS = 10; function run_test() { @@ -40,7 +38,7 @@ add_task(function () { // creation time. // Create fake backups for the newest dates. for (let i = dates.length - 1; i >= 0; i--) { - let backupFilename = PREFIX + dates[i] + SUFFIX; + 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)); @@ -61,10 +59,9 @@ add_task(function () { let backupFile; if (i > 0) { let files = bookmarksBackupDir.directoryEntries; - let rx = new RegExp("^" + PREFIX + dates[i] + "(_[0-9]+){0,1}" + SUFFIX + "$"); while (files.hasMoreElements()) { let entry = files.getNext().QueryInterface(Ci.nsIFile); - if (entry.leafName.match(rx)) { + if (PlacesBackups.filenamesRegex.test(entry.leafName)) { backupFilename = entry.leafName; backupFile = entry; break; @@ -73,7 +70,7 @@ add_task(function () { shouldExist = true; } else { - backupFilename = PREFIX + dates[i] + SUFFIX; + backupFilename = PlacesBackups.getFilenameForDate(new Date(dates[i])); backupFile = bookmarksBackupDir.clone(); backupFile.append(backupFilename); shouldExist = false; diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json index 0ab3c7fdbbe5..22fd3f91cf78 100644 --- a/toolkit/components/telemetry/Histograms.json +++ b/toolkit/components/telemetry/Histograms.json @@ -2845,6 +2845,15 @@ "extended_statistics_ok": true, "description": "PLACES: Time to convert and write bookmarks.html" }, + "PLACES_BACKUPS_HASHING_MS": { + "expires_in_version": "never", + "kind": "exponential", + "low": 50, + "high": 2000, + "n_buckets": 10, + "extended_statistics_ok": true, + "description": "PLACES: Time to calculate the md5 hash for a backup" + }, "FENNEC_FAVICONS_COUNT": { "expires_in_version": "never", "kind": "exponential",