385916 - exceptions occuring when accessing a calendar prevent any other calendar from appearing

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Wolfgang Sourdeau]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.8.1.4) Gecko/20070508 Iceweasel/2.0.0.4 (Debian-2.0.0.4-1) XPCOMViewer/0.9.5
Build Identifier: 0.5rc3

When you have access to many CalDAV calendars and you don't have access rights to one of them or if it's http address is not correct an exception will be generated. So far so good. The problem is that is seems to prevent any other calendar from being accessed. This forces the user to disable the calendar in question and restart Thunderbird to avoid this problem...


Reproducible: Always

Steps to Reproduce:
1. create a CalDAV calendar entry with a valid URL
2. create a CalDAV calendar entry with an invalid URL or without access rights
3. browse the days and weeks....


Expected Results:  
What is expected is that the other calendars would still be accessed and their data displayed correctly. Even better... an error dialog could be displayed to let the user know that something bad occured.

Comment 1

[Bruno Browning]

Attachment: un-wedge calendars

* prevent a wedged (not DAV, no net, offline server, ...) CalDAV calendar from wedging other calendars
* check on each call to getItems() to see if the calendar has become available, and if so reset mReadOnly and mDisabled to allow access (we need to gracefully handle the network-went-away case as well, but that's not this bug)
* change the dialog to say that the calendar has been "disabled" instead of "set read-only", since in these error cases we aren't going to be able to read anything.

Contains user-visible strings; not sure if this requires UI-review, and if so from whom.

At some point we need to rethink this "read-only" issue. We're treating is as an error, which is not necessarily correct, and setting a calendar read-only isn't always a sensible response to an error. For CalDAV calendars, for instance, it would probably make sense for the the readonly checkbox on the calendar-properties dialog to read "Disabled" - and not be user-settable.

Comment 2

[Bruno Browning]

Attachment: minor changes

* Removed the see-if-the-network-came-back bits. We need to do that, but it's properly in a separate bug
* removed redundant bits in calCalendarManager.js

I looked at moving the CalDAV-specific error reporting entirely into the CalDAV provider, as suggested on IRC, but it seems to me that that causes more problems than it solves. I think a better solution - especially given that we need to rethink our whole approach to read-only and errored calendars - would be to change the calCalendarManager's announceError to take three args (aErrNo, aErrMsg, aMessage) so that providers can specify what they want. And that, again, is a separate bug.

Tested Sb/Ltn branch.

Comment 3

[Daniel Boelzle [:dbo]]

(In reply to comment #2)
> * Removed the see-if-the-network-came-back bits. We need to do that, but it's
> properly in a separate bug
Why did you remove that code? It this means that once the calendar is disabled, it won't have a chance to come back until process restart (or reregistering it).
In general IMO it's necessary to revise our error handling. IMO we have currently one major problem when signaling errors:
Errors are notified "globally" on calIObserver while UI should (and could) use calIOperationListener (resp. command specific listeners). Using a command specific listener, UI/frontend code may decide:
a) the operation was initiated by a user action
   => should lead to error box
b) the operation was initiated by e.g. auto update or alarm update
   => error might be suppressed
IMO providers should work optimistically, meaning there is no need to manage offline/online/disabled state.

WCAP side-note: due to the fact that error notification currently runs "context-less", I am hacking around bursts of identical errors, e.g. if multiple operation queue up at startup, waiting for the user to log in. If login fails, all queued up operation logically need to fire an onError. I am simply ignoring to fire a series of identical error within the same second.

> I looked at moving the CalDAV-specific error reporting entirely into the CalDAV
> provider, as suggested on IRC, but it seems to me that that causes more
> problems than it solves. I think a better solution - especially given that we
why?

> need to rethink our whole approach to read-only and errored calendars - would
> be to change the calCalendarManager's announceError to take three args (aErrNo,
> aErrMsg, aMessage) so that providers can specify what they want. And that,
> again, is a separate bug.
I think this complicates calIObserver, tieing it to a specific UI. What if we decide to rework the error box to look different? I think a single string (which may contain LF) should be enough to express an error. Moreover IMO this is an infrequent error, so I'd propose to simply notify "There has been an error reading data for calendar: %1$S. It has been disabled until it is safe to use it." from the provider.

Comment 4

[Daniel Boelzle [:dbo]]

side-note: one outcome of the above is bug 392561.

Comment 5

[Bruno Browning]

Attachment: more changed (and reversions)

I think mostly what we have here are bug-scoping issues.

> Why did you remove that code? It this means that once the calendar is disabled,
> it won't have a chance to come back until process restart (or reregistering
> it).

I removed it because it seemed to me that I was violating the "one problem per bug" rule by fixing an "intermittent network" problem in a "bad calendar wedges others" bug. But the problem needs to be fixed, and I'm not all that particular as to where. This patch restores the ability to detect the network connection being restored; will file a "notice that network disappeared" as a followup.

> In general IMO it's necessary to revise our error handling. 

Agree completely. Just didn't intend to do that in this bug. ;-)

> IMO providers should work optimistically, meaning there is no need to manage
> offline/online/disabled state.

I don't think I follow you here. ISTM that tracking online state and warning the user when offline is useful, even if only by allowing the user to avoid wasting efforts creating items/changes which can't possibly be saved. We'll be able to handle this better when we have a real offline mode, of course, but in the meantime I think tracking network state in the provider is useful.

This version of the patch causes the provider to pop up an alert directly rather  than routing through the calCalendarManager's errorAnnouncer.

Comment 6

[Daniel Boelzle [:dbo]]

(In reply to comment #5)
> > IMO providers should work optimistically, meaning there is no need to manage
> > offline/online/disabled state.
> 
> I don't think I follow you here. ISTM that tracking online state and warning
> the user when offline is useful, even if only by allowing the user to avoid
> wasting efforts creating items/changes which can't possibly be saved. We'll be
> able to handle this better when we have a real offline mode, of course, but in
> the meantime I think tracking network state in the provider is useful.
I agree it is usefull, but who's in charge of doing so. IMO the providers at the end of the command chain are very bad in that. I'd prefer that the front end code actually has appropriate fallbacks in case a provider operations fails. After all, also my experience with WCAP had been bad with probing the network and the like. It ended up that I changed the WCAP code to always work optimistically.

Comment 7

[Daniel Boelzle [:dbo]]

Comment on attachment 278361 [details] [diff] [review]
more changed (and reversions)

>+        // var promptMessage = calGetString("calendar", "disabledMode");
>+        var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                            .getService(Components.interfaces
>+                            .nsIStringBundleService);
>+        var props = sbs.createBundle("chrome://calendar/locale/calendar.properties");
>+        promptMessage  = props.formatStringFromName("disabledMode",
>+                                                    [this.name], 1);
maybe I miss something, but why not
         var promptMessage = calGetString("calendar", "disabledMode", [this.name]);

r=dbo

Comment 8

[Bruno Browning]

removing check-needed because this needs one more tweak before checkin

Comment 9

[Daniel Boelzle [:dbo]]

Bruno, you are adding a string to calendar.properties. Please mind that we have 0.7 string freeze by monday.

Comment 10

[Bruno Browning]

Attachment: display error message

Using an alert as in the previous version of this patch prevents the display of the actual error (DAV_NOT_CALDAV etc). This is actually not all that infrequent an error: a missing network connection is rare, but a bad user-entered URI, an attempt to use the CalDAV provider against a WebDAV server, or an attempt to use the user's non-calendar home collection on the CalDAV server as a calendar all lead to this same alert. Having the errors more visible has been useful for troubleshooting these problem on the maillist, on IRC, in bugzilla and IIRC in the Mozillazine forum, and I'd like to keep that feature. So this version has the CalDAV provider pop up not an alert but a dialog identical to the one produced by the calCalendarManager's errorAnnouncer. I think this should be non-controversial, but it is a UI change so I'm re-requesting review on that basis.

Comment 11

[Daniel Boelzle [:dbo]]

(In reply to comment #10)
> an error: a missing network connection is rare, but a bad user-entered URI, an
> attempt to use the CalDAV provider against a WebDAV server, or an attempt to
> use the user's non-calendar home collection on the CalDAV server as a calendar
> all lead to this same alert. Having the errors more visible has been useful for
> troubleshooting these problem on the maillist, on IRC, in bugzilla and IIRC in
> the Mozillazine forum, and I'd like to keep that feature. So this version has
Regarding the future I'd like the providers to support probing, so wrong urls are more likely prevented up front.

> the CalDAV provider pop up not an alert but a dialog identical to the one
> produced by the calCalendarManager's errorAnnouncer. I think this should be
> non-controversial, but it is a UI change so I'm re-requesting review on that
> basis.
As mentioned in previous comments, I don't like the providers being in charge of caring about a proper UI to present errors.

Comment 12

[Daniel Boelzle [:dbo]]

Comment on attachment 279031 [details] [diff] [review]
display error message

However, since we don't have a proper short term solution for 0.7, we should go ahead with this fix. My hope is that we sort this out soon (bug 392561 be part of that).

r=dbo; thanks for fixing this, Bruno.

Comment 13

[Daniel Boelzle [:dbo]]

BTW: I've fixed some indentation and simplified reportDavError() to use calGetString().

Checked in on HEAD and MOZILLA_1_8_BRANCH.