330573 - EXDATE used without TZID or UTC

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Cyrus Daboo]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en) AppleWebKit/417.9 (KHTML, like Gecko) Safari/417.9.2
Build Identifier: 

When an instance of a recurring event is deleted an EXDATE property is added to the recurring component. However, the EXDATE property value is 'floating' as there is no TZID parameter and its not in UTC. This is wrong because the original DTSTART etc do have a TZID. The EXDATE MUST use a TZID or be in UTC to properly match the recurrence set.

Reproducible: Always

Steps to Reproduce:
1. Creating a recurring event with several instances with a default timezone setup.
2. Select one instance (not the first) in the UI and hit the delete key to delete that instance only.
3. Examine the resuling iCalendar data sent to a CalDAV server. The EXDATE will be wrong.

NB This was done using a CalDAV calendar.

Actual Results:  
EXDATE was missing TZID and not in UTC.

Expected Results:  
EXDATE should have had a TZID or been in UTC.

Comment 1

[Michael Büttner (no reviews TFN)]

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

Comment 2

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

the implementation of calIRecurrenceDate ignores timezones during the conversion from calIDateTime to the appropriate icalproperty and visa versa. this patch adds the necessary TZID parameter to the EXDATE property.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

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

In general, this looks good, but I think a few changes would be helpful.

>diff -a -x CVS -U 8 -pN -rN mozilla_ref/calendar/base/public/calIICSService.idl mozilla/calendar/base/public/calIICSService.idl
>--- mozilla_ref/calendar/base/public/calIICSService.idl	2006-02-24 21:42:47.000000000 +0100
>+++ mozilla/calendar/base/public/calIICSService.idl	2006-05-24 10:17:50.312500000 +0200
>@@ -201,16 +201,17 @@ interface calIIcalProperty : nsISupports
>      * This does not work with X-PARAMETERS, due to limitations in libical.
>      * You have to use clearXParameters() and then rebuild the ones you wanted
>      * to preserve.  Sorry about that.
>      */
>     void removeParameter(in AUTF8String paramname);
>     void clearXParameters();
> 
>     [noscript,notxpcom] icalpropertyptr getIcalProperty();
>+    [noscript,notxpcom] icalcomponentptr getIcalComponent();

How about calling this getParentIcalComponent?  That seems like it would be a more accurate description of what's being returned.  Also, please add doxygen comments describing this new method, including the ownership model of the thing being returned, since it's |notxpcom|.

>@@ -220,16 +224,33 @@ calRecurrenceDate::GetIcalProperty(calII
>     icalproperty_set_value(dateprop, v);
> 
>     calIIcalProperty *icp = new calIcalProperty(dateprop, nsnull);
>     if (!icp) {
>         icalproperty_free(dateprop);
>         return NS_ERROR_FAILURE;
>     }

I'd suggest adding a comment that the only reason this is necessary is because the icalproperty_set_value() has the somewhat non-intuitive behavior of not handling the TZID parameter automagically.  A similar comment would be good for the SetIcalProperty changes too.

>+    nsCAutoString tzid;
>+    if (NS_SUCCEEDED(mDate->GetTimezone(tzid))) {
>+        if (!tzid.IsEmpty() && !tzid.EqualsLiteral("UTC") &&
>+            !tzid.EqualsLiteral("floating")) {
>+            nsCOMPtr<calIICSService> ics = do_GetService(kCalICSService);
>+            nsCOMPtr<calIIcalComponent> tz;
>+            ics->GetTimezone(tzid, getter_AddRefs(tz));
>+            if(tz) {
>+              icalproperty_set_parameter_from_string(dateprop, "TZID", nsPromiseFlatCString(tzid).get());
>+            }
>+            else {
>+              // Uh, we didn't find this timezone.  This should somehow be bad.
>+              NS_WARNING("Timezone was not found in database!");
>+            }

In the case that there is a TZID, but it's not in the database this will silently (in non-debug builds) lose data.  In that case, I think we probably want to return calIErrors::INVALID_TIMEZONE (both here and in SetIcalProperty).  This suggests changing calICSService to return calIErrors::INVALID_TIMEZONE so that it's possible for this code to tell the "no such timezone" case apart from other errors that GetTimezone might return.  Then by using the two-argument form of do_GetService and checking the return values for both those calls, correct and specific error information can be propagated to the caller.

Comment 4

[Michael Büttner (no reviews TFN)]

Attachment: patch v2

>+    [noscript,notxpcom] icalcomponentptr getIcalComponent();
> How about calling this getParentIcalComponent?  That seems like it would be a
> more accurate description of what's being returned.  Also, please add doxygen
> comments describing this new method, including the ownership model of the
> thing being returned, since it's |notxpcom|.
since the icalComponent is always the parent of a icalProperty, i can't see why why should name it getParentIcalComponent. i feel that this is redundant and confusing, if you don't mind i would stick to getIcalComponent() since it does what the name says, return the component this property belongs to. i added the comments, of course.

> In the case that there is a TZID, but it's not in the database this will
> silently (in non-debug builds) lose data.  In that case, I think we probably
> want to return calIErrors::INVALID_TIMEZONE (both here and in 
> SetIcalProperty).
you're right, i should have done this in the first place.

> This suggests changing calICSService to return calIErrors::INVALID_TIMEZONE
> so that it's possible for this code to tell the "no such timezone" case apart > from other errors that GetTimezone might return.
> Then by using the two-argument form of do_GetService and checking the
> return values for both those calls, correct and specific error information
> can be propagated to the caller.
i don't really understand why you're talking about calICSService here, since the modification affects the 'icalProperty' getter/setter of calIRecurrenceItem. anyway, in case we're talking about just changing the returned error code, everything is alright.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #4)
> i feel that this is redundant and
> confusing, if you don't mind i would stick to getIcalComponent() since it does
> what the name says, return the component this property belongs to.

OK.

> i don't really understand why you're talking about calICSService here, since
> the modification affects the 'icalProperty' getter/setter of
> calIRecurrenceItem. anyway, in case we're talking about just changing the
> returned error code, everything is alright.

The problem is that there are multiple possible error conditions here that are being incorrectly folded into calIErrors::INVALID_TIMEZONE.  Since some of these conditions imply different actions for the end user (e.g. try again, help us track down a bug, fix the ICS file itself), not propagating these out to the callers makes support & debugging harder.  Some of these issues are in the code in this patch, but one of them is in calICSCalendar::GetTimezone() itself: there are a bunch of different code paths in that method that can return failure, and only one really indicate an invalid timezone.  So the (minimal) fix that method needs is that if get_timezone_data_struct_for_tzid() fails, it should return calIErrors::INVALID_TIMEZONE instead of NS_ERROR_FAILURE.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 223935 [details] [diff] [review]
patch v2

>+    nsCAutoString tzid;
>+    if (NS_SUCCEEDED(mDate->GetTimezone(tzid))) {

If the above call were to fail, it could result in silent dataloss.  While CAL_STRINGTYPE_ATTR_GETTER, which is used to implement calDateTime::GetTimezone() won't currently ever fail, who knows if that invariant will remain true into the future.

>+        if (!tzid.IsEmpty() && !tzid.EqualsLiteral("UTC") &&
>+            !tzid.EqualsLiteral("floating")) {
>+            nsCOMPtr<calIICSService> ics = do_GetService(kCalICSService);

Failure here wouldn't be detected, and the next line would then crash.  Please use the two-argument form of do_GetService and bail out if there is an error.

>+            nsCOMPtr<calIIcalComponent> tz;
>+            ics->GetTimezone(tzid, getter_AddRefs(tz));
>+            if(tz) {

In concert with the requested fix to GetTimezone, this statement needs to check the return value here and do different things based on whether it is calIErrors::INVALID_TIMEZONE.

>+        // We have to walk up to our parent VCALENDAR and try to find this tzid
>+      icalcomponent *vcalendar = aProp->GetIcalComponent();
>+      while (vcalendar && icalcomponent_isa(vcalendar) != ICAL_VCALENDAR_COMPONENT)
>+        vcalendar = icalcomponent_get_parent(vcalendar);
>+      if (!vcalendar) {
>+        NS_WARNING("VCALENDAR not found while looking for VTIMEZONE!");
>+        return calIErrors::INVALID_TIMEZONE;

INVALID_TIMEZONE isn't really the problem here.  Probably the best thing to do is to map icalerrno into the calIErrors namespace:

  return calIErrors::ICS_ERROR_BASE + icalerrno;

Comment 7

[Michael Büttner (no reviews TFN)]

Attachment: patch v3

> In concert with the requested fix to GetTimezone, this statement needs
> to check the return value here and do different things based on whether it is
> calIErrors::INVALID_TIMEZONE.
in case ics->GetTimezone returns an error, GetIcalProperty() terminates immediately and returns that same error code. i can't see a reason for checking the error code and do different things here. i would like to point to bug #254893 here, which would change GetTimezone() to behave more consistently.

Comment 8

[Michael Büttner (no reviews TFN)]

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

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

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

OK, looks good.  One tweak requested:

>+    if (NS_FAILED(mDate->GetTimezone(tzid)))
>+      return calIErrors::INVALID_TIMEZONE;

We don't know that the timezone is necessarily invalid here; it could have just been (e.g.) an out-of-memory error.  Preferred would be:

rv = mDate->GetTimezone(tid);
if (NS_FAILED(rv)) {
    return rv;
}

r=dmose with that change.  Thanks for the patch.

Comment 10

[Michael Büttner (no reviews TFN)]

patch checked in.