Closed Bug 423154 Opened 13 years ago Closed 13 years ago

off-by-one error for browser.bookmarks.max_backups

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta5

People

(Reporter: zeniko, Assigned: dietrich)

References

()

Details

(Keywords: privacy, regression)

Attachments

(1 file, 1 obsolete file)

First all but max_backups files are deleted, but then a new one is created...

NB: This same issue exists in two files, this bug's URL and
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp&rev=1.60&mark=2575,2587#2575

Besides: Setting max_backups to 0 is broken (should mean no backups at all, now means as many as possible).
Flags: blocking-firefox3?
Assignee: nobody → dietrich
Priority: -- → P2
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 3 beta5
Attached patch fix (obsolete) — Splinter Review
moves browser.bookmarks.max_backups to firefox.js (orig added in bug 307135)

now supports:

0: no backups created, old ones deleted (privacy mode)
-1: no cap on the number of backups created
Attachment #309821 - Flags: review?(mano)
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: Firefox 3 beta5 → Firefox 3
Whiteboard: [has patch][needs review mano]
Comment on attachment 309821 [details] [diff] [review]
fix

>Index: toolkit/components/places/src/utils.js
>===================================================================

leNames.length >= aNumberOfBackups) {
>-        var numberOfBackupsToDelete = backupFileNames.length - aNumberOfBackups;
>+      var numberOfBackupsToDelete = 0;
>+      if (aNumberOfBackups == -1)
>+        numberOfBackupsToDelete  = 0;
>+      else if (aNumberOfBackups == 0)
>+        numberOfBackupsToDelete  = backupFileNames.length;
>+      else if (backupFileNames.length >= aNumberOfBackups)
>+        numberOfBackupsToDelete = backupFileNames.length - aNumberOfBackups;
>+

that should probably be

>+      var numberOfBackupsToDelete = 0;
>+      if (aNumberOfBackups != -1) {
>+        if (aNumberOfBackups == 0)
>+          numberOfBackupsToDelete  = backupFileNames.length;
>+        else if (backupFileNames.length >= aNumberOfBackups)
>+          numberOfBackupsToDelete = backupFileNames.length - aNumberOfBackups;
>+      }

r=mano.
Attachment #309821 - Flags: review?(mano) → review+
(In reply to comment #2)
Or even simpler:
> >+      var numberOfBackupsToDelete = 0;
> >+      if (aNumberOfBackups > -1)
> >+        numberOfBackupsToDelete = backupFileNames.length - aNumberOfBackups;
Attached patch as checked inSplinter Review
thanks, and thanks

Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.7; previous revision: 1.6
done
Attachment #309821 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review mano]
Target Milestone: Firefox 3 → Firefox 3 beta5
a=beltzner for follow-up checkin ...
forgot to check in the removal of the html archival code (unused since JSON landed)

Checking in browser/components/places/public/nsIPlacesImportExportService.idl;
/cvsroot/mozilla/browser/components/places/public/nsIPlacesImportExportService.idl,v  <--  nsIPlacesImportExportService.idl
new revision: 1.4; previous revision: 1.3
done
Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v  <--  nsPlacesImportExportService.cpp
new revision: 1.61; previous revision: 1.60
done
Checking in browser/components/places/src/nsPlacesImportExportService.h;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.h,v  <--  nsPlacesImportExportService.h
new revision: 1.10; previous revision: 1.9
done
Status: RESOLVED → VERIFIED
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.