Closed Bug 330573 Opened 14 years ago Closed 14 years ago

EXDATE used without TZID or UTC

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cyrus, Assigned: michael.buettner)

References

()

Details

Attachments

(1 file, 2 obsolete files)

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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: MacOS X → All
Hardware: Macintosh → All
Assignee: base → michael.buettner
*** Bug 338570 has been marked as a duplicate of this bug. ***
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #223155 - Flags: first-review?(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.
Attachment #223155 - Flags: first-review?(dmose) → first-review-
Attached patch patch v2 (obsolete) — Splinter Review
>+    [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.
Attachment #223155 - Attachment is obsolete: true
Attachment #223935 - Flags: first-review?(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 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;
Attachment #223935 - Flags: first-review?(dmose) → first-review-
Attached patch patch v3Splinter Review
> 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.
Attachment #223935 - Attachment is obsolete: true
Attachment #224554 - Flags: first-review?(dmose)
*** Bug 339462 has been marked as a duplicate of this bug. ***
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.
Attachment #224554 - Flags: first-review?(dmose) → first-review+
patch checked in.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.