Ahmed Nefzaoui [:Nefzaoui] Reporter
	Description • 9 years ago

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.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 1 • 9 years ago

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?

	Gregor Wagner [:gwagner]
	Comment 2 • 9 years ago

[Blocking Requested - why for this release]:

	Mahendra Potharaju [:mahe]
	Comment 3 • 9 years ago

Blocks 2.5 with a Priority P2.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 4 • 9 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 5 • 9 years ago

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

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 6 • 9 years ago

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

	Julien Wajsberg [:julienw]
	Comment 7 • 9 years ago

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

	Julien Wajsberg [:julienw]
	Comment 8 • 9 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 9 • 9 years ago

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

	Julien Wajsberg [:julienw]
	Comment 10 • 9 years ago

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 ?

	Samael Wang [:freesamael] (away for now)
	Comment 11 • 9 years ago

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

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 12 • 9 years ago

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

	Julien Wajsberg [:julienw]
	Comment 13 • 9 years ago

Ahah thanks ! Should we dupe ?

	Samael Wang [:freesamael] (away for now)
	Comment 14 • 9 years ago

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.

	Samael Wang [:freesamael] (away for now)
	Comment 15 • 9 years ago

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

	Samael Wang [:freesamael] (away for now)
	Comment 16 • 9 years ago

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

	Samael Wang [:freesamael] (away for now)
	Comment 17 • 9 years ago

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.

	Samael Wang [:freesamael] (away for now)
	Comment 18 • 9 years ago

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

	Samael Wang [:freesamael] (away for now)
	Comment 19 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3784fccb4dcf

	Samael Wang [:freesamael] (away for now)
	Comment 20 • 9 years ago

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.

	Samael Wang [:freesamael] (away for now)
	Comment 21 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e7081b7be72

	Samael Wang [:freesamael] (away for now)
	Comment 22 • 9 years ago

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.

	Samael Wang [:freesamael] (away for now)
	Comment 24 • 9 years ago

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.

	Jeff Walden [:Waldo]
	Comment 25 • 9 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 26 • 9 years ago

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

	Jeff Walden [:Waldo]
	Comment 27 • 9 years ago

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

	Samael Wang [:freesamael] (away for now)
	Comment 28 • 9 years ago

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.

	Samael Wang [:freesamael] (away for now)
	Comment 31 • 9 years ago

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.

	Samael Wang [:freesamael] (away for now)
	Comment 32 • 9 years ago

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?

	Samael Wang [:freesamael] (away for now)
	Comment 33 • 9 years ago

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?

	Mike Hommey [:glandium]
	Comment 34 • 9 years ago

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

	Samael Wang [:freesamael] (away for now)
	Comment 35 • 9 years ago

(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

	Samael Wang [:freesamael] (away for now)
	Comment 36 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d1b188d5591

	Samael Wang [:freesamael] (away for now)
	Comment 38 • 9 years ago

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.

	Mike Hommey [:glandium]
	Comment 39 • 9 years ago

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

	Samael Wang [:freesamael] (away for now)
	Comment 40 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=920e91cc6153

	Samael Wang [:freesamael] (away for now)
	Comment 42 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff36fc352835

	Samael Wang [:freesamael] (away for now)
	Comment 43 • 9 years ago

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.

	Samael Wang [:freesamael] (away for now)
	Comment 44 • 9 years ago

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

cancel for update

	Samael Wang [:freesamael] (away for now)
	Comment 45 • 9 years ago

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

	Jeff Walden [:Waldo]
	Comment 46 • 9 years ago

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?

	Jeff Walden [:Waldo]
	Comment 47 • 9 years ago

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.

	Samael Wang [:freesamael] (away for now)
	Comment 48 • 9 years ago

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.

	Samael Wang [:freesamael] (away for now)
	Comment 49 • 9 years ago

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

	Mahendra Potharaju [:mahe]
	Comment 50 • 9 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 51 • 9 years ago

(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

	Mahendra Potharaju [:mahe]
	Comment 52 • 9 years ago

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

	Samael Wang [:freesamael] (away for now)
	Comment 53 • 8 years ago

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

	BMO Automation
	Comment 54 • 6 years ago

Firefox OS is not being worked on