Closed Bug 972434 Opened 7 years ago Closed 7 years ago

All of the bookmarks backups disappeared

Categories

(Toolkit :: Places, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: dataloss, regression)

Attachments

(1 file, 1 obsolete file)

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.
Summary: backups disappeared → All of the bookmarks backups disappeared
Whiteboard: p=0
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.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #8375802 - Flags: review?(MattN+bmo)
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?
Attachment #8375802 - Flags: review?(MattN+bmo) → review+
Attached patch patch v1.1Splinter Review
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!
Attachment #8375802 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9cff8846380
Severity: normal → major
Flags: in-testsuite+
Target Milestone: --- → mozilla30
Keywords: dataloss
I merged this to central sooner since it should be in the next nightly.

https://hg.mozilla.org/mozilla-central/rev/6687d299c464
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
No longer blocks: fxdesktopbacklog
Whiteboard: p=0
You need to log in before you can comment on or make changes to this bug.