Timezone information lost, causing wrong results from recurrence rules

RESOLVED FIXED

Status

Calendar
General
--
major
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Cael, Unassigned)

Tracking

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: http://ftp.mozilla.org/pub/mozilla.org/calendar/sunbird/nightly/2006-04-10-08-trunk/sunbird-0.3a1+.en-US.win32.zip

Using the April 10th 0.3a1+ build, I'm experiencing the following issue:

A recurring event created yesterday for the first Monday of every month, beginning 2006-05-01 has drifted one day earlier and now appears on the Sunday previous to the first Monday of every month.

In the calendar view, all instances of this event appear to be visible except for the first-scheduled event, 2006-05-01.  That one is nowhere to be seen.

In the Events Listing pane, the correct date for the first event appears.  Right-clicking and editing all occurrences, then selecting 'Set pattern' prompts me with a pair of radio buttons:

- 2nd day of the month
- First Tuesday of the month

This doesn't make much sense, since the event isn't scheduled for the 2nd of the month, it isn't a Tuesday, and the recurring events are incorrectly appearing on Sundays.

Here's the offending entry in the .ics:

BEGIN:VEVENT
CREATED:20060411T185120Z
LAST-MODIFIED:20060411T185211Z
DTSTAMP:20060411T185211Z
UID:uuid1144781531187
SUMMARY:Suppressed
PRIORITY:0
CLASS:PUBLIC
RRULE:FREQ=MONTHLY;INTERVAL=1;BYDAY=1MO
DTSTART:20060502T000000Z
DTEND:20060502T023000Z
CATEGORIES:Suppressed
LOCATION:Suppressed
END:VEVENT

Note the DTSTART/DTEND.


Reproducible: Didn't try
This looks pretty much like 333664. That one is about weekly recurrence though. If this bug still shows when the other is fixed, feel free to reopen this one.

*** This bug has been marked as a duplicate of 333664 ***
Status: UNCONFIRMED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → DUPLICATE
Actually, how did you create the event? The problem seems to be that the start time is defined in UTC, and monday in UTC is not the same as monday in your timezone.
I'm not sure what the spec says (if it even says something about this), but we shouldn't create those events. recurrence and utc is problematic.
(Reporter)

Comment 3

12 years ago
Interesting -- It was created using the specified nightly build of sunbird using the interface in the usual manner -- I didn't do anything unusual to generate the event.

The timezone for the machine is set to GMT-06:00 with the 'Automatically adjust clock for daylight savings changes' checkbox marked.  The machine wasn't on during the changeover to daylight savings time so the clock was adjusted an hour manually.
After some testing, i found a way to turn an event into an UTC event.
- create an event in an ics calendar (it's in the right timezone)
- close sunbird (still ok)
- start sunbird (still ok)
- change the event
After the last step, the event is in UTC. (For the last step, changing a different event might work too)
Because the event is now in UTC, the recurrance info is wrong.
Reopening.
Chaning summary to reflect the actual problem.
blocking sunbird0.3a2. Loosing timezone information is not good.
Blocks: 330188
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
Summary: Recurring events appear on wrong days (1st day of each month rule) → Timezone information lost, causing wrong results from recurrence rules

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 6

12 years ago
Created attachment 218392 [details]
testing of timezone settings for load, save, create (including cycling)

This is a slightly different example showing loss of local time (to UTC), relating to edit before save.

It is very close to what you tested, and may be same cause and effect.
  (see line 73 of attached)
Created attachment 218878 [details] [diff] [review]
don't convert to UTC

All times were converted to utc on reading the ics file. The patch removes that conversion.
Attachment #218878 - Flags: first-review?(dmose)
Created attachment 218883 [details] [diff] [review]
really don't convert

Now without the calIPeriod parts
Attachment #218878 - Attachment is obsolete: true
Attachment #218883 - Flags: first-review?(dmose)
Attachment #218878 - Flags: first-review?(dmose)

Comment 9

12 years ago
Comment on attachment 218883 [details] [diff] [review]
really don't convert

Looks good; r=dmose.
Attachment #218883 - Flags: first-review?(dmose) → first-review+

Updated

12 years ago
OS: Windows 2000 → All
Hardware: PC → All
patch checked in
Status: NEW → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
IMO no fix, because itt.is_utc is not reset to false. I am experiencing itt.is_utc==1, thus the resulting datetime's timezone is UTC!
Even when resetting is_utc = 0, IMO this is a bad fix: It nails us to ics files created by Lightning/Sunbird, relying on that all timezone references can be resolved to the internal Olson ones (/mozilla.org/20050126...). Otherwise, datetimes will fail (see calDateTime::GetIcalTZ).
"Foreign" calendars e.g. a google calendar ical subscription are cut now, because they come with unknown TZIDs.
A reliable approach, adding new timezone definitions at runtime is attached to bug 254893.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
We have code to try to read the timezones. So in theory, it should work. Can you upload a testcase?
Before writing this patch, i tried the patch in bug 254893, but it didn't solve to problem. Times were still not in the right timezone (they were utc or floating, i'm not sure anymore.)
(In reply to comment #11)
> IMO no fix, because itt.is_utc is not reset to false. I am experiencing
> itt.is_utc==1, thus the resulting datetime's timezone is UTC!

More specifically when reading DATE strings, e.g. DTSTART;TZID=/some/tz:20060328
Created attachment 219018 [details]
sample file

debug sample
(In reply to comment #12)
> We have code to try to read the timezones. So in theory, it should work. Can
> you upload a testcase?

Please debug the attached sample file which constructs a recurring event with timezone for DTSTART from end of march on in CET.
When you switch your Lightning TZ to e.g. African Ceuta UTC, there is no step at end of march (->daylight saving).
Having a look at the series calculation in calRecurrenceRule::GetOccurrences(), it shows up that due to aStartTime's missing tz (not found, asserted in calDateTime::ToIcalTime() when calling aStartTime->ToIcalTime(&dtstart)), the resulting dtstart gets "floating". This seems ok if you have set CET in your Lightning's options... but is wrong.

> Before writing this patch, i tried the patch in bug 254893, but it didn't solve
> to problem. Times were still not in the right timezone (they were utc or
> floating, i'm not sure anymore.)

Maybe I am missing some important test cases, but 254893.patch has solved my scenarios. Why didn't you leave me a note?

Updated

12 years ago
Attachment #219018 - Attachment is obsolete: true
Created attachment 219051 [details]
better sample
Am I correct that the only problem left is with timezones from other sources? In that case, we could remove this bug from the blocker list, because for now, we focus on not having internal dataloss. Correctly reading files from other sources doesn't block 0.3a2.

(In reply to comment #15)
> Maybe I am missing some important test cases, but 254893.patch has solved my
> scenarios. Why didn't you leave me a note?

Because I assumed your patch was trying to fix a different problem, and that your patch and mine combined would fix everything.
> When you switch your Lightning TZ to e.g. African Ceuta UTC, there is no step
Bad idea: Africa Ceuta has a similar daylight saving, better take Africa/Bissau.
(In reply to comment #17)
> Am I correct that the only problem left is with timezones from other sources?
> In that case, we could remove this bug from the blocker list, because for now,
> we focus on not having internal dataloss. Correctly reading files from other
> sources doesn't block 0.3a2.

Yes, bug can be taken from blocker list then. Though #13 needs to be fixed:

> > IMO no fix, because itt.is_utc is not reset to false. I am experiencing
> > itt.is_utc==1, thus the resulting datetime's timezone is UTC!
> 
> More specifically when reading DATE strings, e.g.
> DTSTART;TZID=/some/tz:20060328
(In reply to comment #11)
> 
> > > IMO no fix, because itt.is_utc is not reset to false. I am experiencing
> > > itt.is_utc==1, thus the resulting datetime's timezone is UTC!
> > 
> > More specifically when reading DATE strings, e.g.
> > DTSTART;TZID=/some/tz:20060328
> 

If you end up inside the |if (tzid)| block while is_utc==1, then the object you've got is corrupt: you can't both have a TZID and be in UTC.
(In reply to comment #20)
> (In reply to comment #11)
> > 
> > > > IMO no fix, because itt.is_utc is not reset to false. I am experiencing
> > > > itt.is_utc==1, thus the resulting datetime's timezone is UTC!
> > > 
> > > More specifically when reading DATE strings, e.g.
> > > DTSTART;TZID=/some/tz:20060328
> > 
> 
> If you end up inside the |if (tzid)| block while is_utc==1, then the object
> you've got is corrupt: you can't both have a TZID and be in UTC.

No, the ics data is correct, but libical marks DATE properties (-> whole-day events) as is_utc=1 (hours, minutes, seconds set to zero), and of course is_date=1. One can argue this is a bug in libical, as it seems there is no special handling for a DATE property's TZID attribute, but I would simply get rid of this and patch is_utc=0. Timezone handling sucks in libical as already commented in calICSService.cpp, the very same function we are talking about.
> No, the ics data is correct, but libical marks DATE properties (-> whole-day
> events) as is_utc=1 (hours, minutes, seconds set to zero), and of course
> is_date=1.

The problem here is that a date doesn't really have a timezone. What does it mean to change the timezone of a date? I think that all all-day items should be without a timezone (floating).
(If I would be in some different timezone on my birthday, I won't think at 22:00 that my birthday is over. It's just the day, in whateven timezone happen to be)
> The problem here is that a date doesn't really have a timezone. What does it
> mean to change the timezone of a date? I think that all all-day items should be
> without a timezone (floating).

Yes, of course.

(In reply to comment #20)
> If you end up inside the |if (tzid)| block while is_utc==1, then the object
> you've got is corrupt: you can't both have a TZID and be in UTC.

Having a closer look at rfc2445, 4.2.19, it turns out that my ics test data is indeed corrupt (TZID only allowed on DATE-TIME or TIME), sorry!  That libical sets is_utc=1 is unfortunate, though, floating would have been better...

and thus IMO we should make calDateTime more robust, encorcing if mIsDate ==> tz = "floating" (as suggested by Michiel).
Removing this from the sb0.3a2 blocker list, now the internal dataloss has been fixed.
No longer blocks: 330188

Comment 25

12 years ago
per Comment 22
> The problem here is that a date doesn't really have a timezone. What does it
> mean to change the timezone of a date? I think that all all-day items should
> be without a timezone (floating).
If we go with the the iCal definition, that is almost correct.  For most non-blocking events, Sunbirds definition of an all-day event closely replicates what rc2445 intended.  The exact definition is a little different:

from rc2445 - 4.6.1 Event Component
   For cases where a "VEVENT" calendar component
   specifies a "DTSTART" property with a DATE-TIME data type but no
   "DTEND" property, the event ends on the same calendar date and time
   of day specified by the "DTSTART" property.
   (ie. no time at all, just a marker)

This is the 'feature' that Sunbird needs to provide.  Your intent, I think, is close enough to this (with floating).
(In reply to comment #25)
> If we go with the the iCal definition, that is almost correct.
I was only talking about what i can imagine, not about the spec. And it just happens that the spec agrees with me.

> from rc2445 - 4.6.1 Event Component
I realy fail to see what that part of the spec has to do with timezones for date-only items. First, the comment is about date-time, and second, it doesn't talk about timezones.
If you want the feature that part talks about, there is bug 333362 about zero-length events.
OK, as far as I can tell, this bug itself can be marked fixed.  

I'd vote for filing another bug about what to do about TZID and DATE (note that 4.2.19 doesn't actually go so far as to say that TZID is disallowed on DATEs).  This might be something we'd do well to bring up on ietf-calsify (if it hasn't been addressed in the drafts already) before making a decision on.

I believe part of bug 254893 is still valid, in particular the "how do we handle non mozilla-native timezones" part.  In other words, I think it's morphed into another incarnation of bug 314339.  Since it already has a patch, I'd suggest renaming bug 254893 appropriately, and marking bug 314339 as a DUP.

Sound reasonable?
*** Bug 324804 has been marked as a duplicate of this bug. ***
Whiteboard: [qa discussion needed]

Comment 29

11 years ago
Setting this back to fixed, per Dan's comment.  If the issue still occurs please file some reproducible steps when it is reopened.  Also, I believe that the current 2445 biz-06 version of the spec does address the issue of DATE times w.r.t. tzid's.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [qa discussion needed]
You need to log in before you can comment on or make changes to this bug.