437692 - Timezone picker is operable but shows blank entries if timezone extension is disabled

Status: RESOLVED FIXED

(Calendar :: Preferences, defect)

Description

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre) Gecko/2008060611 Calendar/0.9pre

Steps to Reproduce:
===================
1. Start Sunbird with new profile
2. Open Tools -> Add-ons and disable Timezone Definitions for Mozilla Calendar
3. Restart Sunbird
4. Open Tools -> Options -> Timezones

Actual Results:
===============
Timezone picker is displayed and operable but shows only blank entries. Error Console shows:

Info: using [...]\extensions\[e-mail]\timezones.sqlite

Info: No chrome package registered for chrome://calendar-timezones/locale/timezones.properties

Error: Assert failed: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIStringBundle.GetStringFromName]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: file:///[...]/js/calTimezoneService.js :: calIntrinsicTimezone_get_displayName :: line 66"  data: no]

Expected Results:
=================
Although the timezone extension is disabled the database file timezones.sqlite are still present. Therefore the timezone service opens it, reads it and works as normally. 

But when trying to get a localized name for the timezone picker it fails because the extensions is disabled and the timezones.properties file is not registered.

I suggest to fallback to the Olson names in that case, e.g. "Europe/London".

Comment 1

[Daniel Boelzle [:dbo]]

I think we're better off defining a proper dependency, so users wouldn't be permitted to disable/deinstall calendar-timezones.xpi.
IMO the proper way would probably be
- for lightning to em:require calendar-timezones.xpi
- for sunbird to extend the lightning stub to em:require calendar-timezones.xpi

I think this should be part of the works on bug 437441.

Comment 2

[Stefan Sitter [:ssitter]]

Falling back to a safe solution instead of throwing exceptions in error case wouldn't hurt too. (In addition to correct extension dependencies)

Comment 3

[Daniel Boelzle [:dbo]]

Stefan, I am not convinced that we should add code to actually hide a deployment problem. By now it's only chrome, in the future it may be other stuff that's missing then.
However, if we couldn't define a dependency (what seems to be the case for Sunbird), then it's of course better to show a proper dialog and disable/close the application.
It's even getting a bit more complicated, because we must only allow upgrades of calendar-timezones.xpi: Storage.sdb's calendar data will be updated to the latest timezones (bug 413908) in the future. A downgrade of calendar-timezones could potentially pose dataloss, because timezones once known are unknown then.

Comment 4

[Stefan Sitter [:ssitter]]

Attachment: [checked in] improve error reporting

The same error could happen if the extension is enabled but there is an error in the localization as experienced before. For example some errors like incorrect line breaks or encoding issues were not detected in the past because compare-locales works different than the stringbundle service.

This patch improves the situation by reporting the exact error and timezone that fails and displaying the raw timezone name instead of an empty string. This bug and the patch does not target issues like extension dependencies or extension upgrade behavior.

Comment 5

[Daniel Boelzle [:dbo]]

Comment on attachment 349582 [details] [diff] [review]
[checked in] improve error reporting

>     get displayName calIntrinsicTimezone_get_displayName() {
...
>             } catch (exc) {
>-                ASSERT(false, exc);
>-                this.mDisplayName = null;
>+                Components.utils.reportError("Failed to get display name for timezone '" + this.tzid + "'. \n" + exc);

I've been thinking about reporting this as an error again, and think it's better to just log the error, because we might not have options for a complete l10n cycle each time we update the timezones.xpi, and thus have unlocalized strings. We don't want to bug users with (presumably) a few strings over and over again.

>+                this.mDisplayName = this.tzid;

You need to document this in the IDL file.


I've fixed this problem in a similarly already as part of bug 406579, attachment 348561 [details] [diff] [review] and will take those hunks.

r=dbo anyway

Comment 6

[Daniel Boelzle [:dbo]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/3c516d1c1248>

I am yet not convinced that this fixes the inherent problem. For now it's just resources (sqlite, jar), but it may be code in the future that could be disabled. Maybe listening for addon changes and preventing those is better.

Comment 7

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

We are concentrating on Lightning, where the tz files are bundled, so we have no issues when the tz extension is missing. If someone wants to revive Sunbird, I think the tz files should be bundled there too. Closing this bug for now.