Steps to reproduce: Set the device timezone to Africa/Tunis. Compare the time in the device to http://www.timeanddate.com/worldclock/tunisia/tunis Current result: The device time is one hour ahead of the actual country time.

Ted, you've been working with Timezones lately. Should we review all timezone data we have? Currently we store them in two files (not sure what's the reason): 1) https://github.com/mozilla-b2g/gaia/blob/master/shared/resources/tz.json 2) https://github.com/mozilla-b2g/gaia/blob/master/shared/resources/apn_tz.json CLDR provides us with this list: 1) http://unicode.org/cldr/trac/browser/tags/release-28/common/supplemental/metaZones.xml Kaze, you wrote the tz and apn_tz. What's your take on this? Should we try to just process CLDR and match it or are there reasons not to do that?

[Blocking Requested - why for this release]:

Blocks 2.5 with a Priority P2.

So, it seems that there's bug 1201254 that will deal with the total overhaul of our timezone storage, but we're not sure if it'll make it in for 2.5. What we can do for 2.5 is fix the entry for Tunis.

I understand the issue, but I don't understand where the issue is. Seems to me the entry in [1] is correct ? {"cc":"TN", "offset":"+01:00,+01:00", "city":"Tunis"}, We correctly have the +1 offset both in winter and summer (if I understand properly the "offset" property). Yet I see the issue on the device :) So I don't understand where the issue comes from ;) [1] https://github.com/mozilla-b2g/gaia/blob/master/shared/resources/tz.json

Yeah, I have no answer to that. It also seems to me that summer time should no effect on Tunis but it does :(

OK, taking to investigate. Maybe we have a more serious issue.

OK, now I'm quite sure the timezone information in files from comment 1 is not used. The information comes from somewhere else: maybe gecko's ICU lib.

One possibility is that it's because of bug 1209892.

Yeah I'm quite sure this is this bug. After switching to Africa/Tunis, and running "adb shell date", the returned date is OK. Hey Samuel, do you think this is a dupe to bug 1209892? Samuel, also, I don't understand if after rebooting we're supposed to have the right time though. Can you please explain ?

(In reply to Julien Wajsberg [:julienw] from comment #10) > Yeah I'm quite sure this is this bug. After switching to Africa/Tunis, and > running "adb shell date", the returned date is OK. > > Hey Samuel, do you think this is a dupe to bug 1209892? > > Samuel, also, I don't understand if after rebooting we're supposed to have > the right time though. Can you please explain ? It seems you've found a new bug, but with similar symptom. See this log I tried with Web IDE console (with the patch for Bug 1209892): > >> var d = new Date(); > undefined > >> d.getTimezoneOffset(); > -60 > >> d.toString(); > "Mon Oct 19 2015 08:54:45 GMT+0100 (CET)" > >> d.toLocaleString(); > "10/19/2015, 9:54:45 AM" You can see toString() and getTimezoneOffset() are correct, while toLocaleString() isn't. I'll take a look to see if it's also related to bionic c library.

> You can see toString() and getTimezoneOffset() are correct, while toLocaleString() isn't. I'll take a look to see if it's also related to bionic c library. See bug 1208808.

Ahah thanks ! Should we dupe ?

The ICU4C library inside Gecko is not aware of Android specific "persist.sys.timezone" property. Instead it falls back to tzname variable set by tzset(), which is always an abbreviation such as CET or PST. This causes problem when some countries do not apply the same DST rules. For example Africa/Tunis uses CET, but doens't apply the same DST rules when CET moves to CEST. In this case ICU4C creates incorrect timezone. As a workaround, export the fullname such as "Africa/Tunis" not only to "persist.sys.timezone" but also the "TZ" environment variable which ICU4C is aware of.

(In reply to Julien Wajsberg [:julienw] from comment #13) > Ahah thanks ! Should we dupe ? It might sound confusing but bug 1208808 is most likely to be the same as bug 1209892, but this one is different. I'm kind of surprised how badly timezone works on Firefox OS during the investigation of both bugs. I attached a workaround for test. You may want to take a look. I'm not decided if this should be landed to m-c yet, through.

After a second look I think we should fix it in icu4c. It's kind of icu4c bug for the specific platforms (android and b2g). (In reply to Julien Wajsberg [:julienw] from comment #5) > I understand the issue, but I don't understand where the issue is. Seems to > me the entry in [1] is correct ? > > {"cc":"TN", "offset":"+01:00,+01:00", "city":"Tunis"}, > > We correctly have the +1 offset both in winter and summer (if I understand > properly the "offset" property). Yet I see the issue on the device :) So I > don't understand where the issue comes from ;) > > [1] https://github.com/mozilla-b2g/gaia/blob/master/shared/resources/tz.json BTW, the offset defined in tz.json is not used by the system. When user picks a timezone in Settings, the timezone name is composed with the region / city name [1], and then passed to gecko [2]. Only the name (ex. "Africa/Tunis") is passed to gecko, not the offset. When calling new Date(), the JS engine asks bionic c library for local time, which eventually loads tzdata [3], so the canonical source of timezone database is tzdata, at this moment. When calling Date.toLocaleString() or mozIntl.DateTimeFormat(), however, goes through another internal timezone database shipped with icu4c [4]. [1] https://github.com/mozilla-b2g/gaia/blob/7c23cc16f55f06428ea351acc731f64837985e3b/shared/js/tz_select.js#L175 [2] https://github.com/mozilla-b2g/gaia/blob/7c23cc16f55f06428ea351acc731f64837985e3b/shared/js/tz_select.js#L222 [3] http://androidxref.com/5.1.1_r6/xref/bionic/libc/zoneinfo/tzdata (binary) [4] https://github.com/mozilla/gecko-dev/blob/master/intl/icu/source/data/misc/zoneinfo64.txt

On Android / B2G, it's preferred to use the system property "persist.sys.timezone" to get the full Olson timezone name over tzname abbreviations, since the abbreviations are sometimes ambiguous and may cause incorrect timezone offset to be used.

Upload first quick prototype of icu fix. Not sure if it breaks Firefox for Android build. Expect more refinement later.

On Android / B2G, it's preferred to use the system property "persist.sys.timezone" to get the full Olson timezone name over tzname abbreviations, since the abbreviations are sometimes ambiguous and may cause incorrect timezone offset to be used.

On Android / B2G, it's preferred to use the system property "persist.sys.timezone" to get the full Olson timezone name over tzname abbreviations, since the abbreviations are sometimes ambiguous and may cause incorrect timezone offset to be used.

Comment on attachment 8679285 [details] [diff] [review] Part 1: Support Android timezone sys prop in icu4c Hi Jeff, We're adding android system properties support to the built-in icu4c library in order to make sure |Date.toLocaleString()| gets correct timezone on B2G. Could you possibly help to review this patch? I'm especially not so sure about whether |js/src/configure.in| is the right place to updaete LD_FLAGS.

I'm out the latter half of this week, and I may not get to this before I leave. In the meantime, whether or not the approach is right, be aware that if we took this, we'd want a patch added to intl/icu-patches/ and to the shell script that performs ICU updates, to record the changes made.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #25) > I'm out the latter half of this week, and I may not get to this before I > leave. In the meantime, whether or not the approach is right, be aware that > if we took this, we'd want a patch added to intl/icu-patches/ and to the > shell script that performs ICU updates, to record the changes made. Hello Jeff, As this is a fxos 2.5 blocker, we'd like to have it being landed by Nov. 2nd. If you are not available to review this patch, do you know who else we could ask for review? Thank you!

I'll see if I can do this tomorrow morning, then. Or if it comes to it, I can sneak a little time in while I'm out -- I won't be completely incommunicado. Regarding the configure.in question, tho -- I'm pretty sure that's not the right place for it. autoconf stuff shared between the SpiderMonkey and Gecko build systems -- both may need fixing -- typically goes in build/autoconf/icu.m4, although I don't know if this is such a thing that would go there. If not there, it seems likely both js/src/configure.in and top-level configure.in would need changes. No matter what, you want :glandium for the question, 'cause I know ~nil about build stuff. :-)

On Android / B2G, it's preferred to use the system property "persist.sys.timezone" to get the full Olson timezone name over tzname abbreviations, since the abbreviations are sometimes ambiguous and may cause incorrect timezone offset to be used.

Comment on attachment 8679285 [details] [diff] [review] Part 1: Support Android timezone sys prop in icu4c As per comment 27, split build script change for reviews / feedbacks.

Comment on attachment 8680451 [details] [diff] [review] Part 1(v2): Support Android timezone sys prop in icu4c Hi Jeff, I've split the patch to make it cleaner. Could you help to continue to review this part?

Comment on attachment 8680453 [details] [diff] [review] Part 3: Update build script Hi Mike, As per comment 27, we're not quite sure where we should put '-lcutils' to. Could you provide us some suggestions?

Comment on attachment 8680453 [details] [diff] [review] Part 3: Update build script Review of attachment 8680453 [details] [diff] [review]: ----------------------------------------------------------------- You want it in OS_LIBS in config/external/icu/moz.build like we do for -lgabi++. (Kind of relatedly, why isn't b2g "simply" using gonk's icu?)

(In reply to Mike Hommey [:glandium] from comment #34) > Comment on attachment 8680453 [details] [diff] [review] > Part 3: Update build script > > Review of attachment 8680453 [details] [diff] [review]: > ----------------------------------------------------------------- > > You want it in OS_LIBS in config/external/icu/moz.build like we do for > -lgabi++. Thanks Mike! > (Kind of relatedly, why isn't b2g "simply" using gonk's icu?) About this, refer bug 1172609 comment 6

Comment on attachment 8680473 [details] [diff] [review] Part 3(v2): Update icu's moz.build Hi Mike, Could you help to review the updated build script? Thanks.

Comment on attachment 8680473 [details] [diff] [review] Part 3(v2): Update icu's moz.build Review of attachment 8680473 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/external/icu/moz.build @@ +24,5 @@ > ] > if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' and CONFIG['MOZ_ANDROID_CXX_STL'] == 'mozstlport': > USE_LIBS += [ > + 'gabi++', > + 'cutils' USE_LIBS is meant for libraries in the gecko tree, which cutils is not in, AFAIK. You'd need OS_LIBS. But then, I don't see a libcutils in my NDK, so that wouldn't work...

On Android / B2G, it's preferred to use the system property "persist.sys.timezone" to get the full Olson timezone name over tzname abbreviations, since the abbreviations are sometimes ambiguous and may cause incorrect timezone offset to be used.

Comment on attachment 8680451 [details] [diff] [review] Part 1(v2): Support Android timezone sys prop in icu4c cancel for update

(In reply to Mike Hommey [:glandium] from comment #39) > Comment on attachment 8680473 [details] [diff] [review] > Part 3(v2): Update icu's moz.build > > Review of attachment 8680473 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: config/external/icu/moz.build > @@ +24,5 @@ > > ] > > if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' and CONFIG['MOZ_ANDROID_CXX_STL'] == 'mozstlport': > > USE_LIBS += [ > > + 'gabi++', > > + 'cutils' > > USE_LIBS is meant for libraries in the gecko tree, which cutils is not in, > AFAIK. You'd need OS_LIBS. But then, I don't see a libcutils in my NDK, so > that wouldn't work... It turns out I shouldn't use property_get() from libcutils at first place. I should use __system_property_get() instead, which is included in bionic libc, and then I don't need to change build script at all. I didn't realize the difference until you mentioned libcutils is not included in NDK, thank you!

Comment on attachment 8680513 [details] [diff] [review] Part 1(v3): Support Android timezone sys prop in icu4c Review of attachment 8680513 [details] [diff] [review]: ----------------------------------------------------------------- Have you filed an upstream bug on this, and have they looked it over and okayed it? I have low confidence in our ability to correctly change ICU, and generally upstream review is necessary for me to have much confidence that we're not doing something horribly stupid. (Not to mention this means we can drop the patch the next time we update ICU, a very substantial win.) Please file and get someone to look this over, if you haven't done so already. http://bugs.icu-project.org/trac/newticket Either way, report the ticket here. ::: intl/icu/source/common/putil.cpp @@ +1010,5 @@ > tzid = getenv("TZ"); > +#if U_PLATFORM == U_PF_ANDROID > + /* > + * Use the timezone system property as a fallback of TZ to match the > + * behavior of Bionic C library on Android. Probably should add something noting that this property doesn't exist everywhere, if this link is to be believed: https://bugzilla.xamarin.com/show_bug.cgi?id=4902 @@ +1013,5 @@ > + * Use the timezone system property as a fallback of TZ to match the > + * behavior of Bionic C library on Android. > + */ > + if (tzid == NULL) { > + static char timezone[PROP_VALUE_MAX]; ICU is multi-threaded, so I rather doubt |static| is okay here. This should just be auto-allocated so that multiple threads can call this method at the same time. @@ +1014,5 @@ > + * behavior of Bionic C library on Android. > + */ > + if (tzid == NULL) { > + static char timezone[PROP_VALUE_MAX]; > + if (__system_property_get(SYS_PROP_TIMEZONE, timezone) > 0) { I'd be somewhat inclined to either use "persist.sys.timezone" literally here, or add static const char SYS_PROP_TIMEZONE[] = "persist.sys.timezone"; here. Written as-is, it's actually *harder* to know what's being done, than if it happens the other way. @@ +1015,5 @@ > + */ > + if (tzid == NULL) { > + static char timezone[PROP_VALUE_MAX]; > + if (__system_property_get(SYS_PROP_TIMEZONE, timezone) > 0) { > + tzid = timezone; Can you explain why it's okay to ignore the value of |n| here? Some of this method does, some of it doesn't. Why is this particular ignoring okay?

Also, to repeat: this change needs to be added as a diff in intl/icu-patches, and intl/update-icu.sh (?) needs to be modified to include application of the patch.

On Android / B2G, it's preferred to use the system property "persist.sys.timezone" to get the full Olson timezone name over tzname abbreviations, since the abbreviations are sometimes ambiguous and may cause incorrect timezone offset to be used.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #46) > Comment on attachment 8680513 [details] [diff] [review] > Part 1(v3): Support Android timezone sys prop in icu4c > > Review of attachment 8680513 [details] [diff] [review]: > ----------------------------------------------------------------- > > Have you filed an upstream bug on this, and have they looked it over and > okayed it? I have low confidence in our ability to correctly change ICU, > and generally upstream review is necessary for me to have much confidence > that we're not doing something horribly stupid. (Not to mention this means > we can drop the patch the next time we update ICU, a very substantial win.) > Please file and get someone to look this over, if you haven't done so > already. > > http://bugs.icu-project.org/trac/newticket > > Either way, report the ticket here. Ticket created: http://bugs.icu-project.org/trac/ticket/11992 I'll wait for upstream feedbacks before further reviewing process here. > ::: intl/icu/source/common/putil.cpp > @@ +1010,5 @@ > > tzid = getenv("TZ"); > > +#if U_PLATFORM == U_PF_ANDROID > > + /* > > + * Use the timezone system property as a fallback of TZ to match the > > + * behavior of Bionic C library on Android. > > Probably should add something noting that this property doesn't exist > everywhere, if this link is to be believed: > > https://bugzilla.xamarin.com/show_bug.cgi?id=4902 Comment added in v4. > @@ +1013,5 @@ > > + * Use the timezone system property as a fallback of TZ to match the > > + * behavior of Bionic C library on Android. > > + */ > > + if (tzid == NULL) { > > + static char timezone[PROP_VALUE_MAX]; > > ICU is multi-threaded, so I rather doubt |static| is okay here. This should > just be auto-allocated so that multiple threads can call this method at the > same time. I was hesitate about this. I saw similar usage in the same file [1]. In general system properties are also "process-scoped" but there is an edge case when a timezone change is following by an immediate read... Either I should change all cases of "tzid" to be malloc'd and let the caller to free the memory, or ask the caller to pass in an array for strcpy. I'll think about it. Maybe the upstream reviewer will give me some specific feedbacks on this. > @@ +1014,5 @@ > > + * behavior of Bionic C library on Android. > > + */ > > + if (tzid == NULL) { > > + static char timezone[PROP_VALUE_MAX]; > > + if (__system_property_get(SYS_PROP_TIMEZONE, timezone) > 0) { > > I'd be somewhat inclined to either use "persist.sys.timezone" literally > here, or add > > static const char SYS_PROP_TIMEZONE[] = "persist.sys.timezone"; > > here. Written as-is, it's actually *harder* to know what's being done, than > if it happens the other way. Addressed in v4. > @@ +1015,5 @@ > > + */ > > + if (tzid == NULL) { > > + static char timezone[PROP_VALUE_MAX]; > > + if (__system_property_get(SYS_PROP_TIMEZONE, timezone) > 0) { > > + tzid = timezone; > > Can you explain why it's okay to ignore the value of |n| here? Some of this > method does, some of it doesn't. Why is this particular ignoring okay? The parameter |n| is for the nature that tzset [2] puts the abbreviation of standard timezone to |tzname[0]|, and DST timezone to |tzname[1]|. For instance Central Europe Time could have tzname[0]="CET", and tzname[1]="CEST". The caller of |uprv_timezone| can use the parameter to determine if it wants standard timezone or DST timezone in this case. However, the TZ environment variable or persist.sys.timezone system property should be either a posix rule string such as "UTC-7", "CET6EST5", or an Olson timezone such as "Africa/Tunis" [3]. The string should be taken as a whole for parsing later so |n| isn't quite applicable here. > Also, to repeat: this change needs to be added as a diff in > intl/icu-patches, and intl/update-icu.sh (?) needs to be modified to include > application of the patch. I'll ensure this part is also reviewed and phase-in in this bug. Thanks for reminding. [1] https://github.com/mozilla/gecko-dev/blob/07ed19e582faf40f5f72567169db62452f6c6dc9/intl/icu/source/common/putil.cpp#L1597-L1598 [2] http://pubs.opengroup.org/onlinepubs/007908775/xsh/tzset.html [3] http://www.ibm.com/developerworks/aix/library/au-aix-posix/

Hey Hsin-yi, Is this bug also dependent on bug 1208808 getting fixed? If this is waiting on something else, please let me know. Thanks

(In reply to Mahendra Potharaju [:mahe] from comment #50) > Hey Hsin-yi, > > Is this bug also dependent on bug 1208808 getting fixed? If this is waiting > on something else, please let me know. > > Thanks Hey Mahe, No, this bug isn't dependent on bug 1208808 but dependent upon the icu upstream feedback [1], see comment 49. [1] http://bugs.icu-project.org/trac/ticket/11992

Moving it to 2.6+ so we could get it as part of 2.6 after upstream bug is fixed around 3/31/16

No longer work on this. However upstream is likely to fix it in near future.

Firefox OS is not being worked on