Bug 24086

Summary: Runtime test - TestDaylightSavingsTime fails against Asia/Amman for 2012 DateTimes
Product: Android Reporter: Peter Collins <peter.collins>
Component: GeneralAssignee: marcos.henrich
Status: VERIFIED FIXED    
Severity: normal CC: jonp, marcos.henrich, mohitk, mono-bugs+monodroid, udhams
Priority: Normal    
Version: 4.18.1   
Target Milestone: 5.1.4 (C5SR2)   
Hardware: Macintosh   
OS: Mac OS   
Tags: Is this bug a regression?: ---
Last known good build:

Description Peter Collins 2014-10-27 19:36:20 UTC
One runtime test is failing on a couple of TimeZones. The affected TimeZones are as follows:

> Casablanca (Western European Standard Time)
https://gist.github.com/pjcollins/76933b37fe478e3f417b

> Amman/Jordan (Eastern European Summer Time)
https://gist.github.com/pjcollins/e22470a41b2e3515773b

[FAIL] TestDaylightSavingsTime :   TimeZoneData MISMATCH! See logcat output for details.
I/mono-stdout(30188):   Expected: False
I/mono-stdout(30188):   But was:  True

Environment:
OSX Mavericks
4.18.1 / ef5be128
Comment 1 Jonathan Pryor 2014-10-27 22:58:31 UTC
The question is, how much should we care, short-term?

Casablanca is Africa/Casablanca:
http://www.timeanddate.com/time/change/morocco/casablanca

It has *two* DST intervals within a single year, and every reported error is for the 2nd DST interval. Having two DST inverts is something that *Mono* doesn't support:

https://bugzilla.xamarin.com/show_bug.cgi?id=22648
https://bugzilla.xamarin.com/show_bug.cgi?id=23170#c4
https://github.com/mono/mono/pull/1321

There is a PR to fix it, but it hasn't been merged. (Not(?) coincidentally, it fixes things by removing FIXME's within TimeZoneInfo...)

Amman/Jordan is "weird" because, as per the Year: fields in the output, there are no recognized DST rules for 2012 and 2013:

> I/mono-stdout(30682): Year: 2012; DST: 1/1/0001 12:00 AM -> 1/1/0001 12:00 AM
> I/mono-stdout(30682): Year: 2013; DST: 1/1/0001 12:00 AM -> 1/1/0001 12:00 AM

There *are* entries for 2014, but there are no errors for 2014:

> I/mono-stdout(30682): Year: 2014; DST: 3/28/2014 12:00 AM -> 10/31/2014 1:00 AM

...which is all the more confusing: how is Java getting data for 2012 and 2013 when it's TimeZone DB doesn't have data for those years? Or is there yet another DB that needs supporting?
Comment 2 Jonathan Pryor 2014-10-29 22:34:02 UTC
Current analysis: TimeZoneInfo.ParseTZBuffer() is possibly buggy.

The Android tzdata database *does* have data for Asia/Amman, 233 bytes worth in fact, yet when ParseTZBuffer() is through with the parsing there are no adjustment rules found. Very peculiar.

What's happening is we're hitting this:

https://github.com/mono/mono/blob/dacc3f8c5d4331f7a40413f3a802ec4049da1e2e/mcs/class/System.Core/System/TimeZoneInfo.cs#L1186

We have adjustment rules, but then we hit a change in tt.Name vs. standardDisplayName (?), so we DROP all encountered adjustment rules, and instead rely on "transitions":

https://github.com/mono/mono/blob/dacc3f8c5d4331f7a40413f3a802ec4049da1e2e/mcs/class/System.Core/System/TimeZoneInfo.cs#L1242

Annotated debug output:

> # ParseTZBuffer: id=Asia/Amman
> # ParseTZBuffer: abbreviations.Count=13
> # ParseTZBuffer: time_types.Count=6
> # ParseTZBuffer: transitions.Count=69
> # clearing adjustmentRules; storing transitions! standardDisplayName=; ttype.Name=EET
> # clearing adjustmentRules; storing transitions! standardDisplayName=EET; ttype.Name=AST

This "clearing" of adjustment rules and relying on transitions is why Asia/Amman has no adjustment rules. Furthermore, TimeZoneInfo.transitions is only used by TimeZoneInfo.GetUtcOffset (DateTime dateTime, TimeZoneInfo tz, out bool isDST) and TimeZoneInfo.TryGetTransitionOffset(), both of which are private, which makes it difficult to easily build anything atop them...
Comment 3 Jonathan Pryor 2014-10-29 22:37:19 UTC
@Marcos: As the developer who most recently added the TimeZoneInfo.transitions logic (on 2014-10-04)... ;-)

Please see Comment #2.

The context is that on Android devices, when the timezone is set to Asia/Amman, our timezone results differ from Android's. It seems that this is the case because there are "no" adjustment rules, because the ttype.Name value has changed, which is not quite ideal because Xamarin.Android uses the adjustment rules to determine when the DST intervals occur. (And since Asia/Amman has no adjustment rules, it's never in DST...even when it should be!)

Is there a sane/appropriate fix for this?
Comment 4 marcos.henrich 2014-10-30 10:35:05 UTC
Monodroid is still using mono 491d1f508a62588d7f8b9aa62e162fc841ec70bb. (2014-09-16)
This issues are the ones we were having before the TimeZone improvements.

Bumping monodroid should make tz.GetUtcOffset (localtime) and tz.IsDaylightSavingTime (localtime) to behave when time zones have multiple DST or TZ tables with unexpected name or base offset changes.
Comment 5 Jonathan Pryor 2014-10-30 12:48:09 UTC
> This issues are the ones we were having before the TimeZone improvements.

Unfortunately not.

Please see the new-and-improved TimeZoneInfo self-test for Android: https://github.com/mono/mono/commit/ac1dcc6adefd4887543cd73612580aca2497c543

It's reasonably self-contained, and doesn't require a full monodroid build environment, just an up-to-date Mono build environment (for Consts.cs).

It *does* require an Android device be attached, and that `adb` be in $PATH. Assuming you do:

    cd mono/mcs/class/System.Core
    make android-dump-tzdata

The above will connect to your Android device and download the TimeZone databases into the ./android directory. You can then view e.g. ./android/tzdata/Asia/Amman in your favoriate binary editor, e.g. `hexdump -C`.

Or, you can load the timezone data file and dump the detected timezone rules!

    $ mono tzi.exe -i android/tzdata/Asia/Amman
    Rules for: android/tzdata/Asia/Amman
    DB type: System.TimeZoneInfo+ZoneInfoDB

,,,and here we see the problem: THERE ARE NO RULES.

Compare to e.g. America/New_York:

    $ mono tzi.exe -i android/tzdata/America/New_York 
> Rules for: android/tzdata/America/New_York
> 	AdjustmentRules[  0]: DaylightDelta=01:00:00; DateStart=1919-01; DateEnd=1919-12; DaylightTransitionStart=03-30T02:00:00; DaylightTransitionEnd=10-26T02:00:00
> ...
> 	AdjustmentRules[115]: DaylightDelta=01:00:00; DateStart=2037-01; DateEnd=2037-12; DaylightTransitionStart=03-08T02:00:00; DaylightTransitionEnd=11-01T02:00:00

So the overall dump logic is correct (afaict); the problem is that TimeZoneInfo.GetAdjustmentRules() for Asia/Amman returns nothing, which differs from Java.
Comment 6 Jonathan Pryor 2014-10-30 12:51:13 UTC
Additional testing/poking around suggests that things work if I remove this block of code:

https://github.com/mono/mono/blob/dacc3f8c5d4331f7a40413f3a802ec4049da1e2e/mcs/class/System.Core/System/TimeZoneInfo.cs#L1182-1190

At least, for this invocation:

    mono tzi.exe --offset=2012-10-24 -i android/tzdata/Asia/Amman

Before the patch:

> Using DateTime Offset: 10/24/2012 12:00:00 AM
> Rules for: android/tzdata/Asia/Amman
> 	Date(10/24/2012 12:00:00 AM): Offset(03:00:00) IsDST(False)

After the patch:

> Using DateTime Offset: 10/24/2012 12:00:00 AM
> Rules for: android/tzdata/Asia/Amman
> 	AdjustmentRules[  0]: DaylightDelta=03:00:00; DateStart=1973-01; DateEnd=1973-12; DaylightTransitionStart=06-05T22:00:00; DaylightTransitionEnd=10-01T00:00:00
> 	AdjustmentRules[  1]: DaylightDelta=03:00:00; DateStart=1974-01; DateEnd=1974-12; DaylightTransitionStart=04-30T22:00:00; DaylightTransitionEnd=10-01T00:00:00
> ...
> 	AdjustmentRules[ 33]: DaylightDelta=03:00:00; DateStart=2012-01; DateEnd=2012-12; DaylightTransitionStart=03-29T22:00:00; DaylightTransitionEnd=10-26T01:00:00
> 	Date(10/24/2012 12:00:00 AM): Offset(03:00:00) IsDST(True)

The above shows IsDST(True), which mirrors what Java indicates.

@Marcos: What I don't understand is why block 1182-1190 is required in the first place. A variation on it was originally added in *2007* [0], which likewise clears adjustmentRules when the name changes [1], but there are no tests or commit messages stating *why* this is the case.

[0]: https://github.com/mono/mono/commit/3a941b58ef167029f61601ef2693c7b7be2da58f
[1]: https://github.com/mono/mono/commit/3a941b58ef167029f61601ef2693c7b7be2da58f#diff-da7f21f1e6508d8ab14444f5684b76daR685
Comment 7 marcos.henrich 2014-10-30 13:56:17 UTC
We cannot rely on TimeZoneInfo.GetAdjustmentRules because each time a TZ changes its base offset all the previous AdjustementRules become meaningless  and are discarded.

That means that it is not possible to use AdjustementRules to compute a correct UTC offset. But we can use TimeZoneInfo.GetUtcOffset to do it.

AndroidCurrentSystemTimeZone is using TimeZoneInfo.GetAdjustmentRules thus the problems.

AndroidCurrentSystemTimeZone.GetUtcOffset should be changed to do
return LocalTimeZone.GetUtcOffset (time);

AndroidCurrentSystemTimeZone.IsDaylightSavingTime should be changed to do
return LocalTimeZone.IsDaylightSavingTime  (time);
Comment 8 marcos.henrich 2014-11-05 08:01:18 UTC
The pull request for this issue can be found in the link below.
https://github.com/xamarin/monodroid/pull/80

@jonp The pull request fixes the failing test case. But I will also improve TimeZoneInfo.GetAdjustmentRules by removing the standardDisplayName condition which does not seem to have a reason to exist.
https://github.com/mono/mono/blob/dacc3f8c5d4331f7a40413f3a802ec4049da1e2e/mcs/class/System.Core/System/TimeZoneInfo.cs#L1182
Comment 9 marcos.henrich 2014-12-09 05:57:20 UTC
Fixed in master 7a28a794505fb5f748b5e9f2d45ff14ec55f5abe.
https://github.com/xamarin/monodroid/commit/7a28a794505fb5f748b5e9f2d45ff14ec55f5abe
Comment 10 Peter Collins 2014-12-11 18:00:50 UTC
This specific runtime test is still failing on 5.0-branch against the Asia/Amman timezone, here's the logcat output:

https://gist.github.com/pjcollins/2a4126697610c46f0cba

The failure has improved, as we do now match the java times for these failure dates, however the IsDst values for TimeZone and TimeZoneInfo do not match java in this case.

I'm flagging this as REOPENED, but I'm not certain what, if anything is actionable based on this test failure.
Comment 11 marcos.henrich 2015-02-10 07:52:03 UTC
This is happening because android has an outdated tzdb of Asia/Amman

Below is a dump of my mac and the android Asia/Amman.
The android one ends on 2012.

https://gist.github.com/esdrubal/591a33859c994041bc28

As android inDaylightTime calls return a different value than TimeZoneInfo.IsDaylightSavingTime we should assume that android is not using Asia/Amman

Android is somehow using another time zone table.
Comment 12 Jonathan Pryor 2015-03-30 10:36:25 UTC
> Android is somehow using another time zone table.

Peter: Can you please see if there's possibly another timezone DB installed on the device?

    find / -name \*tz\*
Comment 13 marcos.henrich 2015-04-01 09:36:20 UTC
I dumped the android transitions for Asia/Amman using another method and they are fine.
I am looking for the problem now.
Comment 14 marcos.henrich 2015-04-02 11:26:48 UTC
The pull request for this issue can be found in the link below.
https://github.com/mono/mono/pull/1678
Comment 15 marcos.henrich 2015-04-03 05:31:45 UTC
Fixed in mono master 4614473ae50c8b1217389335330d369befd766e0
https://github.com/mono/mono/commit/4614473ae50c8b1217389335330d369befd766e0

Fixed in mono mono-4.0.0-branch 7fc9ff987ad4e010189b7c62f2a626dd7f249da8
https://github.com/mono/mono/commit/7fc9ff987ad4e010189b7c62f2a626dd7f249da8
Comment 16 Jonathan Pryor 2015-04-03 11:28:18 UTC
Fixed in monodroid-5.1-series/eef15686.
Comment 17 Peter Collins 2015-04-09 17:04:43 UTC
I'm still seeing this failure against monodroid-5.1-series / 0fd868ab unfortunately. The Asia/Amman TZ does not want to agree with this test case.

Logcat output:
https://gist.github.com/pjcollins/c8c6f250be18eae474c6
Comment 18 marcos.henrich 2015-04-14 12:09:06 UTC
I did the cherry picks incorrectly, mono-4.0.0-branch is missing 2 commits.
This should not introduce any new problems but the bug is not fixed.

Patch request on the link below.
https://trello.com/c/8RoYES3q/70-mono-timezoneinfo-bug-fixing
Comment 19 marcos.henrich 2015-04-14 13:07:44 UTC
Disregard last comment, no commit is missing from mono-4.0.0-branch. I was misleaded by github branch comparison tool.
Comment 20 marcos.henrich 2015-04-20 10:03:23 UTC
The test is now failing only with TimeZone, the problem in TimeZoneInfo was fixed.

Note that without reflection it is not possible to get a TimeZone for a previous DateTime. Thus the error specific to the Amman in 2012 is not reproducible without reflection.

I already did some work on changing CurrentTimeZone to use TimeZoneInfo and making it system independent so we can use it in android as well.

See pull request:
https://github.com/mono/mono/pull/1696
Comment 21 marcos.henrich 2015-05-04 10:53:08 UTC
Fixed in master 2c4b3b7f00c07d75a1922e26afc20d58481bdf46
https://github.com/mono/mono/commit/2c4b3b7f00c07d75a1922e26afc20d58481bdf46

Can be fixed in mono-4.0.0-branch by PR 1758.
https://github.com/mono/mono/pull/1758
Comment 22 Peter Collins 2015-05-06 17:11:04 UTC
The PR build looks like it's addressed this at first glance, but requires further testing.
Comment 23 Mohit Kheterpal 2015-06-23 12:37:33 UTC
Seems that the issue is already verified by Peter in trello card : https://trello.com/c/8RoYES3q/37-mono-timezoneinfo-bug-fixing

Hence closing this issue.