Closed Bug 1016953 Opened 6 years ago Closed 6 years ago

renaming old uncompressed backups changes their extension to .jsonlz4 even if they are uncompressed

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox31 --- unaffected
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: mak, Assigned: ahameez)

References

Details

(Whiteboard: p=0 s=33.1 [qa-])

Attachments

(2 files, 7 obsolete files)

This is a particular case of interaction between compressed backups and hashes we use to figure if we should create a new backup.

Basically when the new backup would have the same hash as the old one, we just bail out creating the backup and the code just renames the last one to the current date.
Looks like in this process it may also change the extension of the old backup from .json to .jsonlz4, but the old backup may not be compressed.

We should preserve the extension when renaming an old backup.

Ahameez, would you like to take this bug? Having a test would be awesome.
Flags: needinfo?(althaf.mozilla)
Flags: firefox-backlog+
took a bit longer than expected :)
Attachment #8432185 - Flags: review?(mak77)
Flags: needinfo?(althaf.mozilla)
Comment on attachment 8432185 [details] [diff] [review]
bug1016953_renaming_uncompressed_backups.diff v1

Review of attachment 8432185 [details] [diff] [review]:
-----------------------------------------------------------------

Some minor details to fix but looks good

::: toolkit/components/places/PlacesBackups.jsm
@@ +439,5 @@
>          this._entries.shift();
>          newBackupFile = mostRecentBackupFile;
> +        let rx = new RegExp("\.json$");
> +        if (rx.test(OS.Path.basename(mostRecentBackupFile)))
> +          newBackupFilename = this.getFilenameForDate();

please add a comment above this explaining why we do this.

note you can compact this a little bit like

if (/\.json$/.test(...))

::: toolkit/components/places/tests/bookmarks/test_1016953-renaming-uncompressed.js
@@ +10,5 @@
> +*/
> +
> +add_task(function* bug10169583_test() {
> +
> +  // Same Date Same hash, file should be left alone

please rephrase this comment to make clearer what it means

@@ +14,5 @@
> +  // Same Date Same hash, file should be left alone
> +
> +  let backupFolder = yield PlacesBackups.getBackupFolder();
> +  // Save to profile dir to obtain hash and nodeCount to append to filename
> +  let tempPath = OS.Path.join(OS.Constants.Path.profileDir, "bookmarks.json");

please use a filename including the bug#, like bug1016953_bookmarks.json, so there's less risk of name conflicting with future changes

@@ +15,5 @@
> +
> +  let backupFolder = yield PlacesBackups.getBackupFolder();
> +  // Save to profile dir to obtain hash and nodeCount to append to filename
> +  let tempPath = OS.Path.join(OS.Constants.Path.profileDir, "bookmarks.json");
> +  let metaData = yield BookmarkJSONUtils.exportToFile(tempPath);

you can use destructuring here: let {count, hash} = yield...

@@ +21,5 @@
> +  let dateObj = new Date();
> +  let filename = "bookmarks-" + dateObj.toLocaleFormat("%Y-%m-%d") + "_" +
> +                  metaData.count + "_" + metaData.hash + ".json";
> +  let backupFile = OS.Path.join(backupFolder, filename);
> +  yield BookmarkJSONUtils.exportToFile(backupFile);

why don't you just move the file you previously created to the backupFolder (with the proper destination name clearly)? it should be slightly cheaper.

@@ +23,5 @@
> +                  metaData.count + "_" + metaData.hash + ".json";
> +  let backupFile = OS.Path.join(backupFolder, filename);
> +  yield BookmarkJSONUtils.exportToFile(backupFile);
> +  // Force a compressed backup which fallbacks to rename
> +  yield PlacesBackups.create(1, true);

There should be no reason to enforce a backup or a limit here, .create() should be enough, if I'm not wrong. since every test has a new profile folder the backup folder here should be empty.

@@ +34,5 @@
> +  converter.charset = "UTF-8";
> +  let result = yield OS.File.read(mostRecentBackupFile);
> +  let jsonString = converter.convertFromByteArray(result, result.length);
> +  let validJSON = IsJson(jsonString);
> +  do_check_true(validJSON);

no reason for all of this, you may just do JSON.parse(jsonString) here, if it throws the test will fail. I'd also add a do_log_info("Check is valid json");

@@ +36,5 @@
> +  let jsonString = converter.convertFromByteArray(result, result.length);
> +  let validJSON = IsJson(jsonString);
> +  do_check_true(validJSON);
> +  // Cleanup
> +  yield OS.File.remove(backupFile);

I'd probably also set PlacesBackups._backupFiles = null here, and split the 3 tests into 3 add_task calls with proper function names (depending on what the test is doing, like function* test_same-date-diff-hash)

@@ +44,5 @@
> +                  metaData.count + "_" + "differentHash==" + ".json";
> +  backupFile = OS.Path.join(backupFolder, filename);
> +  yield BookmarkJSONUtils.exportToFile(backupFile);
> +  PlacesBackups._backupFiles = null; // To force re-cache of backupFiles
> +  yield PlacesBackups.create(1, true); // Force compressed backup

ditto for arguments

@@ +50,5 @@
> +  // Decode lz4 compressed file to json and check if json is valid
> +  result = yield OS.File.read(mostRecentBackupFile, { compression: "lz4" });
> +  jsonString = converter.convertFromByteArray(result, result.length);
> +  validJSON = IsJson(jsonString);
> +  do_check_true(validJSON);

ditto for the check

@@ +64,5 @@
> +                  metaData.count + "_" + metaData.hash + ".json";
> +  backupFile = OS.Path.join(backupFolder, oldFilename);
> +  let newBackupFile = OS.Path.join(backupFolder, newFilename);
> +  yield BookmarkJSONUtils.exportToFile(backupFile);
> +  PlacesBackups._backupFiles = null; // To force re-cache of backupFiles

ditto

@@ +65,5 @@
> +  backupFile = OS.Path.join(backupFolder, oldFilename);
> +  let newBackupFile = OS.Path.join(backupFolder, newFilename);
> +  yield BookmarkJSONUtils.exportToFile(backupFile);
> +  PlacesBackups._backupFiles = null; // To force re-cache of backupFiles
> +  yield PlacesBackups.create(1, true);

ditto
Attachment #8432185 - Flags: review?(mak77) → feedback+
changed implemented
Attachment #8432185 - Attachment is obsolete: true
Attachment #8432444 - Flags: review?(mak77)
Comment on attachment 8432444 [details] [diff] [review]
bug1016953_renaming_uncompressed_backups.diff v2

Review of attachment 8432444 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/PlacesBackups.jsm
@@ +437,5 @@
>          // rename it as if it was today's backup.
>          this._backupFiles.shift();
>          this._entries.shift();
>          newBackupFile = mostRecentBackupFile;
> +        // if recent backup is json then backup filename is obtained for json

I'd prefer something like:

// Ensure we retain the proper extension when renaming
// the most recent backup file.

::: toolkit/components/places/tests/bookmarks/test_1016953-renaming-uncompressed.js
@@ +10,5 @@
> +*/
> +
> +add_task(function* test_same_date_same_hash() {
> +
> +  // If old file has been created on the same date and has the same hash

nit: I'm not a lover of newlines after function definition

@@ +28,5 @@
> +
> +  // Force a compressed backup which fallbacks to rename
> +  yield PlacesBackups.create();
> +  let mostRecentBackupFile = yield PlacesBackups.getMostRecentBackup();
> +  // check to ensure not renmaed to jsonlz4

typo: renmaed

@@ +29,5 @@
> +  // Force a compressed backup which fallbacks to rename
> +  yield PlacesBackups.create();
> +  let mostRecentBackupFile = yield PlacesBackups.getMostRecentBackup();
> +  // check to ensure not renmaed to jsonlz4
> +  do_check_eq(mostRecentBackupFile, backupFile);

Sorry to point out this late, but I just read the thread in mozilla.dev.platform, sounds like do_ functions are deprecated, this is the table for replacements:

* do_check_eq(a, b) —>  Assert.equal(a, b)
* do_check_neq(a, b) —> Assert.notEqual(a, b)
* do_check_true(expr) —> Assert.ok(expr)
* do_check_false(expr) —> Assert.ok(!expr)
* do_check_null(expr) —> Assert.equal(expr, null)

please use the new version and verify it works.
Attachment #8432444 - Flags: review?(mak77) → review+
Updated using the new Assert methods as well.
Attachment #8432444 - Attachment is obsolete: true
Attachment #8432621 - Flags: review?(mak77)
Comment on attachment 8432621 [details] [diff] [review]
bug1016953_renaming_uncompressed_backups.diff v3

Review of attachment 8432621 [details] [diff] [review]:
-----------------------------------------------------------------

yes, please push to try (run all unit tests on win32, linux, linux64 and mac, no need for other platforms and no need for Talos).

You can either use this page http://trychooser.pub.build.mozilla.org/
or install the trychooser extension to hg (the link is in the above page)
Usually you need something like try: -b o -p linux,linux64,macosx64,win32 -u all -t none
Attachment #8432621 - Flags: review?(mak77) → review+
looks like this is breaking toolkit/components/places/tests/bookmarks/test_818584-discard-duplicate-backups.js... backupFiles.length is returning 2 here
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/bookmarks/test_818584-discard-duplicate-backups.js#48

that means an enforced backup is not replacing an existing one but rather creating a new one. Maybe one of the two is compressed and the other one is not, and we end up keeping both?
So basically the error was in line 407 which attempts to get the backup file for the same date using the filename which has a jsonlz4 extension. Therefore it will be unable to find the json file and remove it.

> let backupFile = yield getBackupFileForSameDate(newBackupFilename);

Do the additional lines require further comments or is understandable what is going on?
Attachment #8432621 - Attachment is obsolete: true
Attachment #8435062 - Flags: review?(mak77)
forgot to include the version number.
Attachment #8435062 - Attachment is obsolete: true
Attachment #8435062 - Flags: review?(mak77)
Attachment #8435063 - Flags: review?(mak77)
Somehow managed to remove the added test from xpcshell.ini in the last patch.
Here is the corrected one.
Attachment #8435063 - Attachment is obsolete: true
Attachment #8435063 - Flags: review?(mak77)
Attachment #8435080 - Flags: review?(mak77)
Comment on attachment 8435080 [details] [diff] [review]
bug1016953_renaming_uncompressed_backups.diff v4

Review of attachment 8435080 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/PlacesBackups.jsm
@@ +405,4 @@
>        // 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)) ||
> +                       (yield getBackupFileForSameDate(backupFilename));

Wouldn't be saner to change getBackupFileForSameDate to return either compressed or uncompressed backups?
in the end the method name means "give me a backup file with the same date as this one", the extension shouldn't matter.
that basically means isFilenameWithSameDate is broken cause it shouldn't compare the extension.
so remove the check for the file extension as I have done in the above patch or make it match for "json or jsonlz4" only?
Attachment #8435080 - Attachment is obsolete: true
Attachment #8435080 - Flags: review?(mak77)
Attachment #8435743 - Flags: review?(mak77)
Comment on attachment 8435743 [details] [diff] [review]
bug1016953_renaming_uncompressed_backups.diff v5

Review of attachment 8435743 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/PlacesBackups.jsm
@@ +62,5 @@
>    let sourceMatches = aSourceName.match(filenamesRegex);
>    let targetMatches = aTargetName.match(filenamesRegex);
>  
>    return sourceMatches && targetMatches &&
> +         sourceMatches[1] == targetMatches[1];

I think that this is fine, the scope of this method is to check the date, plus the regex itself guarantees that the input is coherent.
I suspect the extension check was introduced in the past to filter out old html files from the backups folder. we don't have that issue anymore.
Attachment #8435743 - Flags: review?(mak77) → review+
So the last patch had the wrong commit message. This one has the correct commit message.
Try run for the latest : https://tbpl.mozilla.org/?tree=Try&rev=c37af6c81f38
Attachment #8435743 - Attachment is obsolete: true
uh I thought this landed already, sorry!
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/0c0a0265a9a0
Assignee: nobody → althaf.mozilla
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0c0a0265a9a0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla33
Whiteboard: p=0 s=33.1 [qa?]
Comment on attachment 8435902 [details] [diff] [review]
bug1016953_renaming_uncompressed_backups.diff v5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 818587
User impact if declined: the most recent backup in some cases may become unusable
Testing completed (on m-c, etc.): m-c, has an automated test
Risk to taking this patch (and alternatives if risky): low impact patch
String or IDL/UUID changes made by this patch: none
Attachment #8435902 - Flags: approval-mozilla-aurora?
marking qa-, since this is flagged in-testsuite+.  If anyone feels there should be manual QA here, feel free to change to qa+
Whiteboard: p=0 s=33.1 [qa?] → p=0 s=33.1 [qa-]
Status: RESOLVED → VERIFIED
Comment on attachment 8435902 [details] [diff] [review]
bug1016953_renaming_uncompressed_backups.diff v5

Aurora approval granted.
Attachment #8435902 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
What build did you have this happen on?
Flags: needinfo?(sasanaruto)
that backup is compressed and has the right extension, what's the bug?
Flags: needinfo?(sasanaruto)
You need to log in before you can comment on or make changes to this bug.