Closed Bug 329570 Opened 18 years ago Closed 15 years ago

Concurrent editing ICS calendars by multiple users can lose data

Categories

(Calendar :: Provider: ICS/WebDAV, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dmosedale, Assigned: sebo.moz)

References

Details

(Keywords: dataloss)

Attachments

(2 files, 1 obsolete file)

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.
Whiteboard: [cal relnote]
*** Bug 315109 has been marked as a duplicate of this bug. ***
Flags: blocking0.3?
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
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.
Flags: blocking0.3? → blocking0.3-
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.
De-scarifying the bug's title a bit since we're relnoting this.
Summary: editing ICS & CalDAV calendars by multiple writers can lose data → concurrent editing ICS & CalDAV calendars by multiple users can lose data
*** Bug 357095 has been marked as a duplicate of this bug. ***
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!
The CalDAV/etag part of this is being dealt with in bug 362698
bug 352333 and bug 287612 talk about the read before put. Also, to a lesser
extend, bug 329570.
Keywords: relnote
Whiteboard: [cal relnote]
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. 
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)... 
Flags: blocking-calendar0.7?
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.
Flags: blocking-calendar0.7? → blocking-calendar0.7-
(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?
Flags: wanted-calendar0.8?
Flags: wanted-calendar0.8? → wanted-calendar0.8+
Flags: blocking-calendar0.7-
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.
The bug summary is somewhat out of date: the CalDAV part of this has been dealt with in bugs 362698 and 373370. 
(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?
(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. 
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.
Not going to happen for 0.8.
Flags: wanted-calendar0.8+ → wanted-calendar0.8-
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.
In my opinion this bug is highly dangerous. Can't understand why this does not block 0.8
(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.
(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?
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.
(In reply to comment #29)
Thanks for clarification. WebDAV locking should be tracked in bug 290446.
Depends on: 290446
(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). 
(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.
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.
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.
Flags: blocking-calendar0.9+
Please only nominate (?) bugs, not approve (+) as blocking, unless you're on the Calendar team.
Flags: blocking-calendar0.9+ → blocking-calendar0.9?
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
Flags: wanted-calendar0.8- → wanted-calendar0.9?
Version: Trunk → unspecified
Flags: wanted-calendar0.9?
Flags: wanted-calendar0.9+
Flags: blocking-calendar0.9?
Changing summary and component to reflect fixed CalDAV portion of the bug.
Component: Internal Components → Provider: ICS/Webdav
QA Contact: base → ics-provider
Summary: concurrent editing ICS & CalDAV calendars by multiple users can lose data → Concurrent editing ICS calendars by multiple users can lose data
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.
Flags: wanted-calendar0.9+ → wanted-calendar1.0?
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.
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...
Flags: wanted-calendar1.0? → blocking-calendar1.0+
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.
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'?
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?
(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.
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.
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???
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.
(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)
Attached patch fix - v1 (obsolete) — Splinter Review
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.
Assignee: nobody → daniel.boelzle
Attachment #341488 - Flags: review?(mvl)
- added check for listener
Attachment #341488 - Attachment is obsolete: true
Attachment #342224 - Flags: review?(mvl)
Attachment #341488 - Flags: review?(mvl)
Status: NEW → ASSIGNED
No longer depends on: 290446
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
Attachment #342224 - Flags: review?(mvl) → review+
@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?
Assignee: daniel.boelzle → nobody
Flags: blocking-calendar1.0+
Target Milestone: --- → 1.0
Attachment #342224 - Attachment description: fix - v2 → [checked in] fix - v2
Status: ASSIGNED → NEW
Keywords: helpwanted
(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.
Flags: wanted-calendar1.0?
Flags: tb-integration?
Flags: blocking-calendar1.0?
Is there any chance for this to get fixed in 1.0 / Thunderbird 3 ?
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.
Flags: wanted-calendar1.0?
Flags: wanted-calendar1.0+
Flags: blocking-calendar1.0?
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.
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.
(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. :-) )
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
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.
(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.
(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.
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.
(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 ?
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.
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.
(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...(?)
(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.
(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?
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)
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.
Attachment #364099 - Flags: review?(philipp)
(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?
(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.
Can you say why merging the two versions cannot be implemented since that's what happens in version 0.2?
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.
(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 on attachment 364099 [details] [diff] [review]
check timestamps v1

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

r=philipp
Attachment #364099 - Flags: review?(philipp) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/6fa0d62c4f24>

-> FIXED
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Please don't forget to file follow-up bugs for the issues left open.
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
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.
Severity: major → critical
Flags: tb-integration?
Assignee: nobody → sebo.moz
Is there a followup bug to track the progress of a more definitive solution.
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.