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

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Althaf Hameez [:ahameez]]

Attachment: bug1016953_renaming_uncompressed_backups.diff v1

took a bit longer than expected :)

Comment 2

[Marco Bonardo [:mak]]

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

Comment 3

[Althaf Hameez [:ahameez]]

Attachment: bug1016953_renaming_uncompressed_backups.diff v2

changed implemented

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Althaf Hameez [:ahameez]]

Attachment: bug1016953_renaming_uncompressed_backups.diff v3

Updated using the new Assert methods as well.

Comment 6

[Marco Bonardo [:mak]]

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

Comment 7

[Althaf Hameez [:ahameez]]

Try run
https://tbpl.mozilla.org/?tree=Try&rev=091956205036

Comment 8

[Marco Bonardo [:mak]]

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?

Comment 9

[Althaf Hameez [:ahameez]]

Attachment: bug1016953_renaming_uncompressed_backups.diff

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?

Comment 10

[Althaf Hameez [:ahameez]]

Attachment: bug1016953_renaming_uncompressed_backups.diff v4

forgot to include the version number.

Comment 11

[Althaf Hameez [:ahameez]]

Attachment: bug1016953_renaming_uncompressed_backups.diff v4

Somehow managed to remove the added test from xpcshell.ini in the last patch.
Here is the corrected one.

Comment 12

[Althaf Hameez [:ahameez]]

Try run 
https://tbpl.mozilla.org/?tree=Try&rev=b693359f7e80

Comment 13

[Marco Bonardo [:mak]]

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.

Comment 14

[Marco Bonardo [:mak]]

that basically means isFilenameWithSameDate is broken cause it shouldn't compare the extension.

Comment 15

[Althaf Hameez [:ahameez]]

Attachment: bug1016953_renaming_uncompressed_backups.diff v5

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?

Comment 16

[Marco Bonardo [:mak]]

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.

Comment 17

[Althaf Hameez [:ahameez]]

Attachment: bug1016953_renaming_uncompressed_backups.diff v5

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

Comment 18

[Marco Bonardo [:mak]]

uh I thought this landed already, sorry!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/0c0a0265a9a0

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c0a0265a9a0

Comment 21

[Marco Bonardo [:mak]]

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

Comment 22

[Tracy Walker [:tracy]]

marking qa-, since this is flagged in-testsuite+.  If anyone feels there should be manual QA here, feel free to change to qa+

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8435902 [details] [diff] [review]
bug1016953_renaming_uncompressed_backups.diff v5

Aurora approval granted.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/da411590e56d

Comment 25

[[e-mail]]

Attachment: bookmarks-2014-08-16_143_K6Z862ddBPKLxvZooBjVwQ==.jsonlz4

Comment 26

[Kevin Brosnan [Ex-Mozilla]]

What build did you have this happen on?

Comment 27

[Marco Bonardo [:mak]]

that backup is compressed and has the right extension, what's the bug?