Adding to read-only remote calendars incorrectly allowed; item appears until Sunbird restart

This is possibly a dupe of bug 316941103 and/or bug 341501104.

Bug what? Too many digits.

Sorry, Philipp. :)

Perhaps bug 316941 and/or bug 341501.

FYI, the extra digits in my previous comment are caused by the "Mouseless Browsing" Thunderbird extension when I copy/paste links. I forgot to clean it up.

I don't think this is a dup of the bugs that Pete mentioned. My colleague and I have confirmed this bug by starting from no .mozilla/sunbird directory, then adding an event, accidentally putting it on the read-only http:// web-only ics calendar. The error message is:

"There has been an error reading data for calendar: basic. It has been placed in read-only mode, since changes to this calendar will likely result in data-loss. You may change this setting by choosing 'Edit Calendar'.

Error number: 0x0
Description: Publishing the calendar file failed
Status code: 404: Not Found"

After that, the event appears on calendar until Sunbird is restarted and cannot be deleted or manipulated (e.g. moved to another calendar). When Sunbird is restarted, the event is gone (i.e. restarting Sunbird is a workaround to this bug). Once error is produced for the first time, the remote calendar will not be available when adding additional events.

We only confirmed it in version 0.7 of sunbird, BTW, as shipped with Ubuntu hardy. We filed a bug there as well: https://bugs.launchpad.net/sunbird/+bug/320618

> We only confirmed it in version 0.7 of sunbird

Please retest with Sunbird 0.9 release build (http://www.mozilla.org/projects/calendar/) or a Sunbird 1.0pre nightly test build. Sunbird 0.7 was released 15 months ago many bugs have been fixed since then.

Confirmed on Sunbird 1.0pre nightly build for 1/22, following same steps as #4.

Error returned at command line: "Warning: There has been an error reading data for calendar: basic. It has been placed in read-only mode, since changes to this calendar will likely result in data-loss. You may change this setting by choosing 'Edit Calendar'. Error code: DAV_PUT_ERROR. Description: Publishing the calendar file failed
Status code: 0"

Error in GUI:
"An error occurred when writing to the calendar basic!
Error number: MODIFICATION_FAILED
Description:"

Aaron, DAV_PUT_ERROR looks like a CalDAV error, while this bug is about ics/webdav. Are you sure you chose the right calendar type when subscribing to your calendar?

Philipp: I'm pretty sure. This is what I'm doing:

File -> Subscribe to remote calendar -> On the Network -> iCalendar (ICS); Location: [Google calendar .ics URL] -> Next -> etc.

*** Bug 463667 has been marked as a duplicate of this bug. ***

Confirmed in 1.0b2pre. The event is shown in the view, an alarm fires for it and its not deletable.

I can reproduce this for local ics files also if the files are set to read-only. I believe the new events are being added to the memory cache before the the write attempt fails, then are not cleaned out of the cache on refresh because the file time is unchanged or http response 304 is returned.

Created attachment 472206
Adds force-refresh flag to calICSCalendar

Adds an optional force parameter to the refresh method of calICSCalendar, instructing the method to ignore the modification status of the calendar and force the cache to be refreshed, and added this call after a modification failure.

Comment on attachment 472206
Adds force-refresh flag to calICSCalendar

This solution is much simpler than what I imagined, but I'm not sure its the right way to go:

You're lucky that adding the argument to refresh actually works. The calICalendar interface doesn't specify any arguments, but since JS is forgiving w.r.t passing arguments and you're not crossing any xpcom boundaries here makes it work out. If in some case the call to savedthis.refresh() does pass xpcom boundaries, I believe the extra |true| argument will be stripped.

Aside from that, it seems not to be working for me:

STR:
* Create a calendar on a file://.../test.ics calendar
* Remove write access (i.e chmod -w ~/test.ics)
* Write an event on the calendar

Results:
* I get a message that writing failed, but the event is still there

>
> mObserver: null,
> locked: false,
>+ forceRefresh: false,

>- refresh: function calICSCalendar_refresh() {
>+ refresh: function calICSCalendar_refresh(aForce) {
>+ this.forceRefresh = aForce;
> this.queue.push({action: 'refresh'});
> this.processQueue();
> },
If we do use the forceRefresh() flag, then maybe we could pass aForce as a further object parameter to the {action: 'refresh'} object. This way there's no chance of re-entrancy issues in the future.

r- to clarify

(In reply to comment #13)
> You're lucky that adding the argument to refresh actually works. The
> calICalendar interface doesn't specify any arguments, but since JS is forgiving
> w.r.t passing arguments and you're not crossing any xpcom boundaries here makes
> it work out. If in some case the call to savedthis.refresh() does pass xpcom
> boundaries, I believe the extra |true| argument will be stripped.

Maybe the better way to go is to leave refresh() as it is and add a reLoad() to pass the forceRefresh parameter into doRefresh(), since we really only need to do this internally.

> STR:
> * Create a calendar on a file://.../test.ics calendar
> * Remove write access (i.e chmod -w ~/test.ics)
> * Write an event on the calendar
>
> Results:
> * I get a message that writing failed, but the event is still there

Looks like we need to clear the memory calendar in the case of a new file and zero length file also, in case any previous write operations failed.

Created attachment 481757
fix v2

Forces a reload after a modification failure, and clears the memory calendar in case the calendar was never created or isn't readable on refresh.

Comment on attachment 481757
fix v2

> calICSCalendar.prototype = {
> __proto__: cal.ProviderBase.prototype,
>
> mObserver: null,
> locked: false,
>+ forceRefresh: false,
My hopes were we could get rid of this member, see below.

>+ reLoad: function calICSCalendar_reLoad() {
>+ this.queue.push({action: 'refresh', forceRefresh: true});
>+ this.processQueue();
>+ },
I'd prefer to call this method forceRefresh(), we'll need to get rid of the member for that.

>@@ -172,38 +190,44 @@ calICSCalendar.prototype = {
> // Lock other changes to the item list.
> this.lock();
>
> try {
> streamLoader.init(this);
> channel.asyncOpen(streamLoader, this);
Currently we pass |this| as a second parameter but don't use it in the stream listener. Instead, you could probably use nsISupportsPRBool to pass a boolean value for the refresh flag. This way you can eliminate the need for forceRefresh as a member of the prototype. See http://mxr.mozilla.org/comm-central/source/mozilla/testing/mochitest/tests/SimpleTest/quit.js#75 for how to create one.

> onStreamComplete: function(loader, ctxt, status, resultLength, result)
> {
>- // No need to do anything if there was no result
>+ // Clear any existing events if there was no result
...then later in onStreamComplete, you can do |let forceRefresh = ctxt.QueryInterface(Components.interfaces.nsISupportPRBool).data;|. This way you don't need forceRefresh to be a member.

>
>- doRefresh: function calICSCalendar_doRefresh() {
>+ createMemoryCalendar: function calICSCalendar_createMemoryCalendar() {
>+ // Create a new calendar, to get rid of all the old events
>+ // Don't forget to remove the observer
>+ this.mMemoryCalendar.removeObserver(this.mObserver);
What happens if the memory calendar hasn't been initialized? I'd suggest adding if (this.mMemoryCalendar) here.
Also, does it make sense to add the new observer in this function already? I had the feeling you're adding the observer outside of the call each time createMemoryCalendar is called.

> // File not found: a new calendar. No problem.
>+ this.createMemoryCalendar();
>+ this.mMemoryCalendar.addObserver(this.mObserver);
>+ this.mObserver.onLoad(this);
> this.unlock();
Lets hope this doesn't cause dataloss in certain situations. While it totally makes sense when the file is really empty, do you think there are situations where there is no content because of an error, that would trigger this code? I'm not sure.

r- to resolve above comments, in general I'm happy about the patch though :-)

Note that nsISupportsPRBool may be deprecated (i.e doesn't work with libxul?), please check with #developers before following my advice :-)

(In reply to comment #17)
> Note that nsISupportsPRBool may be deprecated (i.e doesn't work with libxul?),
> please check with #developers before following my advice :-)

I asked in #developers, and Callek said he "thought he heard something about them going away shortly after FF4", but no one had any further info on it being deprecated, and I wasn't able to find anything anywhere else about this either. Someone suggested that jsval may replace the functionality of the support primitives, but I don't think that would work here. It seems to still be working OK for me with comm-central (and libxul is already landed there, right?)

(In reply to comment #16)

> Also, does it make sense to add the new observer in this function already? I
> had the feeling you're adding the observer outside of the call each time
> createMemoryCalendar is called.

There's a comment in onStreamComplete that says it's important to add the observer only after all the events are adopted so the views won't be notified too early.

> > // File not found: a new calendar. No problem.
> >+ this.createMemoryCalendar();
> >+ this.mMemoryCalendar.addObserver(this.mObserver);
> >+ this.mObserver.onLoad(this);
> > this.unlock();
> Lets hope this doesn't cause dataloss in certain situations. While it totally
> makes sense when the file is really empty, do you think there are situations
> where there is no content because of an error, that would trigger this code?
> I'm not sure.

You're absolutely right - this could cause dataloss with a file on a network share - if it was disconnected before a refresh, all the events would be cleared, and if it was reconnected before a subsequent write operation all the previous data would be gone.

I think we should leave this case alone, it makes sense that if we leave the old events in view when the calendar is temporarily non-readable, the unwritten event could stay also and possibly be written if the calendar becomes connected and writable again.

Created attachment 487219
fix v3

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/255e5b4859d2>
-> FIXED

Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/1547912840a1>
a=philipp

Comment on attachment 487219
fix v3

Got checked in, but never marked as r+ by Fallen. ;)

Reopening due to regression with webdav (bug 610152)

Created attachment 518903
fix for webdav

Fixes issue with WebDAV calendars causing memory calendar to be cleared on 304 response.

Comment on attachment 518903
fix for webdav

Looks good, lets do it. r=philipp

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/60833e1149eb>
Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/fce654340106>

-> FIXED

*** Bug 643390 has been marked as a duplicate of this bug. ***

*** Bug 609653 has been marked as a duplicate of this bug. ***