477583 - Backups of bookmarks stops working if a future backup exists

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[fabio217]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6 (.NET CLR 3.5.30729)

Backups of bookmarks will stop working as soon as a *.json file with a newer date is found in the backup folder.
If you happen move ahead the date on your computer, to test some software in my case, and during that time open and close Mozilla a backup will be created and stamped with the fake date. Once you set the computer calendar to the real date and time Mozilla backup will stop working. 

Reproducible: Always

Steps to Reproduce:
1.Open and close all running instances of Mozilla.(This step should create a backup file. *.json)
2.Move forwared the computer date (lets say 30 days) 
3.Open and close Mozilla.
4.Check the backup folder for the latest backup file (*.json) It will be stamped with the current date +30 days
5.Move back the date to the current one.
6.Open Mozilla, then close it.
7.Check the backup folder and you will notice that no new backups are being made.
Actual Results:  
Backups of bookmarks stop working

Expected Results:  
Backup of bookmarks should work

The software has to be able to always make backups.  Probably, the software will have to label the backups differently to distinguish them from the rest.

Comment 1

[Shawn Wilsher :sdwilsh]

Do you mean automatic backups stop working?

Comment 2

[Marco Bonardo [:mak]]

have you by chance changed the number of backups value in about:config?

Comment 3

[fabio217]

(In reply to comment #1)
> Do you mean automatic backups stop working?

Yes, I mean automatic backups stop working

Comment 4

[fabio217]

(In reply to comment #2)
> have you by chance changed the number of backups value in about:config?

No, I have not changed the number of backups value.

Comment 5

[Tracy Walker [:tracy]]

I can confirm this behavior on Win XP Shiretoko nightly build from 20090414

Once the time was shifted forward then back, the only way to get an automatic backup is to remove all the backups.  Then, that one doesn't get updated on subsequent shutdowns.  Is there a timer that only updates after some period of time of browser usage?

Comment 6

[Marco Bonardo [:mak]]

bah, yeah, getMostRecentBackup is a bogus util.

Comment 7

[Marco Bonardo [:mak]]

non critical or major because requires a non common setup (not many people move on the clock, people doing that are usually testing something, so they are expert enough to workaround the problem removing the future backup)

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 9

[Marco Bonardo [:mak]]

this adds some cleanup to backup code, tries to reuse some common code paths to avoid bugs, groups things together.
i also increased number of backups to 10, due to recent sqlite corruption issues we have seen, that often are detected by users after some day (so 5 days are a really small amount of time).
I moved idle time before backup to 15 mins (was 1 hour), to reduce chances to run that at shutdown. Users with many bookmarks could experience slower shutdowns if that happens. I've not moved it to idle-daily because it would have less chances to run, and most likely would always run at first shutdown.

Also added a unit test for this specific (future backup) case.

Comment 10

[Asaf Romano (gone)]

Comment on attachment 399656 [details] [diff] [review]
patch v1.0

r=mano

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1.1

unbitrot

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v1.2

i forgot a qrefresh :(

Comment 13

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/798a53f68398

cc-ing Gavin since he is using this code in Feenec and if this lands on 1.9.2 he could need to update it, changes for import should be trivial though, just add the helper object in the middle: PU.backups.utilname

Comment 14

[:Gavin Sharp [email: [e-mail]]]

The renaming in this patch (PlacesUtils -> PlacesUtils.backups) isn't ideal - Fennec currently builds off of both trunk and branch gecko with the same frontend code. Since there's little practical benefit to that part of the change, can we just undo it?

Comment 15

[:Gavin Sharp [email: [e-mail]]]

I filed bug 520547 on that.

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> The renaming in this patch (PlacesUtils -> PlacesUtils.backups) isn't ideal -
> Fennec currently builds off of both trunk and branch gecko with the same
> frontend code. Since there's little practical benefit to that part of the
> change, can we just undo it?
So what, we can't ever rename anything that Fennec uses because it builds on two different branches?  That seems like it is unnecessarily tying the hands of developers.  Is Fennec shipping on both mozilla-central and 1.9.2 right now?

Comment 17

[Marco Bonardo [:mak]]

Gavin pointed out that the win is not worth the breakage, and finally i have to agree with him, i'll restore something in the filed bug.

But for the most part i agree fennec should move using separate branches not only for frontend since it's more likely in future the divergence between trunk and 1.9.2 APIs will get larger.

Comment 18

[Marco Bonardo [:mak]]

Attachment: roll-up patch for 1.9.2 branch

this patch includes fix for bug 520547, that address a compatibility concern, reducing risk of incompatibilities of existing code.

The patch comes with tests.
It brings the following improvements:
- finding/walking backups is faster
- if a future backup exists we will still create backups (actually we give up)
- backup on idle time is reduced to 15 minutes (to try avoid backing up at shutdown) from 1 hour
- number of backups increased to 10 to prevent dataloss when a not-immediately-visible database corruption happens

Comment 19

[Jesse Ruderman]

Attachment: fix js strict warning

Comment 20

[Marco Bonardo [:mak]]

We already have a reviewed patch that will fix that, i'll push it now, btw thanks

Comment 21

[Jesse Ruderman]

(That patch is in bug 520743.)

Comment 22

[Marco Bonardo [:mak]]

yeah, if we push the above patch to 1.9.2 we should still fix the strict warning here though.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #16)
> So what, we can't ever rename anything that Fennec uses because it builds on
> two different branches? That seems like it is unnecessarily tying the hands of
> developers.  Is Fennec shipping on both mozilla-central and 1.9.2 right now?

Sorry, I didn't see this comment until now. I didn't say "we can't ever rename anything" - your rephrasing is unnecessarily inflammatory.

Yes, Fennec currently runs on both 1.9.2 and trunk from a single front-end repository. This change introduced pain (API change, fennec broke on trunk) for practically no gain, so Marco and I agreed to revert it.

Fennec may have to deal with having separate code across multiple branches in the future, but hopefully only in response to a compatibility change whose benefits outweigh its cost!

Comment 24

[Gervase Markham [:gerv]]

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

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 404831 [details] [diff] [review]
roll-up patch for 1.9.2 branch

seems too late to be considering this on branch to solve an uncommon problem (per comment 7)