972434 - All of the bookmarks backups disappeared

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

For whatever reason, my bookmarkbackups folder today is empty, while yesterday it was not. I must investigate if this may be a regression of bug 824433, since it's quite bad.

Comment 1

[Marco Bonardo [:mak]]

ok I guess I found the issue here:

while (numberOfBackupsToDelete--)

if numberOfBackupsToDelete is negative we should not enter the loop, otherwise we keep looping until something throws not finding a backup to remove.

I'll modify (or add) a test to check for this condition.

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8375802 [details] [diff] [review]
patch v1

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

r=me. I confirmed the test failed prior to the patch and passes now.

::: toolkit/components/places/PlacesBackups.jsm
@@ +310,5 @@
>  
>        if (!aForceBackup) {
> +        let backupFiles = yield this.getBackupFiles();
> +        // If there are backups, limit them to aMaxBackups, if requested.
> +        if (backupFiles.length > 0 && aMaxBackups > -1 &&

re: aMaxBackups:
undefined > 1 === false whereas null > -1 === true
Should we protect against someone passing "null" for the optional aMaxBackups argument?

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.1

adds a typeof aMaxBackups == "number" check, and 2 tests, one passing null and one passing nothing (undefined), the null test failes without the additional check. Good catch!

Comment 5

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9cff8846380

Comment 6

[Marco Bonardo [:mak]]

I merged this to central sooner since it should be in the next nightly.

https://hg.mozilla.org/mozilla-central/rev/6687d299c464

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b9cff8846380