1066992 - Exporting bookmarks doesn't honor browser.bookmarks.max_backups

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[sworddragon2]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140910020313

Steps to reproduce:

1. Create a new profile.
2. Start Firefox.
3. Go with a tab to about:config and set browser.bookmarks.max_backups to 0.
4. Right click on an empty place in the tab bar and click on "Menu Bar".
5. Click in the menu bar on Bookmarks -> "Show All Bookmarks" -> "Import and Backup" -> Backup... and save the file somewhere.


Actual results:

A backup was created in ~/.mozilla/firefox/_profile_/bookmarkbackups.


Expected results:

No backup should be created in this case.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yep. actually it should be removed the next time we try to backup, still we should not even copy it.

Comment 2

[Mark Banner (:standard8)]

We should probably also make sure that we limit the number of backups in the case where we're creating a new one for that day (and max_backups is not zero).

Comment 3

[Siddhant [:Siddhant085]]

I would like to work on this bug.

Comment 4

[Mark Banner (:standard8)]

Sure, I would suggest that looking at PlacesBackup.jsm is the best place to start. If you search for "PlacesBackups" you'll also find some existing tests that we have.

Comment 5

[Siddhant [:Siddhant085]]

The problem occurs because of the fix for bug 424389[https://bugzilla.mozilla.org/show_bug.cgi?id=1472486]. The new backup is created so that the daily backup file is updated to the point when the user manually creates a backup. I can solve this by adding in a check for the maximum number of backups before creating a new backup. If the number of backups are zero nothing is created, if the number of backups is maximum we should remove the oldest backup and add this new entry. However I am still in doubt if the default backup directory should be updated when the user manually creates a backup. The change to update the default directory was introduced by bug 424389[https://bugzilla.mozilla.org/show_bug.cgi?id=1472486]

Comment 6

[Mark Banner (:standard8)]

Reordered slightly...

(In reply to Siddhant from comment #5)
> The problem occurs because of the fix for bug
> 424389[https://bugzilla.mozilla.org/show_bug.cgi?id=1472486]. The new backup
> is created so that the daily backup file is updated to the point when the
> user manually creates a backup.

Yes, that is intentional. We know that the idle process which does the background backup doesn't always have a chance to kick in (because it is only run when there's an idle period). As a result, if the user is exporting a set of bookmarks then we've already paid the cost of creating it, so doing a quick copy is quite cheap, and ensures the backups are up to date.

> I can solve this by adding in a check for
> the maximum number of backups before creating a new backup.

Yes, that's the right thing to do.

> If the number of backups are zero nothing is created, if the number of backups is maximum we
> should remove the oldest backup and add this new entry.

Yes, I believe that check should happen after the new entry is created.

Comment 7

[Siddhant [:Siddhant085]]

Attachment: Exporting bookmarks doesn't honor browser.bookmarks.max_backups

Comment 8

[Siddhant [:Siddhant085]]

Comment on attachment 8997530 [details] [diff] [review]
Exporting bookmarks doesn't honor browser.bookmarks.max_backups

I did a manual test and it seems to be working fine. I also ran toolkit/components/places/tests/bookmarks/* and all the test completed successfully.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8997530 [details] [diff] [review]
Exporting bookmarks doesn't honor browser.bookmarks.max_backups

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

Thank you for working on this. There's a couple of comments below that I think will simplify the code slightly, and reduce unnecessary work.

This should also get tests to prevent regressions, and ensure the code is doing what we expect.

For testing that the backups are limited when a new bookmark export happens on that day, I think you could extend something on toolkit/components/places/tests/unit/test_utils_backups_create.js.

You could move most of that existing code to a function or two, and then have two add_tasks(), one would do the existing create() call, the other would be to call saveBookmarksToJSONFile (https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/toolkit/components/places/tests/bookmarks/test_818587_compress-bookmarks-backups.js#28-29)

For checking that there's no backup created when the maximum is zero, you could start with an empty backups directory (which should be the default for tests), call saveBookmarksToJSONFile, and then check `(await PlacesBackups.getBackupFiles()).length` is still zero.

::: toolkit/components/places/PlacesBackups.jsm
@@ +266,5 @@
>            // There is no backup for today, add the new one.
>            if (!this._backupFiles)
>              await this.getBackupFiles();
> +          if (aMaxBackup === 0)
> +            return nodeCount;

I think we can move this check right to the start of the else that goes with `OS.Path.dirname(aFilePath) == backupFolderPath`. 

There's no point in doing any of the backup work if we've got backups disabled.

@@ +269,5 @@
> +          if (aMaxBackup === 0)
> +            return nodeCount;
> +          if (this._backupFiles.length === aMaxBackup) {
> +            let oldestBackup = this._backupFiles.pop();
> +            await OS.FILE.remove(oldestBackup);

I think it would be worth moving the existing limitBackups function out of create() and into a separate function that's global just to this file, i.e. like getBackupFileForSameDate. It could be passed the max backup count, and the _backupFiles array.

This would make sure that we get the same functionality in all places.

(Note also, it is OS.File not OS.FILE).

Comment 10

[Siddhant [:Siddhant085]]

Mark I had made the changes and was almost done with the test cases, however my hard disk crashed. So I have lost all my data. It will take some time to get the hard disk replaced and redo the changes.

Comment 11

[Mark Banner (:standard8)]

(In reply to Siddhant from comment #10)
> Mark I had made the changes and was almost done with the test cases, however
> my hard disk crashed. So I have lost all my data. It will take some time to
> get the hard disk replaced and redo the changes.

Ouch, I'm sorry to hear that. Hope you're able to recover soon. There's no rush on this at the moment, so I'll keep it assigned to you.

Comment 12

[Siddhant [:Siddhant085]]

@standard8 I have made all the changes again. There is a small problem. getBackupFiles is a lazy getter and since we are modifying the cache folder externally in test it doesn't get reflected in the internal cache. Thus the tests work independently but fail when they are run together.

Comment 13

[Mark Banner (:standard8)]

(In reply to Siddhant from comment #12)
> There is a small problem.
> getBackupFiles is a lazy getter and since we are modifying the cache folder
> externally in test it doesn't get reflected in the internal cache. Thus the
> tests work independently but fail when they are run together.

I think there's two options for how to deal with this. One would be to put the two tests in separate files. The second one, which might be slightly easier an better for maintainability/readability, would be to `delete PlacesBackups._backupFiles;` in-between the tests - that would force it to re-initialise I think.

Comment 14

[Siddhant [:Siddhant085]]

Attachment: Bookmarks backup does not exceed max backups

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 9005282 [details] [diff] [review]
Bookmarks backup does not exceed max backups

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

I'm glad you were able to get set up again. Thank you for the updates. There's a few comments on the test to address, but I think we're not far off being able to land this now :-)

::: toolkit/components/places/tests/unit/test_utils_backups_create.js
@@ +78,4 @@
>      let entry = files.nextFile;
>      entry.remove(false);
>    }
> +  delete PlacesBackups._backupFiles; //As the internal cache doesn't get refreshed by itself

Please put the comment in a new line above, and make it something like:

// Clear the cache to match the manual removing of files.

Note the space between // and C (this keeps ESLint happy).

@@ +82,4 @@
>    Assert.ok(!bookmarksBackupDir.directoryEntries.hasMoreElements());
> +}
> +
> +add_task(async function() {

nit: we need to name these functions so that it is easier to distinguish test sections in the output, e.g. `add_task(async function test_create_backups() {`

@@ +96,3 @@
>  });
> +
> +add_task(async function() {

nit: test_saveBookmarks_with_no_backups

@@ +100,5 @@
> +  let bookmarksBackupDir = new FileUtils.File(backupFolderPath);
> +
> +  Services.prefs.setIntPref("browser.bookmarks.max_backups", 0);
> +
> +  let filePath = "/tmp/backup.json";

Not everyone has a tmp directory at /tmp unfortunately, so we need to make this:

let filePath = do_get_tempdir().path;

Please do the same for the other test.

@@ +104,5 @@
> +  let filePath = "/tmp/backup.json";
> +  await PlacesBackups.saveBookmarksToJSONFile(filePath);
> +  let files = bookmarksBackupDir.directoryEntries;
> +  if (files.hasMoreElements())
> +    do_throw("Backup should not exist: " + files.nextFile.leafName);

This if can be simplified to:

Assert.ok(!files.hasMoreElements(), "Should have no backup files");

@@ +109,5 @@
> +  await OS.File.remove(filePath);
> +  await cleanupFiles(bookmarksBackupDir);
> +});
> +
> +add_task(async function() {

nit: test_saveBookmarks_with_backups

Comment 16

[Siddhant [:Siddhant085]]

Attachment: Bookmarks backup should not exceed max backups

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 9005705 [details] [diff] [review]
Bookmarks backup should not exceed max backups

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

Thank you for the update. Looks good. I did a try push as well to make sure the tests are happy:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a2202f4f83ec5e4a482132d770ebd7eae7b587f

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69199edacdf6
Bookmarks backup should not exceed max backups a=standard8

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/69199edacdf6