413908 - Events using internal timezones are no longer updated to recent timezone version

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Stefan Sitter [:ssitter]]

Attachment: sample event using timezone "/mozilla.org/20050126_1/Europe/Berlin" from Sunbird 0.3

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12pre) Gecko/2008012408 Calendar/0.8pre

Events using internal timezones are no longer updated to recent timezone version

Steps to Reproduce A:
1. Start Sunbird with new profile
2. Import the attached testcase into default storage calendar
3. Check the timezone in storage.sdb (e.g. using the SQLite Manager extension) or export the event to iCalendar format and check the timezone in the .ics file

Steps to Reproduce B:
1. Start Sunbird with new profile
2. Save attached testcase to local file and open it via menu File -> Open Calendar File
3. In the new ics calendar create a new dummy event
4. Check the timezone for the original event in the updated .ics file

Actual Results using Sunbird 0.7
Using Sunbird 0.7: The event was updated from timezone "/mozilla.org/20050126_1/Europe/Berlin" to "/mozilla.org/20070129_1/Europe/Berlin".

Actual Results using Sunbird 0.8pre:
The event is still using timezone "/mozilla.org/20050126_1/Europe/Berlin".

Expected Results:
Same behavior as in Sunbird 0.7.

Comment 1

[Bas van den Bosch]

The TZ-definition is probably treated as a foreign timezone. When importing evenst there should be a check for native timezones and old definitions should be replaced. 

Comment 2

[Daniel Boelzle [:dbo]]

Yes, all external timezones are now strictly treated as foreign. Since they are prefixed as "/mozilla.org/" we _could_ map them to current versions, but I am still unsure whether we better should just drop the prefix in the future; strictly thought the leading solidus character '/' is not allowed for TZIDs.

Comment 3

[Stefan Sitter [:ssitter]]

Yes, for the future removing the prefix this makes sense. (By the way: If I remember correctly this was being worked on for 0.8 with the tz database but was post phoned).

Currently we still use the /mozilla.org/yyyymmdd... timezone names. If the existing events with this timezones are not updated to the current timezone definition I think we might experience events that are displayed at wrong times and issues with daylight saving time start/end date. 

Comment 4

[Bas van den Bosch]

Can't we just drop the leading solidus? 

Comment 5

[Daniel Boelzle [:dbo]]

(In reply to comment #3)
> Yes, for the future removing the prefix this makes sense. (By the way: If I
> remember correctly this was being worked on for 0.8 with the tz database but
> was post phoned).
IMO this is not necessarily bound to the timezone database thing. Gary could as well drop those in the latest timezone update bug, too.

> Currently we still use the /mozilla.org/yyyymmdd... timezone names. If the
> existing events with this timezones are not updated to the current timezone
> definition I think we might experience events that are displayed at wrong times
> and issues with daylight saving time start/end date. 
Yes, might be the case if e.g. a timezone adds DST.
I think we need to define what data gets automatic timezone updates and under what circumstances. No doubt about doing automatic updates for local calendars (storage.sdb), but I am unsure whether we should do that for external data (ics subcriptions). However, we could silently update those we've issued (recognizing them either by the mozilla.org prefix or adding an X-prop into the timezone definition).
A consequence that comes to my mind that we need to consider when dropping the mozilla.org prefix: We will face clashes with foreign definitions using the same Olson TZIDs. If we are sure Sunbird/Lightning runs the latest Olson definitions, then it's ok to override those IMHO.

Comment 6

[Daniel Boelzle [:dbo]]

(In reply to comment #4)
I think we need to drop it anyway in the future, because it's reserved for a global timezone registry. Question is whether we should drop the mozilla.org prefix in the same step, too.

Comment 7

[Daniel Boelzle [:dbo]]

should be resolved for 0.9

Comment 8

[Daniel Boelzle [:dbo]]

related comment <https://bugzilla.mozilla.org/show_bug.cgi?id=363191#c26>

Comment 9

[Daniel Boelzle [:dbo]]

Attachment: patch

This patch
- adds API for versioning the timezone-service / timezones.sqlite
- auto updates storage calendar's timezones if the timezone-service's version has changed
- changes precedence for ICS parsing timezone lookup: Olson (timezone-service) timezones take precedence over the included ones in the ical data, so the data stays current.

This whole patch assumes that our (timezone-service) Olson set of definitions is always the best choice (the most up-to-date), because any foreign timezone definition tagged with such a TZID (an Olson one) is going to be exchanged with the timezone-service's definition, thus overwritten.

Comment 10

[Daniel Boelzle [:dbo]]

Attachment: timezones.sqlite with patched version

+ changed the schema of version to TEXT

Comment 11

[Daniel Boelzle [:dbo]]

Comment on attachment 324051 [details] [diff] [review]
patch

>+    updateTimezones: function stor_updateTimezones() {
>+        // check if timezone version has changed:
>+        var selectTzVersion = createStatement(this.mDB, "SELECT version FROM cal_tz_version");
>+        var version = (selectTzVersion.step() ? selectTzVersion.row.version : null);
>+        if (!version || getTimezoneService().version != version) {

We should better use nsIVersionComparator here, and check whether there's no version yet or the timezoneservice's version is newer than the current to prevent downgrading the timezones (referring to bug 437692 comment #3).

Comment 12

[Daniel Boelzle [:dbo]]

Attachment: patch - v2

Updated patch.

In addition:
- added version checks and disallowing downgrades of calendar-timezones.sqlite (bug 437692 comment #3)
- updates cal mgr schema to 10 as suggested in bug 437622 comment #8 (and thus might fix bug 438372), fixing the schema check for 0.8. But I think this coupling is odd for the future (schema updates in storage need to bump cal mgr's schema version), thus I've added calIErrors.STORAGE_UNKNOWN_SCHEMA_ERROR being thrown by the provider in those cases in the future (cal mgr will alert and quite as before for now).
- Reset some DB statements: not resetting statements caused those to have locks on tables (at least my debugging experience on Windows).

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 324619 [details] [diff] [review]
patch - v2

>+            // In general, we might consider to keep calendars disabled in case they couldn't
>+            // be created instead of filtering them out. This leaves the chance to clean up
>+            // calendar registration, getting rid of error boxes.
Please mark this as TODO and file a followup bug (if possible note here). I think we should show calendars that have no provider in the calendar list as disabled, without the possibility to enable.

>             for each (var caldata in newCalendarData) {
>                 try {
>                     var cal = this.createCalendar(caldata.type, makeURL(caldata.uri));
>+                    if (!cal) {
>+                        continue;
>+                    }
For now, what about a WARN() message that there is still a stray calendar? If we decide to put stray calendars back into the calendar list, users might wonder where all those calendars came from. If they constsantly saw a warning in their console, some might not be as supprised.
> calTimezoneService.prototype = {
>+    createStatement: function calTimezoneService_createStatement(sql) {
>+        var statement = this.mDb.createStatement(sql);
>+        var ret = Components.classes["@mozilla.org/storage/statement-wrapper;1"]
>+                            .createInstance(Components.interfaces.mozIStorageStatementWrapper);
>+        ret.initialize(statement);
>+        return ret;
>+    },
Since also used in storage, maybe a util function instead?

>             try {
>                 sqlTzFile.append("timezones.sqlite");
>                 LOG("using " + sqlTzFile.path);
>                 var dbService = Components.classes["@mozilla.org/storage/service;1"]
>                                           .getService(Components.interfaces.mozIStorageService);
>                 this.mDb = dbService.openDatabase(sqlTzFile);
>+                this.mSelectByTzid = this.createStatement("SELECT * FROM tz_data WHERE tzid = :tzid LIMIT 1");
Interestingly enough, this.mSelectByTzid being null in case the database cannot be opened is the only thing keeping items from showing up in the unifinder. Maybe we can open the addons manager and close the main window, showing a dialog that the xpi needs to be installed?


     g_stringBundle = calGetStringBundle(bundleURL);
>             } catch (exc) {
>                 var msg = calGetString("calendar", "missingCalendarTimezonesError");
>                 LOG(msg);
>                 Components.utils.reportError(msg);
>                 showError(msg);
Lots of different error locations here...I don't remember what you answered on the last review. Also, it seems that showError is never correctly executed. In showError, a window is expected and since this call is from a component there is no window. An exception in the error console appears. Adding |var window;| to showError makes the message box show up, but it pops up about 4-5 times, which is probably not what we want. Either remove showError or fix it.


calStorageCalendar seems to have quite a few execute() without reset()s. While you are at it, could you add those? Also a try/catch block around the stuff in deleteCalendar() would be nice. I know its totally off topic though, I'm fine with doing that in a different bug.

Comment 14

[Stefan Sitter [:ssitter]]

(In reply to comment #13)
> >                 LOG(msg);
> >                 Components.utils.reportError(msg);
> >                 showError(msg);
> Lots of different error locations here...

Yeah, see my Bug 432719. In particular reporting the same message once as information and once as error is confusing. I suggest to report it only once and to clear up if it's an error or not.

Comment 15

[Daniel Boelzle [:dbo]]

(In reply to comment #13)
> (From update of attachment 324619 [details] [diff] [review])
> >+            // In general, we might consider to keep calendars disabled in case they couldn't
> >+            // be created instead of filtering them out. This leaves the chance to clean up
> >+            // calendar registration, getting rid of error boxes.
> Please mark this as TODO and file a followup bug (if possible note here). I
> think we should show calendars that have no provider in the calendar list as
> disabled, without the possibility to enable.
done, filed bug 439620

> >             for each (var caldata in newCalendarData) {
> >                 try {
> >                     var cal = this.createCalendar(caldata.type, makeURL(caldata.uri));
> >+                    if (!cal) {
> >+                        continue;
> >+                    }
> For now, what about a WARN() message that there is still a stray calendar? If
> we decide to put stray calendars back into the calendar list, users might
> wonder where all those calendars came from. If they constsantly saw a warning
> in their console, some might not be as supprised.
Users already get an error box, that ought to be sufficient.

.getService(Components.interfaces.mozIStorageService);
> >                 this.mDb = dbService.openDatabase(sqlTzFile);
> >+                this.mSelectByTzid = this.createStatement("SELECT * FROM tz_data WHERE tzid = :tzid LIMIT 1");
> Interestingly enough, this.mSelectByTzid being null in case the database cannot
> be opened is the only thing keeping items from showing up in the unifinder.
> Maybe we can open the addons manager and close the main window, showing a
> dialog that the xpi needs to be installed?
I am really no friend of adding such code (Stefan already suggested something similar). I think a solution based on requirements would make all of this code obsolete (defining that Lightning and Sunbird require the calendar-timezones.xpi). We should go for that.

>      g_stringBundle = calGetStringBundle(bundleURL);
> >             } catch (exc) {
> >                 var msg = calGetString("calendar", "missingCalendarTimezonesError");
> >                 LOG(msg);
> >                 Components.utils.reportError(msg);
> >                 showError(msg);
> Lots of different error locations here...I don't remember what you answered on
> the last review. Also, it seems that showError is never correctly executed. In
> showError, a window is expected and since this call is from a component there
> is no window. An exception in the error console appears. Adding |var window;|
> to showError makes the message box show up, but it pops up about 4-5 times,
> which is probably not what we want. Either remove showError or fix it.
Basically we don't want to run the whole code if calendar-timezones are not installed; I think triggering the dialog multiple times is ugly, but OK for now. I'll remove the LOG entry though.

> calStorageCalendar seems to have quite a few execute() without reset()s. While
> you are at it, could you add those? Also a try/catch block around the stuff in
> deleteCalendar() would be nice. I know its totally off topic though, I'm fine
> with doing that in a different bug. 
done that for calStorageCalendar.js


Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.