ICU-22041 Fix "Africa/Casablanca" show strange LONG displayName

[FrankYFTang]

Fix incorrect raw offset and display name of "Africa/Casablanca" caused by
extra * U_MILLIS_PER_SECOND before using return value of
uprv_getUTCtime() which is already in ms per source/common/putilimp.h

Adding test to show the issue
The code is currently only printout in -v mode
Running (notice the -v parameter)

LD_LIBRARY_PATH=lib:stubdata:tools/ctestfw:../../lib:../../stubdata:../../tools/ctestfw:$LD_LIBRARY_PATH  ./intltest format/TimeZoneTest/TestCasablancaNameAndOffset22041 -v

get

-----------------------------------------------
 IntlTest (C++) Test Suite for                 
   International Components for Unicode 72.0.1
   Bits: 64, Byte order: Little endian, Chars: ASCII
-----------------------------------------------
 Options:                                       
   all (a)                  : Off
   Verbose (v)              : On
   No error messages (n)    : Off
   Exhaustive (e)           : Off
   Leaks (l)                : Off
   utf-8 (u)                : Off
   notime (T)               : Off
   noknownissues (K)        : Off
   Warn on missing data (w) : Off
   Write golden data (G)    : Off
   Threads                  : 12
-----------------------------------------------

=== Handling test: format/TimeZoneTest/TestCasablancaNameAndOffset22041: ===
   format {
   TestSuite Format---
   
      TimeZoneTest {
      TestSuite Format: 
      TimeZoneTest test---
      
         TestCasablancaNameAndOffset22041 {
         TestSuite TestTimeZone
         TestCasablancaNameAndOffset22041---
         
         Normal Name: GMT+01:00
         Summer Name: GMT+02:00
         getRawOffset() return 3600000
         
         getOffset(Calendar::getNow(), false, raw, dst, status) return raw=0 dst=3600000 status=0
         
         } OK:   TestCasablancaNameAndOffset22041 
      } OK:   TimeZoneTest 
   } OK:   format 

Checklist

• Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22041
• Required: The PR title must be prefixed with a JIRA Issue number.
• Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
• Required: Each commit message must be prefixed with a JIRA Issue number.
• Issue accepted (done by Technical Committee after discussion)
• Tests included, if applicable
• API docs and/or User Guide docs changed or added, if applicable

[FrankYFTang]

The buggy behavior is not observable on ICU 70 but only on 71 and trunk

[FrankYFTang]

The updated test work in 0f49e50 but broken since ee6433c
The error output showing issues on two timezones "Africa/Casablanca" and "Africa/El_Aaiun"

         TestCasablancaNameAndOffset22041 {
         FAIL: TimeZone name for Africa/Casablanca should not contain '+02' since it is located in UTC, but got GMT+02:00; got 3=0x03; expected -1=0xFFFFFFFF
         FAIL: getRawOffset() and the raw from getOffset(now, false, raw, dst, status) should not be different but got; got 0=0x00; expected 3600000=0x36EE80
      
         } ERRORS (2) in TestCasablancaNameAndOffset22041
      
         TestRawOffsetAndOffsetConsistency22041 {
         FAIL: TimeZone 'Africa/Casablanca' getRawOffset() and the raw from getOffset(now, false, raw, dst, status) should not be different but got; got 0=0x00; expected 3600000=0x36EE80
         FAIL: TimeZone 'Africa/El_Aaiun' getRawOffset() and the raw from getOffset(now, false, raw, dst, status) should not be different but got; got 0=0x00; expected 3600000=0x36EE80
      
         } ERRORS (2) in TestRawOffsetAndOffsetConsistency22041 (14ms) 
     
   

[FrankYFTang]

The root problem is getRawOffset() now return 3600000 instead of 0 after #2042

[FrankYFTang]

@yumaoka please see the problem demostrated by the test inside this PR

[FrankYFTang]

This looks related https://mm.icann.org/pipermail/tz/2022-March/031331.html
we may have data issue

[yumaoka]

Sorry - I was sick since last week. Looking.

[FrankYFTang]

@yumaoka see how the unit test break

[FrankYFTang]

Sorry - I was sick since last week. Looking.

sorry to hear that. take care.

[yumaoka]

OK - so the new test cases are failing.

[yumaoka]

It looks the root cause is this line:

icu/icu4c/source/i18n/olsontz.cpp

Line 433 in e172195

getOffset((double) uprv_getUTCtime() * U_MILLIS_PER_SECOND,

ICU4J works fine, and I debugged the equivalent code side by side.

uprv_getUTCtime() returns epoch milliseconds, but this code multiply it by U_MILLIS_PER_SECOND. This produces far future time. This is usually not a problem because many zones consist with transition table and final rule. And current date usually fell into final rule part.

I think this bug affects zones that translation table is populated next 50 years - Casablanca, Sao_Paulo and some others fall into this category. And Casablanca's rule is ending with "DST" (in rearguard version), and it is treated as permanent DST.

This looks very old bug sitting there nearly 20 years...

[FrankYFTang]

• U_MILLIS_PER_SECOND

so is the fix removing " * U_MILLIS_PER_SECOND" ?

[yumaoka]

so is the fix removing " * U_MILLIS_PER_SECOND" ?

Yes. I was debugging the logic to compare the given time with transition data. The function expect epoch milliseconds as input. Java works, because the java implementation takes long (System.currentTimeMillis()). Then I noticed the value is 1000 times bigger comparing to Java version although transition data to be compared are same.

[FrankYFTang]

Scary

$  egrep U_MILLIS_PER_SECOND i18n/*|egrep uprv_getUTCtime
grep: i18n/unicode: Is a directory
i18n/olsontz.cpp:    getOffset((double) uprv_getUTCtime() * U_MILLIS_PER_SECOND,
i18n/rbtz.cpp:    getOffset(uprv_getUTCtime() * U_MILLIS_PER_SECOND,
i18n/rbtz.cpp:    UDate now = uprv_getUTCtime() * U_MILLIS_PER_SECOND;

so we have two other places has such code in rbtz.cpp

[FrankYFTang]

So... I just change the code here. Could you approve?

[yumaoka]

LGTM.

[jira-pull-request-webhook]

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

[markusicu]

Please verify that Java works correctly. I suggest a Java unit test.

[FrankYFTang]

ok, I will add a unit test soon. stay tune

[yumaoka]

I'm also surprised such bug has been there. But as I mentioned above, the problematic code is only executed for very small number of zones, and only when the last transition populated by tz database is ending rule inconsistent with current. This is why it was not revealed for long time.

getRawOffset() is old API created when ICU did not support historic transitions. And the implementation just retro-fitted to old behavior after historic transitions are supported.

[FrankYFTang]

removed the (double) cast in C++ and add Java test. PTAL

[FrankYFTang]

@sffc I will need your LGTM on chromium when I cherrypick since Jungshik is ooo this week. Better you also look at this now.

[FrankYFTang]

sorry, hit the wrong botton

[yumaoka]

Thanks. Java test case also looks good.

[jira-pull-request-webhook]

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

[markusicu]

lgtm tnx