After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 593019 - "Etc/UTC" timezone + e_cal_get_timezone: g_assert() aborts program
"Etc/UTC" timezone + e_cal_get_timezone: g_assert() aborts program
Status: RESOLVED FIXED
Product: evolution-data-server
Classification: Platform
Component: Calendar
2.28.x (obsolete)
Other Linux
: Normal normal
: ---
Assigned To: Patrick Ohly
Evolution QA team
Depends on:
Blocks:
 
 
Reported: 2009-08-25 14:25 UTC by Patrick Ohly
Modified: 2013-09-14 16:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
triggers syszone assert (1.02 KB, text/calendar)
2009-08-25 14:25 UTC, Patrick Ohly
  Details
fixes the handing of "UTC" TZIDs (3.39 KB, patch)
2009-08-25 14:28 UTC, Patrick Ohly
reviewed Details | Review
fixes the handing of "UTC" TZIDs, now with C-style comments (3.40 KB, patch)
2009-08-25 14:37 UTC, Patrick Ohly
none Details | Review
avoid problematic mapping to "UTC", remove asserts (3.95 KB, patch)
2009-09-17 10:10 UTC, Patrick Ohly
committed Details | Review

Description Patrick Ohly 2009-08-25 14:25:58 UTC
Created attachment 141648 [details]
triggers syszone assert

A SyncEvolution user reported that SyncEvolution aborted when applied to his
calendar:

libecal:ERROR:e-cal.c:4761:e_cal_get_timezone: assertion failed: (syszone)

It turned out that he had a VEVENT with a TZID=Etc/UTC in his calendar, which
wasn't handled properly by e_cal_get_timezone(). I have a patch which explains
this in more detail.

To reproduce the problem, try importing the attached calendar.ics into
Evolution.
Comment 1 Patrick Ohly 2009-08-25 14:28:13 UTC
Created attachment 141649 [details] [review]
fixes the handing of "UTC" TZIDs
Comment 2 Matthew Barnes 2009-08-25 14:37:23 UTC
As mentioned on IRC...

Patch seems fine to me, but defering to a calendar maintainer for approval.
Please use /* ... */ style comments instead of //.
Comment 3 Patrick Ohly 2009-08-25 14:37:50 UTC
Created attachment 141651 [details] [review]
fixes the handing of "UTC" TZIDs, now with C-style comments
Comment 4 Patrick Ohly 2009-08-25 14:40:25 UTC
Chenthill, can you check the patch?
Comment 5 Milan Crha 2009-08-26 12:54:05 UTC
Hmm, I'm not sure of the RFC 2445, as the [1] says the TZID must not be used when times are in UTC. They probably mean the times which ends with a "Z" only. This is, sort of, on the edge, from my point of view.

[1] http://tools.ietf.org/html/rfc2445#section-4.2.19
Comment 6 Patrick Ohly 2009-08-26 16:01:17 UTC
From a standards point of view, DTSTART;TZID=Etc/UTC:20090826T160000 is a normal time stamp relative to a custom VTIMEZONE. There's nothing wrong with it. From a practical point of view, UTC is "meant", so DTSTART:20090826T160000Z would be more natural.

DTSTART;TZID=Etc/UTC:20090826T160000Z would be wrong, which is what section-4.2.19 is about.
Comment 7 Milan Crha 2009-08-26 16:14:04 UTC
(In reply to comment #6)
> DTSTART;TZID=Etc/UTC:20090826T160000Z would be wrong, which is what
> section-4.2.19 is about.

Yup, that's the clear thing there. What I'm not sure about is whether
> DTSTART;TZID=UTC:20090826T160000
is also good or bad thing to do, as it's an equivalent of
> DTSTART:20090826T160000Z
Comment 8 Patrick Ohly 2009-08-27 08:07:22 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > DTSTART;TZID=Etc/UTC:20090826T160000Z would be wrong, which is what
> > section-4.2.19 is about.
> 
> Yup, that's the clear thing there. What I'm not sure about is whether
> > DTSTART;TZID=UTC:20090826T160000
> is also good or bad thing to do, as it's an equivalent of
> > DTSTART:20090826T160000Z

Getting rid of the TZID would be nice. But it would also make the patch more complicated - is that desirable?
Comment 9 Patrick Ohly 2009-09-04 08:26:39 UTC
> Getting rid of the TZID would be nice. But it would also make the patch more
> complicated - is that desirable?

I've looked into this some more. Getting rid of a TZID=*/UTC isn't possible in e_cal_get_timezone(), the function where the UTC "timezone" is detected. The function simply hasn't access to the VEVENT or VJOURNAL data at all.

I also considered simply returning the original VTIMEZONE instead of a builtin, pseudo one. The drawback is that there is a non-zero risk that some broken peers send an event with TZID=UTC without a corresponding VTIMEZONE; falling back to our own definition handles that.

I therefore think that the suggested way of dealing with such a timezone (returning a VTIMEZONE which has the same semantic as the Z suffix) is still the best solution.
Comment 10 Chenthill P 2009-09-16 17:55:50 UTC
It wouldn be possible to change the tzid in that function. One question I have is, when exporting the calendar as .ics file from evolution is the tzid correctly exported ? does it get exported as Etc/UTC instead of just UTC ?
Comment 11 Patrick Ohly 2009-09-17 10:08:24 UTC
(In reply to comment #10)
> One question I have
> is, when exporting the calendar as .ics file from evolution is the tzid
> correctly exported ? does it get exported as Etc/UTC instead of just UTC ?

You are on to something here. No, it wasn't.

However, that wasn't due to the code in e_cal_get_timezone(). e_cal_check_timezones() does the same "Etc/UTC" -> "UTC" mapping inside e_cal_match_timezone() and then imports the event into the calendar with TZID=UTC. Displaying the event works fine (I had tested that) but exporting it creates an event with DTSTART;TZID=UTC:20090825T160000 and no corresponding VTIMEZONE definition. Not good.

My suggestion is to disable the mapping to "UTC" in e_cal_match_timezone() because e_cal_check_timezones() is not able to handle this correctly. I'll attach a patch. This makes Evolution import the example with TZID=Etc/UTC preserved unchanged, which then can be exported like that again (tested).

The e_cal_get_timezone() patch then becomes obsolete. However, the assert should be removed (part of the patch).

e_cal_match_timezone() could be changed so that it actually turns all time values into fixed UTC (Z suffix), but even after thinking about it a bit I don't see an easy way to do it. It has to be done correctly for all kinds of properties; some of them look complicated (EXDATE with list of time values!). I don't have time for such a patch and/or wouldn't be able to ensure that it really works in all cases.
Comment 12 Patrick Ohly 2009-09-17 10:10:02 UTC
Created attachment 143331 [details] [review]
avoid problematic mapping to "UTC", remove asserts
Comment 13 Chenthill P 2009-09-18 12:21:50 UTC
Comment on attachment 143331 [details] [review]
avoid problematic mapping to "UTC", remove asserts

The patch looks good. Please commit the same to master immediately and stable after the hard code freeze ends next week.

I don see any need to change the tzid though since it can be anything, just in this case its UTC.
Comment 14 Akhil Laddha 2009-09-23 09:52:33 UTC
Patch committed in master (2.29.1+)
http://git.gnome.org/cgit/evolution-data-server/commit/?id=ccea859f88844c0cc3a94857e47eade6b1257cba

@Patrick, please commit the patch in gnome-2-28 branch, change the patch statue in bug and close the bug, TIA
Comment 15 Patrick Ohly 2009-09-23 11:51:50 UTC
(In reply to comment #14)
> Patch committed in master (2.29.1+)
> http://git.gnome.org/cgit/evolution-data-server/commit/?id=ccea859f88844c0cc3a94857e47eade6b1257cba
> 
> @Patrick, please commit the patch in gnome-2-28 branch, change the patch statue
> in bug and close the bug, TIA

Done, commit d90c3992e956c17e5dc6e34844529c827d35c749.

I didn't commit it there earlier because Chenthil said saomething about a freeze.