Last Comment Bug 329570 - Concurrent editing ICS calendars by multiple users can lose data
: Concurrent editing ICS calendars by multiple users can lose data
Status: RESOLVED FIXED
: dataloss, relnote
Product: Calendar
Classification: Client Software
Component: Provider: ICS/WebDAV (show other bugs)
: unspecified
: All All
: -- critical with 28 votes (vote)
: 1.0b1
Assigned To: Sebastian Schwieger
:
Mentors:
: 256890 287612 315109 357095 370517 381906 388925 398931 456978 468611 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-06 17:26 PST by Dan Mosedale (:dmose)
Modified: 2010-02-04 11:06 PST (History)
42 users (show)
philipp: wanted‑calendar1.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix - v1 (7.38 KB, patch)
2008-10-02 13:02 PDT, Daniel Boelzle [:dbo]
no flags Details | Diff | Splinter Review
[checked in] fix - v2 (7.48 KB, patch)
2008-10-08 04:40 PDT, Daniel Boelzle [:dbo]
mvl: review+
Details | Diff | Splinter Review
check timestamps v1 (6.02 KB, patch)
2009-02-25 07:22 PST, Sebastian Schwieger
philipp: review+
Details | Diff | Splinter Review

Description Dan Mosedale (:dmose) 2006-03-06 17:26:31 PST
The high-level issue is that the providers currently do not always detect if a remote calendar or event has been overwritten by someone else since it was last read.  

When such a change is detected (only possible in HTTP currently), a rather unfriendly dialog box with the HTTP 412 (Precondition failed) error is shown.  At this point, if the user restarts and then re-does the edit that failed, no data is lost.

The first step on this bug is to get to the point where we always detect such cases and show more friendly UI, or somehow indicate to the user in the UI in the cases where such detection is not possible (or stop supporting configurations where detection is impossible).  Further down the road, we should attempt to merge the client change into the new server version automatically.

In particular:

a) with HTTP WebDAV calendars, if the server always returns an ETag, 
all such changes should be detected.  Current versions of Apache mod_dav do not behave this way, however.  So while many server-side edits are likely to be detected, there are points in time where they may not be.

b) with FTP "WebDAV" calendars there is currently no good way to detect this.  Perhaps an X- property at the top-level of the ICS file could be used as a revision number to at least allow cooperating clients that respect it to avoid stomping on each other.

c) with file: "WebDAV" calendars, the X- property could be a start.  Additionally, file-locking would help.

d) CalDAV currently does not detect collisions at all.  This should be fairly straightforward to fix, I think.

e) Im not currently aware of plans to support sqlite calendar access outside of a single profile; if that should change, we will need to worry about that case as well.
Comment 1 Joey Minta 2006-03-28 07:35:03 PST
*** Bug 315109 has been marked as a duplicate of this bug. ***
Comment 2 Reed Loden [:reed] (use needinfo?) 2006-07-19 21:22:06 PDT
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Comment 3 Joey Minta 2006-08-02 15:34:38 PDT
I think we got the 80% case here with the etag.  I'm not convinced we should block on all the edges, but I could be convinced otherwise.  Re-nominate if there's a good case we're missing here.
Comment 4 Jeff Mitchell 2006-09-20 15:41:55 PDT
I don't know all the capabilities of the different protocols, but there are ways to make this kind of writing atomic, pulled from concurrent computing theory.  Some writes may fail if several are tried at once, but you won't get corruption.  You do have to make a copy of the file to do it, and you need to be able to get all of the file's data to compare hash values.  I can provide details if anyone is interested, but if the protocols aren't up to it, there's no point.
Comment 5 Matthew (lilmatt) Willis 2006-09-24 17:21:19 PDT
De-scarifying the bug's title a bit since we're relnoting this.
Comment 6 Stefan Sitter 2006-10-18 03:31:45 PDT
*** Bug 357095 has been marked as a duplicate of this bug. ***
Comment 7 Paul K 2006-11-07 05:36:48 PST
I'm not sure about all the details as stated in previous comments, but what I find is that when you enter new data, the code does not re-read the calendar data before appending your new data. What should happen, I believe, is when adding new data, the code should 1st lock the calendar data file, 2nd re-read to make sure to have current data, 3rd add new data (or edit existing data), 4th re-write the calendar data, and finally unlock the file. I wish I knew how to make this happen!
Comment 8 Bruno Browning 2006-12-03 20:35:50 PST
The CalDAV/etag part of this is being dealt with in bug 362698
Comment 9 Bas van den Bosch 2007-02-28 22:25:36 PST
bug 352333 and bug 287612 talk about the read before put. Also, to a lesser
extend, bug 329570.
Comment 10 cmtalbert 2007-03-22 14:52:20 PDT
*** Bug 256890 has been marked as a duplicate of this bug. ***
Comment 11 Martin Fleurke 2007-05-26 08:53:42 PDT
*** Bug 381906 has been marked as a duplicate of this bug. ***
Comment 12 Stefan Sitter 2007-07-20 02:20:35 PDT
*** Bug 388925 has been marked as a duplicate of this bug. ***
Comment 13 Andrew Daviel 2007-07-21 14:54:06 PDT
As per 388925, Sunbird is doing a GET before PUT, it's just not checking the content to realize that it changed. At least, it does not if the ICS store does not support ETag/If/Propfind. 
Comment 14 Bas van den Bosch 2007-09-16 11:03:07 PDT
Do we keep relnoting this? It's major dataloss for something which should have been fixed already (multiple-user support for webdav, ftp and local files is useless now)... 
Comment 15 Simon Paquet [:sipaq] 2007-09-16 12:11:55 PDT
Yes, this will stay in the release notes. We will try to fix this in the upcoming releases, but not in the 0.7 timeframe, which is nearly over as we're going to prepare a release candidate in the coming week.

In addition, there's no patch available for this bug, so there's really nothing that can be done here now.
Comment 16 Michiel van Leeuwen (email: mvl+moz@) 2007-09-17 08:39:43 PDT
*** Bug 287612 has been marked as a duplicate of this bug. ***
Comment 17 Kevin 2007-10-08 15:32:59 PDT
*** Bug 398931 has been marked as a duplicate of this bug. ***
Comment 18 Kristof Petr 2007-10-21 07:08:07 PDT
(In reply to comment #0)

> a) with HTTP WebDAV calendars, if the server always returns an ETag, 
> all such changes should be detected.  Current versions of Apache mod_dav do not
> behave this way, however.  So while many server-side edits are likely to be
> detected, there are points in time where they may not be.

Is Dan's statement right? I did quick test on my servers (simple tcpdump of communication between client and server) and apache 2.2.3 + mod_dav returns ETag always.

CalDAV provider was fixed (bug #362698) to work with ETag. Can this be done the same way for WebDAV? Or am I wrong? Is there one shared code to serve WebDAV and CalDAV store?
Comment 19 Anthony 2007-12-04 01:46:21 PST
Is there any possiblitly that this issue will be addressed in the upcoming 0.8 release?

Unless I'm mistaken, as it is, the shared calendar functionality has been rendered useless pretty much since v0.3 using either a CalDAV or WebDAV store.
Comment 20 Bruno Browning 2007-12-04 03:05:51 PST
The bug summary is somewhat out of date: the CalDAV part of this has been dealt with in bugs 362698 and 373370. 
Comment 21 Anthony 2007-12-04 04:48:17 PST
(In reply to comment #20)
> The bug summary is somewhat out of date: the CalDAV part of this has been dealt
> with in bugs 362698 and 373370.

Bruno, as I understand it, these bugs deal with etag management. Currently, using WebDAV I get a 412 error when a calendar has been modified by another user, and the calendar is set to 'Read-Only'. Am I correct in understanding that with 373370, updates are now integrated on the server copy with CalDAV?
Comment 22 Bruno Browning 2007-12-04 05:34:09 PST
(In reply to comment #21)
> (In reply to comment #20)
> Am I correct in understanding
> that with 373370, updates are now integrated on the server copy with CalDAV?
> 

With current nightlies, when the server responds to a CalDAV request with a 412 Sb/Ltn warns the user ("item has been modified on server", "item has been deleted on server") and asks how to proceed, i.e. reload from server or perform the requested action in spite of the changed server state. Or at least it does for me; if it doesn't do so for you please file a bug. :-)

With CalDAV, the domain of the 412 is the single VEVENT or VTODO referrenced in the request. With WebDAV/ICS, the domain is the entire calendar and all the VEVENTs/VTODOs it contains, which make this a much gnarlier problem to solve for the ICS provider. 
Comment 23 Marco van Beek 2007-12-20 04:20:13 PST
Hi,

I have just been checking datestamps on my WebDAV lock file and changes made on a network calendar within Sunbird 0.7 does not cause Apache to write a lock, but doing exactly the same with another ics calendar program does, so there may be something wrong with file locking requested to http / https WebDAV servers.
Comment 24 Simon Paquet [:sipaq] 2008-02-08 01:12:01 PST
Not going to happen for 0.8.
Comment 25 Marco van Beek 2008-02-08 01:17:55 PST
I do not understand why this bug is not classified as "Critical". The definition of critical is "crashes, loss of data, severe memory leak". Since this bug causes reproducible loss of data it fits the category, therefore it should receive much higher priority.
Comment 26 Lars Wohlfahrt (thetux) 2008-02-08 01:21:15 PST
In my opinion this bug is highly dangerous. Can't understand why this does not block 0.8
Comment 27 Jon Jones 2008-02-08 10:34:09 PST
(In reply to comment #26)
> In my opinion this bug is highly dangerous. Can't understand why this does not
> block 0.8
> 

Especially since it is not only highly dangerous but also has been around for several releases now and keeps getting pushed aside.
Comment 28 Sebastian Schwieger 2008-02-09 12:27:58 PST
(In reply to comment #25 - #27)
I do not understand why you are so keen on this bug. We have etag checking for webdav calendars. Are you using ftp/file protocols or is something wrong with etag checking?
Comment 29 Marco van Beek 2008-02-10 01:57:15 PST
All I know is that Sunbird appears to be doing no file locking when using WebDav via Apache whereas other iCal programs I have used are (but none have the functionality & versatility of Sunbird). It means that if you have two people editing the same calendar, one can overwrite the other's work. Especially in an environment where you have one person working remotely over a slower bandwidth link, and the other in the office on the local LAN. What I have seen happen, and I can see no reason why this cannot happen again under my understanding of how etags are supposed to work, is that both parties make a change calendar change. At the point of saving Sunbird checks to see if the file has changed, and if it has, it downloads a new copy, so by now the local user has already got their new version, and has been able to save it back to the server, while the remote user is still downloading. The remote user's copy of Sunbird then saves the copy back overwritting the local users changes. It is a very narrow window of opportunity, but I have seen it happen, and the more people who access the calendar, the more likely it is to occur. What should happen is that the first person to click on save should be granted a lock, which would prevent the second person from uploading their changes until the lock was released, at which point Sunbird would be required to download any changes again. There is a lock timeout option which should allow for any stale locks created by lost connections or crashed programs, as well as methods to discover active locks before writing.

I do not know if the ETag methods used within Sunbird are supposed to prevent this, but although this bug is difficult to reproduce, I see no way within the ETag system to prevent it, and I assume that is also why the WebDAV protocol has a locking mechanism in place.
Comment 30 Sebastian Schwieger 2008-02-10 02:34:07 PST
(In reply to comment #29)
Thanks for clarification. WebDAV locking should be tracked in bug 290446.
Comment 31 max.t 2008-02-10 03:19:30 PST
(In reply to comment #28)
> (In reply to comment #25 - #27)
> I do not understand why you are so keen on this bug. We have etag checking for
> webdav calendars. Are you using ftp/file protocols or is something wrong with
> etag checking?

To my knowledge, etag checking just shows that the calendar you are going to edit was changed by another user on the server. After that you manually have to refresh this remote calendar and then you have to publish your changes again (keeping fingers cross that know other changes were made in the meantime). 

Locking the calendar file on the server, downloading the actual version from the server, merging your changes and finally republishing the calendar should automatically be done by lightning or sunbird and not manually by the user (who can't even lock the file on the server). 
Comment 32 Sebastian Schwieger 2008-02-10 04:01:04 PST
(In reply to comment #31)
Merging is a different story again. Handled in bug 274967. I agree that the current behaviour is far from user friendly.
Comment 33 Paul K 2008-02-10 05:07:15 PST
Wrap the issue up in any bug you like, it's all the same problem. It's just odd that it worked in 0.2 (The version I am stuck on because of this) and hasn't worked since. It's not like this has never worked.
Comment 34 Marco van Beek 2008-02-10 07:31:37 PST
I agree that fixing bug 290446 is likely to solve this problem. That bug has been on the books for even longer, 2 months shy of 3 years. The latest comment from the developers as to why this will not be fixed is "The reason is primarily that we have a different focus". Critical data loss not part of the developers main focus then? Until somebody can explain what is more important than fixing catastrophic data loss I remain convinced that this bug (and the related bug 290446) should block any further releases.
Comment 35 Simon Paquet [:sipaq] 2008-02-17 16:16:51 PST
*** Bug 370517 has been marked as a duplicate of this bug. ***
Comment 36 Gary Kwong [:gkw] [:nth10sd] 2008-04-09 18:48:58 PDT
Please only nominate (?) bugs, not approve (+) as blocking, unless you're on the Calendar team.
Comment 37 Greg Trounson 2008-04-09 18:56:03 PDT
Sorry I thought I was nominating it.  As I'm not on the dev team I would have thought Bugzilla would automatically hide options to approve anything.

Thanks for the clarification
Comment 38 Martin Schröder [:mschroeder] 2008-06-07 05:15:45 PDT
Changing summary and component to reflect fixed CalDAV portion of the bug.
Comment 39 Daniel Boelzle [:dbo] 2008-09-29 15:41:33 PDT
Could anyone please convince me why we should hold locks, and what strategy we should follow on locking? It's still unclear to me what that buys us, since etag checking is in place and seems to work, preventing dataloss, because the PUT won't happen if there's been a concurrent change on the file.
I agree that we could do better on refreshing the file once we've detected such a conflict, e.g. the same provider operation could most likely be continued.
Comment 40 Anthony 2008-09-30 02:46:41 PDT
This problem has been kicking about for so long we've kind of lost the essence in a spaghetti of issues and bugs related to it. Personally, I'm very confused.

With etag checking, should concurrent editing be working properly with BOTH CalDav and WebDav?
My experience with WebDav is that this continues to present problems. Comment 22 by Bruno seems to affirm this. Perhaps Damiel could elaborate further on how the etag mechanism works with WebDav and how it prevents dataloss.
Comment 41 Daniel Boelzle [:dbo] 2008-09-30 04:22:52 PDT
The etag check is implemented here:
<http://mxr.mozilla.org/comm-central/source/calendar/providers/ics/calICSCalendar.js#820>
The relevant WebDAV section is here:
<http://rfc.net/rfc2518.html#s9.4>

The etag check seems to work fine when issuing a concurrent PUT, I get a MODIFICATION_FAILED (admittedly odd error message though).
But reloading the calendar (getting an up to date copy of the ics file) doesn't work for me on these occasions. However the etag seems to be updated(!), and modifying that calendar once again overwrites the concurrent changes...
Comment 42 Bruno Browning 2008-09-30 04:27:22 PDT
I agree with Daniel here. Locking is great when you need to make changes to
several things in an atomic fashion, but in this case we're only changing a
single resource and the etag checking should be sufficient. 

This bug, as written, is about "lost update" problems when >1 people are using
an ICS calendar. Presumably those could be file:/// or ftp:// URLs as well as
http://, but the discussion has been primarily about WebDAV and certainly
that's where locking would be relevant. Presuming that the etag checking is
working properly I would say that this bug is solved wrt WebDAV though not the
others: if the WebDAV resource has changed, the server returns a 412 and we do
not overwrite the changed data there.

I agree that we could handle the 412 better. Currently we throw an error and
abort the write (and on the test I just did, update the display as if the write
had completed, another bug). Instead we could refresh from the server and
re-attempt the add/modify/delete operation. But to my mind that's a different
bug than the stop-dataloss bug. 

IMO this bug is now relevant only for ics-over-ftp and ics-over-file:/// and
the 412-handling for ics-over-webdav should be handled in a separate bug. DAV
locking is not needed in any case.
Comment 43 Anthony 2008-09-30 04:33:26 PDT
Code and RFC's...I was really hoping for a short high level summary of the logic :( .

Sorry Daniel but I'm still unclear. Are you saying that the behaviour with WebDav is satisfactory / according to 'spec'?
Comment 44 Anthony 2008-09-30 04:39:05 PDT
I just read through Bruno's comments and I think I understand what he's saying; The user thinks his change has been integrated, whereas from what Bruno's saying the server has actually responded with a 412 error which was simply ignored.

This would explain the symptoms, but in my mind this is still a form of data loss, no?
Comment 45 Daniel Boelzle [:dbo] 2008-09-30 04:43:32 PDT
(In reply to comment #43)
> Code and RFC's...I was really hoping for a short high level summary of the
> logic :( .
It's saying that if the etag doesn't match the server's etag (which is updated on every PUT), then the PUT attempt will fail.

> Sorry Daniel but I'm still unclear. Are you saying that the behaviour with
> WebDav is satisfactory / according to 'spec'?
Yes, I think so.

(In reply to comment #44)
> I just read through Bruno's comments and I think I understand what he's saying;
> The user thinks his change has been integrated, whereas from what Bruno's
> saying the server has actually responded with a 412 error which was simply
> ignored.
No, that's not my experience. The PUT seems to fail (i.e. the etag check seems to work) whereas the reload just updates the etag, leaving stale calendar data. Another PUT then overwrites the concurrently PUT data.
Comment 46 Anthony 2008-09-30 04:54:08 PDT
Thanks for the clarification Daniel. So from what you're saying, the second time you do the PUT you'll upload stale data and overwrite the other user's previous update (during the concurrent PUT). So again we've lost data.
Comment 47 Paul K 2008-09-30 05:50:43 PDT
When I test 0.9 and the PUT fails, it locks the calendar to read-only. I could not attempt a second PUT without re-reading. I'm still running 0.2 because it will automatically re-read the calendar data, add my new data, and then write the new package to the remote location. Why is it that it worked prior, but seems to be avoided ever since???
Comment 48 Phil Anderson 2008-09-30 05:57:49 PDT
I'm in the same situation as Paul K.  I'm happily using 0.2 for all my users, so that calendars don't get put in read-only mode.  All higher versions are unworkable with multipile users.
Comment 49 Bas van den Bosch 2008-09-30 09:24:31 PDT
(In reply to comment #42)
> This bug, as written, is about "lost update" problems when >1 people are using
> an ICS calendar. Presumably those could be file:/// or ftp:// URLs as well as
> http://, but the discussion has been primarily about WebDAV and certainly
> that's where locking would be relevant. Presuming that the etag checking is
> working properly I would say that this bug is solved wrt WebDAV though not the
> others: if the WebDAV resource has changed, the server returns a 412 and we do
> not overwrite the changed data there.

Bug 398931 which was marked as a duplicate of this one. 
 
> I agree that we could handle the 412 better. Currently we throw an error and
> abort the write (and on the test I just did, update the display as if the write
> had completed, another bug). Instead we could refresh from the server and
> re-attempt the add/modify/delete operation. But to my mind that's a different
> bug than the stop-dataloss bug. 

bug 287612

> IMO this bug is now relevant only for ics-over-ftp and ics-over-file:/// and
> the 412-handling for ics-over-webdav should be handled in a separate bug. DAV
> locking is not needed in any case.

I agree, re-reading on modified ETAG should be done automatically. However, this precessing of external calendars should be quicker (bug 412914 and others)
Comment 50 Daniel Boelzle [:dbo] 2008-10-02 13:02:21 PDT
Created attachment 341488 [details] [diff] [review]
fix - v1

The etag has been refetched in case of errors (e.g. 412, precondition failed). This has lead to the situation that the etag is up-to-date, but the calendar data not. Another modification will overwrite concurrent changes.
This patch fixes this, triggering a refresh in case of a mid-air collision. Although this dumps the recent change(s) to the calendar, IMO it's better: IMHO the calendar must not be written any longer, because it's based on an outdated revision.

Of course, it's even better if we would merge in the recent changes to the updated file (most often this works if e.g. distinct items have been modified); I am leaving this for a follow-up patch.
Comment 51 Daniel Boelzle [:dbo] 2008-10-08 04:40:28 PDT
Created attachment 342224 [details] [diff] [review]
[checked in] fix - v2

- added check for listener
Comment 52 Michiel van Leeuwen (email: mvl+moz@) 2008-10-25 09:50:31 PDT
Comment on attachment 342224 [details] [diff] [review]
[checked in] fix - v2

I finally understand what is going on here (I missed a very important refresh() call before). Patch looks good. I do wonder how the user gets notified of the failed attempt to change. Or is an attempt made to re-apply the change?

r=mvl
Comment 53 Daniel Boelzle [:dbo] 2008-10-25 10:15:43 PDT
@mvl: On a failed PUT, the user will get notified (via calIObserver::onError).

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/106c97fa0fcf>

This is a fix to properly check and refresh the etag and calendar; taking from blocking-list since this is fixed now.

What seems to be left from the initial bug description is ftp "webdav" calendars, however we could fix that at all...

Suggestions, opinions?
Comment 54 Stefan Sitter 2008-12-09 07:43:34 PST
*** Bug 468611 has been marked as a duplicate of this bug. ***
Comment 55 Daniel Boelzle [:dbo] 2008-12-31 02:36:52 PST
(In reply to comment #0)
> b) with FTP "WebDAV" calendars there is currently no good way to detect this. 
> Perhaps an X- property at the top-level of the ICS file could be used as a
> revision number to at least allow cooperating clients that respect it to avoid
> stomping on each other.
An X-prop would certainly lower the risk, but since the file would need to be read/checked/written in one atomic operation, it's not a safe one.

> c) with file: "WebDAV" calendars, the X- property could be a start. 
> Additionally, file-locking would help.
In general, I am quit uneasy about file locking, esp. on network drives. IMO b) alone would certainly limit the risk sufficiently.
Comment 56 kujub 2009-02-07 10:41:27 PST
Is there any chance for this to get fixed in 1.0 / Thunderbird 3 ?
Comment 57 Philipp Kewisch [:Fallen] 2009-02-09 10:05:14 PST
Kurt, please don't request all possible flags at once. I don't think this should be blocking the next release. This is only an issue for ftp/webdav calendars and There is no standardized way to do this for ftp. I guess some hack like http://search.cpan.org/~ronaldws/LockFile-NetLock-0.29/NetLock.pm could be done. For local files we might be able to compare the timestamp.
Comment 58 Dan Mosedale (:dmose) 2009-02-09 11:55:07 PST
I'd suggest that the most interesting piece of this to fix is the WebDAV piece.  Perhaps FTP support should be pushed entirely to an extension.
Comment 59 Marco van Beek 2009-02-09 12:06:23 PST
I would agree with comment 58. WebDav has the option to write lock files before editing. Implementing this would solve this problem. FTP was never designed for file sharing, whereas WebDav is specifically designed for shared working. I lost another shared calendar the other day. This is a real day-to-day problem.
Comment 60 kujub 2009-02-10 01:55:53 PST
(In reply to comment #57)
> I don't think this should be blocking the next release.
This is very sad news IMHO.

I think this is a major issue for all those small office and home users sharing their calendar files on a LAN. They will use file:-URIs on their networks and even ftp: with some NAS devices if these seem to be supported. Beside ftp: as far as I can see there is no reliable file locking when using file-URIs eiher, at least when using some mounted network resource (ftp again), right ? So there will be many users loosing data and get very disappointed when trying the shiny brand new TB 3.0 with integrated calendar.
This will not increase adoption of Thunderbird. :-/

I suggest limiting calendar files to read-only access when using file: or ftp: and additionaly fix bug 356002 (which makes read-only calendars with enabled alarms unusable).
If this would be to much effort access to network calendar files should be allowed only with webdav.

Additionally I agree with comment #59
WebDAV access should be fixed to use lock files. WebDAV will be the only save and easy way to share a calendar or simply use the same one on more then one machine for those who don't want to use some calendar web service. ( People who like web calendars like web mail too and are unlikely to use TB at all. :-) )
Comment 61 Lukas Ruf 2009-02-10 02:35:16 PST
In reply to (In reply to comment #60)
> (In reply to comment #57)
> > I don't think this should be blocking the next release.
> This is very sad news IMHO.
>

I agree this should not block the next release.  Please just add a predominant warning for this race condition.
 
> I think this is a major issue for all those small office and home users 
> sharing their calendar files on a LAN. 

I totally agree: One of the greatest benefits Thunderbird offers!

> I suggest limiting calendar files to read-only access when using file: or ftp:
> and additionaly fix bug 356002 (which makes read-only calendars with enabled
> alarms unusable).
> If this would be to much effort access to network calendar files should be
> allowed only with webdav.
> 

I strongly disagree.  This would kill the benefits of Thunderbird -- and therewith Sunbird too.  Please don't.

Thanks!

Lukas
Comment 62 Philipp Kewisch [:Fallen] 2009-02-10 02:58:39 PST
Isn't the webdav bit already fixed since we use etags? locking is not needed if the correct headers are sent (if-none-match/if-match). This makes sure that the PUT is only done if the current etag matches the one specified in the headers.
Comment 63 max.t 2009-02-10 03:18:57 PST
(In reply to comment #62)
> Isn't the webdav bit already fixed since we use etags? locking is not needed if
> the correct headers are sent (if-none-match/if-match). This makes sure that the
> PUT is only done if the current etag matches the one specified in the headers.

This prevents data loss, but in case of an unsuccesful PUT (Error: MODIFICATION_FAILED) the user has to uncheck the read-only option by hand before trying to write the changes again. Locking could help to prevent users from this work around.
Comment 64 kujub 2009-02-10 03:23:17 PST
(In reply to comment #61)
> I agree this should not block the next release.  Please just add a predominant
> warning for this race condition.
OK, but just let me say that this would help only for situations where only one person is allowed to write to the shared calendar file. This isn't guaranteed on simple share level networks where two or more persons work or live together.
Comment 65 Sebastian Schwieger 2009-02-10 03:46:24 PST
I had once played around with the timestamp option for local files and I think I had working code back then. I might look into this again if it is wanted.
Comment 66 kujub 2009-02-10 04:48:18 PST
(In reply to comment #65)
> I had once played around with the timestamp option for local files and I think
> I had working code back then. I might look into this again if it is wanted.

Hmm, AFAIK on FAT-formated storage devices (USB-Stick or even hdd attached to some router/NAS device for example) the granularity of timestamps is limited to 2 seconds. So there still remains a time slot of two seconds during which some writing to the file could happen without any change of the timestamp. This could cause loosing at least those changes written before (if the file would be rewritten completely) or even file corruption (if only some changes would be written to somewhere into the file).
Do I miss something ?
Comment 67 Paul K 2009-02-10 04:56:31 PST
Could someone smarter than me look at Sunbird v.0.2? It works with that
version, but I am unsure how much the code has changed since then. I understand
the concern of file locking on network drives, but I think the current
configuration, while addressing file integrity, is unacceptable in practice. My
setup would never exist with current versions. Not many here are going to
manually de-select read-only setting and rewrite their data with a chance of
identical problem. If a network drive becomes unavailable while locked, the
chances are no one else is going to have access to the locked, orphaned
calendar file until further intervention.
Comment 68 Marco van Beek 2009-02-10 05:00:03 PST
I can confirm that using Sunbird with two users on a WebDav http server still leads to occasional data loss with the latest version. As far as I have been able to determine, the user gets no errors, the file is not put into read only, it just gets overwritten.
Comment 69 kujub 2009-02-10 07:53:05 PST
(In reply to comment #67)
> Could someone smarter than me look at Sunbird v.0.2? It works with that
> version, but I am unsure how much the code has changed since then. I understand
> the concern of file locking on network drives, but I think the current
> configuration, while addressing file integrity, is unacceptable in practice. My
> setup would never exist with current versions. Not many here are going to
> manually de-select read-only setting and rewrite their data with a chance of
> identical problem. If a network drive becomes unavailable while locked, the
> chances are no one else is going to have access to the locked, orphaned
> calendar file until further intervention.
If I remember right, there is a timeout for file lockings on WebDAV servers for cases like this.
 On other network shares using timestamps or other tricks may reduce the risk of dataloss, but how long it takes until dataloss occurs is only a question of how many times one makes "changes" in a calendar file and how many clients are active at the same time.
 Since every time one clicks on "Snooze" or "Dismiss" in any alarm popup the calendar file is changed too, writing can easily happen several times in one single hour depending on the habits of the user.
(In reply to comment #68)
> I can confirm that using Sunbird with two users on a WebDav http server still
> leads to occasional data loss with the latest version.
That would mean the patch committed at comment #53 does not help under all circumstances / with all kinds of WebDAV servers and therefore the removal from the blocking-list should be considered to be reverted...(?)
Comment 70 Lukas Ruf 2009-02-10 08:23:18 PST
(In reply to comment #69)
> (In reply to comment #67)
>  On other network shares using timestamps or other tricks may reduce the risk
> of dataloss, but how long it takes until dataloss occurs is only a question of
> how many times one makes "changes" in a calendar file and how many clients are
> active at the same time.

If any race may happen, it doesn't help at all: Irrespective of timing tricks or the like, one cannot trust the result and remains as unsure.

Therefore, I strongly prefer to know that there is a problem instead of having a solution that helps in some cases but not in the others.
Comment 71 Daniel Boelzle [:dbo] 2009-02-10 11:26:39 PST
(In reply to comment #62)
> Isn't the webdav bit already fixed since we use etags? locking is not needed if
> the correct headers are sent (if-none-match/if-match). This makes sure that the
> PUT is only done if the current etag matches the one specified in the headers.
Sorry for hooking in so late:
- Yes, I've fixed the WebDAV piece checking for 412 in the above patch.
- I don't see a benefit of locking over etag checking; we've discussed this over and over in this and other bugs...
- I agree this bug doesn't block at all since we can require servers implementing etags properly, and IMHO ftp is a rather exotic case. I even don't think this needs to be on the wanted-list.
- W.r.t comment #50: We should move further "merge/UI" patches into a separate bug. This one has definitely become too long.

(In reply to comment #68)
> I can confirm that using Sunbird with two users on a WebDav http server still
> leads to occasional data loss with the latest version. As far as I have been
> able to determine, the user gets no errors, the file is not put into read only,
> it just gets overwritten.
Could you please investigate into this further, please, and eventually file a new bug? May it be that you see the collision-refreshes I've mentioned in comment #50?
Comment 72 kujub 2009-02-11 11:54:37 PST
OK let me try to put this all together.

(In reply to comment #71)

WebDAV
------
> - I agree this bug doesn't block at all since we can require servers
> implementing etags properly
So, there may remain some (rare) problems because of broken WebDAV servers.

FTP / file://
--------------
> and IMHO ftp is a rather exotic case.
Agreed, but maybe the risks could be reduced for the ftp and file: parts if we could get in the timestamp thing. (comment #65)

Warn the user
--------------
> I even don't think this needs to be on the wanted-list.
Disagreed. Since we can't fix all aspects of this bug in a clean way, there should be at least some warning given to the user when setting up a network calender file. (comment #61)
Comment 73 Sebastian Schwieger 2009-02-25 07:22:47 PST
Created attachment 364099 [details] [diff] [review]
check timestamps v1

This patch checks the lastModifiedTime in case of usage of the file:// protocol. Philipp, what do you think? Does this improve the situation? I think that in small networks/offices the file protocol is a typical usecase.
In future I think we should automatically reload the file, discard the local modification and tell the user about the problem (without putting the calendar in read only mode). But I fear this will start some discussion, hence I kept this out of the patch.
Comment 74 Jon Jones 2009-02-25 07:46:46 PST
(In reply to comment #73)
> In future I think we should automatically reload the file, discard the local
> modification and tell the user about the problem (without putting the calendar
> in read only mode). But I fear this will start some discussion, hence I kept
> this out of the patch.

I would sort of like to know your logic behind that.  It seems undesirable to purposefully introduce a data loss situation.  Why not keep the user's changes?
Comment 75 Sebastian Schwieger 2009-02-25 15:50:08 PST
(In reply to comment #74)
> I would sort of like to know your logic behind that.  It seems undesirable to
> purposefully introduce a data loss situation.  Why not keep the user's changes?

The dataloss situation is happening already. Modification is not possible (in case of an earlier remote change using webdav protocol). The calendar is put in read-only mode and will be reloaded within the next 30 min. Remote changes overwrite local changes. 
Of course, the best solution would be a merge of the two versions, but thats likely not going to be implemented in the near future. My proposal was just to ease the normal workflow of 1. reload, 2. remove write protection and 3. redo the modification. point 1 and 2 could be easily implemented.
Comment 76 Paul K 2009-02-25 18:01:42 PST
Can you say why merging the two versions cannot be implemented since that's what happens in version 0.2?
Comment 77 Bas van den Bosch 2009-02-25 22:06:08 PST
I agree, the most ideal situation would be as following imho:
for webdav/caldav: check etag-> if it doesn't match reload calendar and check contents or dtstamp of the event, if it matches, overwrite, if it doesn't -> throw warning -> dont't put into read-only
for ftp/local: check timestamp (see also bug 373430) - if it doesn't match reload calendar and check contents or dtstamp of the event, if it matches, overwrite, if it doesn't -> throw error -> don't put into read-only

but these can be implemented in follow-up bugs. For now: just timestamp or filedate would be much better then the current situation.

@Paul K: we don't want to overwrite events which were modified by other users or overwrite the whole calendar with the version which is on your workstation, that would be real dataloss instead of just on event. Reloading and processing the whole calendar everytime before you do a "save"-action takes up too much system resources.
Comment 78 Michiel van Leeuwen (email: mvl+moz@) 2009-02-25 23:51:01 PST
(In reply to comment #76)
> Can you say why merging the two versions cannot be implemented since that's
> what happens in version 0.2?

version 0.2 did not merge the two versions. All it did was download the file from the server right before applying the change. that still leaves a small window for dataloss (when two users change the data at the same time)
Comment 79 Philipp Kewisch [:Fallen] 2009-03-03 09:23:22 PST
Comment on attachment 364099 [details] [diff] [review]
check timestamps v1

I've done some simple tests and this seems to work for me.

r=philipp
Comment 80 Philipp Kewisch [:Fallen] 2009-03-03 09:24:51 PST
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/6fa0d62c4f24>

-> FIXED
Comment 81 Stefan Sitter 2009-03-04 09:33:18 PST
Please don't forget to file follow-up bugs for the issues left open.
Comment 82 Stefan Sitter 2009-03-04 11:09:47 PST
Comment on attachment 364099 [details] [diff] [review]
check timestamps v1

This patch caused the JavaScript strict warning:

  Warning: trailing comma is not legal in ECMA-262 object initializers
  Source File: file:///E:/obj/sb/mozilla/dist/bin/calendar-js/calICSCalendar.js
  Line: 1034
Comment 83 Pete 2009-05-19 03:55:15 PDT
No need to modify the 0.9. The Fix is included in 1.0pre.
But it is still not the best solution.
The Warning and the Read-Only Mode is annoying. Refresh has to be set to one minute. Otherwise the message could show up pretty often.
But sure, the Message is better than a dataloss.
I would like to see this solution: a checkmark for the function just like in 0.2: Refresh the Calendar right bevor applying. If someone is editing the same event at the same time a hint would be okay to redo the changes.
Comment 84 Wayne Mery (:wsmwk, NI for questions) 2009-05-30 05:00:07 PDT
*** Bug 456978 has been marked as a duplicate of this bug. ***
Comment 85 Keith Page 2009-11-18 03:46:57 PST
Is there a followup bug to track the progress of a more definitive solution.

Note You need to log in before you can comment on or make changes to this bug.