333717 - Timezone information lost, causing wrong results from recurrence rules

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Cael]

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

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

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 ***

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 3

[Cael]

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.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

Reopening.
Chaning summary to reflect the actual problem.
blocking sunbird0.3a2. Loosing timezone information is not good.

Comment 6

[Andrew N Dowden]

Attachment: 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)

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: don't convert to UTC

All times were converted to utc on reading the ics file. The patch removes that conversion.

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: really don't convert

Now without the calIPeriod parts

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

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

Looks good; r=dmose.

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

patch checked in

Comment 11

[Daniel Boelzle [:dbo]]

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.

Comment 12

[Michiel van Leeuwen (email: mvl+moz@)]

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.)

Comment 13

[Daniel Boelzle [:dbo]]

(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

Comment 14

[Daniel Boelzle [:dbo]]

Attachment: sample file

debug sample

Comment 15

[Daniel Boelzle [:dbo]]

(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?

Comment 16

[Daniel Boelzle [:dbo]]

Attachment: better sample

Comment 17

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 18

[Daniel Boelzle [:dbo]]

> 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.

Comment 19

[Daniel Boelzle [:dbo]]

(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

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

(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.

Comment 21

[Daniel Boelzle [:dbo]]

(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.

Comment 22

[Michiel van Leeuwen (email: mvl+moz@)]

> 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)

Comment 23

[Daniel Boelzle [:dbo]]

> 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).

Comment 24

[Michiel van Leeuwen (email: mvl+moz@)]

Removing this from the sb0.3a2 blocker list, now the internal dataloss has been fixed.

Comment 25

[Andrew N Dowden]

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).

Comment 26

[Michiel van Leeuwen (email: mvl+moz@)]

(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.

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

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?

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 29

[cmtalbert]

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.