309366 - ICS provider backup issues

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

I just saw three consecutive errors on executing moveTo in a row while
attempting to do the pre-upload backup.  I suspect that this signals two problems:

* moveTo isnt overwriting like the code thinks it should be
* we need to have some try-catch blocks to handle errors around any disk actions

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

The problem happens when two changes are suitably close to each other in time. 
The backup semantics in nsLocalFileWin::CopySingleFile are bogus, as per the
newly-filed bug 309562.  We'll probably want to work around this problem somehow
so that we can hope to have Lightning run with versions of Gecko without a fix
and also file a followon bug to get rid of the workaround once we no longer care
about such versions.

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: don't rename files, just use a new name

This patch looks for old files to remove. It uses createUnique to ensure that
older files are not overwritten.
It also makes it so that no edit backup is made when a daily backup is created.
It  makes it slightly harder to look for the last backup (you need to look at
two series of files), but i don't think that's too big of a problem. It does
make the code simpler.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 197259 [details] [diff] [review]
don't rename files, just use a new name

>Index: providers/ics/calICSCalendar.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/providers/ics/calICSCalendar.js,v
>retrieving revision 1.23
>diff -u -9 -p -d -r1.23 calICSCalendar.js
>--- providers/ics/calICSCalendar.js	21 Sep 2005 21:34:11 -0000	1.23
>+++ providers/ics/calICSCalendar.js	24 Sep 2005 14:33:23 -0000
>@@ -146,20 +146,20 @@ calICSCalendar.prototype = {
>      * This will download the remote file into the profile dir.
>      * It should be called before every upload, so every change can be
>      * restored. By default, it will keep 3 backups. It also keeps one
>      * file each day, for 3 days. That way, even if the user doesn't notice
>      * the remote calendar has become corrupted, he will still loose max 1
>      * day of work.
>      * After the back up is finished, will call doWriteICS.
>      */
>     makeBackup: function() {
>-        function makeName(type, num) {
>-            return 'calBackupData_'+psuedoID+'_'+type+num.toString()+'.ics';
>+        function makeName(type) {
>+            return 'calBackupData_'+psuedoID+'_'+type+'.ics';

The correct spelling is "psuedoID".

>         var doDailyBackup = false;
>         var backupTime = getCalendarManager()
>                          .getCalendarPref(this, 'backup-time');
>         if (!backupTime ||
>             (new Date().getTime() > backupTime + backupDays*24*60*60*1000)) {
>             // It's time do to a daily backup
>             doDailyBackup = true;
>             getCalendarManager().setCalendarPref(this, 'backup-time', 
>                                                  new Date().getTime());
>         }
> 
>         var backupFile = backupDir.clone();
>-        backupFile.append(makeName('edit',1));
>+        if (doDailyBackup)
>+            backupFile.append(makeName('edit'));
>+        else
>+            backupFile.append(makeName('day'));
>+        backupFile.createUnique(CI.nsIFile.NORMAL_FILE_TYPE, 0600);

So this seems to suggest that one of the "edit" backups actually becomes the
"daily" backup rather than having it be in addition to the daily.  Is that
correct?  Maybe that's ok, but to me it's a surprising semantic.

>+        
>+        if (doDailyBackup)

This looks wrong since it means that the dirEnum below will only happen
sometimes.

>+        // Enumarate files in the backupdir for expiry of old backups

spelling: "Enumerate".

>+        var dirEnum = backupDir.directoryEntries;
>+        var files = [];
>+        while (dirEnum.hasMoreElements()) {
>+            var file = dirEnum.getNext().QueryInterface(CI.nsIFile);
>+            if (file.isFile()) {
>+                files.push({name: file.leafName, lastmodified: file.lastModifiedTime});
>+            }
>+        }
> 
>-        // Move the previous backup files up one index number, starting
>-        // with the second-to-last one.  The last is overwritten.
>-        for (var i = numBackupFiles-1; i > 0; i--) {
>-            var oldFile = backupDir.clone();
>-            oldFile.append(makeName('edit',i));
>-            if (oldFile.exists())
>-                oldFile.moveTo(backupDir, makeName('edit',i+1));
>-            if (doDailyBackup) {
>-                oldFile = backupDir.clone();
>-                oldFile.append(makeName('day',i));
>-                if (oldFile.exists())
>-                    oldFile.moveTo(backupDir, makeName('day',i+1));
>+        function removeOldBackups(files, type) {

Can you collect all the lexically nested functions at the beginning of the
function block?  It's hard to read when they're interspersed with the code that
calls them.

>+            // Filter out the edit backups.
>+            var filteredFiles = files.filter(
>+                function f(v) { 
>+                    return (v.name.indexOf("calBackupData_"+psuedoID+"_"+type) != -1)
>+                });
>+            // Sort by lastmodifed
>+            filteredFiles.sort(
>+                function s(a,b) {
>+                    return (a.lastmodified - b.lastmodified);
>+                })
>+            // And delete the oldest files, and keep the desired number of
>+            // old backups
>+            var i;
>+            for (i = 0; i < filteredFiles.length - numBackupFiles; ++i) {
>+                file = backupDir.clone();
>+                file.append(filteredFiles[i].name);
>+                file.remove(false);
>             }
>         }

Prefer explicit |return|s to falling off the end of a block, as it makes it
clear that this was the intent and not an inadvertant omission.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #3, wherein I wrote...)
>
> >     makeBackup: function() {
> >-        function makeName(type, num) {
> >-            return 'calBackupData_'+psuedoID+'_'+type+num.toString()+'.ics';
> >+        function makeName(type) {
> >+            return 'calBackupData_'+psuedoID+'_'+type+'.ics';
> 
> The correct spelling is "psuedoID".
> 

Hah.  By which I meant, of course, "pseudoID".  Sorry about that!

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: updated patch

Updated patch. It always makes an edit backup now.

Comment 6

[Joey Minta]

Comment on attachment 197589 [details] [diff] [review]
updated patch

My only concern is that our system can now be hurt if a user starts messing
around in his profile.	(ie. open a backupFile, click 'Save', changes
lastmodified)  Personally, I feel that anyone touching their profile folder is
asking for trouble and deserves what they get.	Since the only other solution I
can see is to actuall store the locations we make backups in, I don't mind this
tradeoff.  r=jminta

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

I believe we need a new patch that implements batch mode to avoid problems mvl
found with this one.  Is that correct?

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #7)
> I believe we need a new patch that implements batch mode to avoid problems mvl
> found with this one.  Is that correct?

The problem i found was that importing didn't set batchmode, which results in
lots and lots of (useless) backups.
The patch here isn't problematic, we just need to add support for setting
batchmode, and I think that should be done in another bug.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 197589 [details] [diff] [review]
updated patch

>+            dailyBackupFile.append(makeName('day'));
>+            backupFile.createUnique(CI.nsIFile.NORMAL_FILE_TYPE, 0600);
>+            dailyBackupFileName = backupFile.leafName;

Ofcourse those |backupFile| should be |dailyBackupFile|

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 197589 [details] [diff] [review]
updated patch

>-        var backupFile = backupDir.clone();
>-        backupFile.append(makeName('edit',1));
>+        // This is a bit messy. createUnique already creates a file,
>+        // and we want to copyTo is loater. copyTo needs a filename. The

"and we want to copyTo is loater" doesnt make sense to me.

>+        // creation of the file isn't really needed, but so be it. I don't
>+        // feel like re-implementing createUnique.
>+        var dailyBackupFileName;
>+        if (doDailyBackup) {
>+            var dailyBackupFile;
>+            dailyBackupFile = backupDir.clone();
>+            dailyBackupFile.append(makeName('day'));
>+            backupFile.createUnique(CI.nsIFile.NORMAL_FILE_TYPE, 0600);
>+            dailyBackupFileName = backupFile.leafName;
>+            dailyBackupFile = null;
>+        }
> 
>-        // Move the previous backup files up one index number, starting
>-        // with the second-to-last one.  The last is overwritten.
>-        for (var i = numBackupFiles-1; i > 0; i--) {
>-            var oldFile = backupDir.clone();
>-            oldFile.append(makeName('edit',i));
>-            if (oldFile.exists())
>-                oldFile.moveTo(backupDir, makeName('edit',i+1));
>-            if (doDailyBackup) {
>-                oldFile = backupDir.clone();
>-                oldFile.append(makeName('day',i));
>-                if (oldFile.exists())
>-                    oldFile.moveTo(backupDir, makeName('day',i+1));
>+        var backupFile = backupDir.clone();
>+        backupFile.append(makeName('edit'));
>+        backupFile.createUnique(CI.nsIFile.NORMAL_FILE_TYPE, 0600);
>+        
>+        // Enumerate files in the backupdir for expiry of old backups
>+        var dirEnum = backupDir.directoryEntries;
>+        var files = [];
>+        while (dirEnum.hasMoreElements()) {
>+            var file = dirEnum.getNext().QueryInterface(CI.nsIFile);
>+            if (file.isFile()) {
>+                files.push({name: file.leafName, lastmodified: file.lastModifiedTime});
>             }
>         }

Im still finding this code fairly difficult to read because the above low-level
naming logic is mixed in with the much higher-level start-a-backup logic that
is the rest of this function.  Can you factor this stuff out so that all the
naming logic is done cleaning in a sub-function?

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v3

Patch updated to review comments. Should be more readable now.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 198608 [details] [diff] [review]
patch v3

OK, this looks good; thanks!  I see one semantic problem and a few minor nits
left to address:

>+        // This is a bit messy. createUnique creates an empty file,
>+        // but we don't use that file. All we want is a filename, to be used
>+        // in the call to copyTo later. So we create a file, get the filename,
>+        // and never use the file again, but write over it.
>+        // Using createUnique anyway, because I don't feel like 
>+        // re-implementing it
>+        function makeDailyFileName() {
>+            var dailyBackupFile;
>+            dailyBackupFile = backupDir.clone();

Might as well merge the above two lines together.

>+            dailyBackupFile.append(makeName('day'));
>+            dailyBackupFile.createUnique(CI.nsIFile.NORMAL_FILE_TYPE, 0600);
>+            dailyBackupFileName = dailyBackupFile.leafName;
>+            dailyBackupFile = null;

Is the null assignment actually necessary?  On the other hand, I guess it can't
hurt.

>+            return dailyBackupFileName;
>+        }
>+
>+        function doPurgeOldBackups(files, type, files) {

How about calling this function "purgeBackupsByType", as that makes it more
clear how it differs from purgeOldBackups,
Also, how can a function have two arguments with the same name?

>+            // Filter out the edit backups.
>+            var filteredFiles = files.filter(
>+                function f(v) { 
>+                    return (v.name.indexOf("calBackupData_"+pseudoID+"_"+type) != -1)
>+                });

The comment above should actually be "filter out backups of the type we care
about", right?

>+            // Sort by lastmodifed
>+            filteredFiles.sort(
>+                function s(a,b) {
>+                    return (a.lastmodified - b.lastmodified);
>+                })

The above line looks like it wants a trailing semicolon.

>+            // And delete the oldest files, and keep the desired number of
>+            // old backups
>+            var i;
>+            for (i = 0; i < filteredFiles.length - numBackupFiles; ++i) {
>+                file = backupDir.clone();
>+                file.append(filteredFiles[i].name);
>+                file.remove(false);
>+            }
>+            return;
>+        }
>+
>+        function purgeOldBackups() {
>+            // Enumerate files in the backupdir for expiry of old backups
>+            var dirEnum = backupDir.directoryEntries;
>+            var files = [];
>+            while (dirEnum.hasMoreElements()) {
>+                var file = dirEnum.getNext().QueryInterface(CI.nsIFile);
>+                if (file.isFile()) {
>+                    files.push({name: file.leafName, lastmodified: file.lastModifiedTime});
>+                }
>+            }
>+
>+            if (doDailyBackup)
>+                doPurgeOldBackups(files, 'day', files);
>+            else
>+                doPurgeOldBackups(files, 'edit', files);
>         }

A |return| here would be nice.

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: another patch

patch updated to review comments.
(added a comment about the null assignment)

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 198733 [details] [diff] [review]
another patch

r=dmose; thanks!

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

patch checked in.