307033 - It is too easy to damage a remote calendar

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

Subscribing to a remote calendar means that every change will be uploaded. The
risk of damaging it is a bit too big. We need to make it less easy to destroy data.
A possible solution would be to have an option to 'import into a new calendar'
in the new calendar wizard. That would download the file into a local storage
calendar, so that the user can do what he wants with the data, but the file on
the server won't be changed.

Comment 1

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

I think we should try to do something about this for the next release, given the
alpha-status of it. We don't want to accidently delete lots of data due to a bug
in, for example, the ics provider or the views.

Comment 2

[Simon Paquet [:sipaq]]

IMO this should go into our alpha2 release.
We're now working over 10 weeks on 0.3alpha1 and IMO we're nearly there. We
should get this thing out as soon as possible, so more people can test our new
codebase and so we get less bug reports on 0.2

Comment 3

[James Bow]

I can confirm this behaviour. I have two calendars that are stored remotely that
I access from three installations of Mozilla. Occasionally, the events will
appear to vanish from the calendars. Looking at the calendar files on my hard
drive, I see that the end of the .ics files have been lopped off. The events can
be made to reappear by cutting the file to the last END:VEVENT and then adding
the line END:VCALENDAR, but this results in lost data.

Because the calendars overwrite the remote installation, the problems are
quickly propagated to the other calendars.

Comment 4

[Worcester12345]

This needs the "dataloss" key word, and there should also be a blocker status
for these bugs.

Comment 5

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

This bug is to work around other bugs that can lead to dataloss. This is not a
dataloss issue. You also can't confirm this bug, since there is nothing really
wrong. I just want to add a feature. And this bug already has a 'blocker'
status. Now that we are done with administrative things, we can go back to the
topic of the bug.

It has been suggested by dmose on irc to make a backup of the calendar file
(just plain file copy for ics) instead of the import thing i mentioned in
comment 0. And i agree that a backup is a better plan.

Comment 6

[Worcester12345]

(In reply to comment #3)
> I can confirm this behaviour. ... but this results in lost data.



(In reply to comment #5)
> This bug is to work around other bugs that can lead to dataloss. This is not a
> dataloss issue. ...

If it looks, smells, and tastes like data loss...

Comment 7

[Joey Minta]

Attachment: patch v1

Patches creates, by default, 7 backup copies for each remote calendar.	Backup
copies are made just prior to writing to the remote file.  1 copy is created
just before the first edit.  3 copies store the last 3 states of the calendar
before editing.  3 copies store the state of the calendar over the last three
24-hour periods.

Comment 8

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

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

>+            var profileDir = dirService.get("ProfD", CI.nsIFile);
...
>+            var localFile = Components.classes["@mozilla.org/file/local;1"]
>+                                      .createInstance(CI.nsILocalFile);
>+            // Appending 'cal' puts us inside the directory.  Actual backup
>+            // is named calBackupData-X.ics and needs savedDir as a target
>+            profileDir.append("backupData");
>+            var savedDir = profileDir.clone();
>+            profileDir.append('cal');
>+            localFile.leafName = profileDir.path;

Setting leafName doesn't sound like a good idea to me. Also, why are you
appending 'backupData' and 'cal'?
Just copy profileDir, and append a filename.

>+            function writeToFile(lFile) 
>+            {
>+                fileOutputStream.write(str, str.length);

You never defined |str|.

>+                function getIntPrefSafe(prefName, defaultValue)
...
>+                    catch (ex) {
>+                        backupBranch.setIntPref(prefName, defaultValue);

Don't set the pref it is isn't set. Just assing a default value. That allows us
to change the default later, if needed.

>+                for (var i = numBackupFiles; i > 1; i--) {

That should be i>0, otherwise you get n-1 backups, not n. Or something. I am a
bit confused about what this block does. Can you add comments?

>+                if (doDailyBackup) {
>+                    getCalendarManager().setCalendarPref(
>+                                   this, 'backup-time', new Date().getTime());

The indentation is a bit werid here. I would prefer:
+		     getCalendarManager().setCalendarPref(this, 'backup-time',
							  new
Date().getTime());

Comment 9

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

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

>+        var str = unicodeConverter.convertFromByteArray(result, result.length);
...
>+                fileOutputStream.write(str, str.length);

I think you need to pass in result here, since that's an array of bytes, not a
string. Writing requires a byte-array, not a string. (But test it with a
calendar with utf8 chars in it)
The other option is to recode back to a byta-array, if passing in result
directly doesn't work.

Comment 10

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

Attachment: new patch

This patch is based on jminta's patch. It uses nsIDownloader to download the
files, so that the risk of changing the data is minimized. (converting from and
to utf8 as in the original patch might result in dataloss)

Comment 11

[Joey Minta]

Comment on attachment 196057 [details] [diff] [review]
new patch

	 this.mUri = aUri;
+
	 this.refresh();
You said you wanted to call this.makeBackup('refresh'); here.  This causes
problems on its own, that aren't entirely trivial to solve:
+	 backupFile.append(makeName('edit',1));
We shouldn't make an edit-backup for refresh calls.  Otherwise 1.) We get edit
backups when we never touched the file. 2.) We get edit backups every time
Sunbird loads.

+	 if (!backupTime ||
+	     (new Date().getTime() > backupTime + backupDays*24*60*60*1000)) {
Minor:If no backup has been done before, this creates a daily AND an initial
backup.  We don't really need both.

+	     channel.asyncOpen(downloader, {});
New calendars don't exist yet, and then this fails.

makeBackup('edit') is being called when Sunbird loads.	You need to set
this.loading=true sooner to avoid this.

This would all be easier if we had that onInit method, but until we do, we need
to find a better way to do refresh-backups.

Comment 12

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

(In reply to comment #11)
> You said you wanted to call this.makeBackup('refresh'); here.  This causes
> problems on its own, that aren't entirely trivial to solve:

Then the easiest solution would be to not call backup from the uri setter. After
thinking about it, i don't think there is a need for it. It is only needed to
backup before uploading a change.

> +	 if (!backupTime ||
> +	     (new Date().getTime() > backupTime + backupDays*24*60*60*1000)) {
> Minor:If no backup has been done before, this creates a daily AND an initial
> backup.  We don't really need both.

It's easier that way, and i don't think it hurts.

> This would all be easier if we had that onInit method, but until we do, we need
> to find a better way to do refresh-backups.
> 
Or just not do refresh backups at all. Why should we?

Comment 13

[Joey Minta]

(In reply to comment #12)
> Or just not do refresh backups at all. Why should we?

I'm fine with not doing them, as long as we're 100% sure we never write before a
backup (this shouldn't be hard).  Let's see what a backup system that only does
write-backups would look like.  Don't forget the try/catch suggestion you also
mentioned, since profile directories don't exist in shell testing.

Comment 14

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

Attachment: patch v3

Patch updated. No more refresh backups, no more problems in xpcshell.

Comment 15

[Joey Minta]

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

+
	 this.refresh();
Nit: Lose the newline

+     * file each day, for 3 days. So even if the users doesn't notice the
Nit: Change "So even"->"That way, even", "users"->"user"

+     * After the back up is finished, will call doWrite.
Nit: it calls doWriteICS

+	 } catch(e) {
+	     // Backup dir wasn't found. Likely because we are running in
+	     // xpcshell. Don't die, but continue the upload
+	     this.doWriteICS();
+	 }
Failing silently scares me.  Let's dump something like ("Backup
failed:"+e+'\n') here, just in case someone reports backups not being made. 
Same for the other try/catch.

+	 // Now gow download the remote file, and store it somewhere local.
Nit: "gow"->"go"

+				  
.getService(Components.interfaces.nsIIOService);
Nit: This is > 80 chars.  You have CI, so use it.  Same for nsIDownloader.

+	 this.makeBackup('write');
makeBackup doesn't take an argument anymore.

Looks great!  I'll feel much better about 0.3a1 once this is in.  r=jminta with
those nits fixed

Comment 16

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

patch checked in