Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ICU-21465 Fix Windows Time Zone offset when Automatic DST setting is Off #1539

Merged
merged 1 commit into from
Jan 26, 2021

Conversation

jefgen
Copy link
Member

@jefgen jefgen commented Jan 26, 2021

This PR fixes a bug in the changes for ICU-13845, for handling when the "Automatic DST" setting is OFF in the Control Panel.

It turns out that the offset value for the calculated time zone when the "Automatic DST" setting is turn off is wrong. The sign for the time zone offset is flipped. (It's positive when it should be negative, and vice-versa).

I think this was perhaps due to confusion about the value for the UTC offset, how POSIX style offsets are handled, and what is reported by the Win32 API GetDynamicTimeZoneInformation.
It seems that this issue was actually present in the original PR for ICU-13845 here: #1297 , and it was missed in the reviews on the other PR for the ticket here: #1362 .

I also missed this when I was manually testing out the changes, as I was expecting the wrong values for the offset to be reported. (For example, Etc/GMT-8 instead of Etc/GMT+8).

Note: The actual change here is only one character. The rest are comments that I added in order to help explain things for others in the future.

Checklist

@FrankYFTang
Copy link
Contributor

Could you list a step-by-step (manual is ok) test set up procedure that we can follow to reproduce the problem before the fix, and verify your PR fix the problem after we apply the PR?

@FrankYFTang
Copy link
Contributor

Also, for the issue fixed in this PR, could you also scope under which timezone, what condition (control panel setting, os) and when (say between which months) would the issue be reproducible and then fixed by this PR. Is it reproducible in any timezone which observe daylight saving time?

@jefgen
Copy link
Member Author

jefgen commented Jan 26, 2021

Could you list a step-by-step (manual is ok) test set up procedure that we can follow to reproduce the problem before the fix, and verify your PR fix the problem after we apply the PR?

Yes, I've created a document that has different manual test cases that exercise various scenarios for most of the code paths for the Windows TZ mapping in ICU. (I also added notes on what the various test cases are exercising in the code.)
Link: https://docs.google.com/document/d/17Y_Ns4EBV8wnHioJNFc-dJCRRpJ3hpJZDYmTq6ekIsM

Test case 7 in that document reproduces the issue; with the fix the correct result of Etc/GMT-10 should be reported back from the icuinfo.exe tool.

Also, for the issue fixed in this PR, could you also scope under which timezone, what condition (control panel setting, os) and when (say between which months) would the issue be reproducible and then fixed by this PR. Is it reproducible in any timezone which observe daylight saving time?

This issue would effect anyone that has set the "Automatic DST" setting to be OFF in the Control Panel -- and potentially anyone with the random corruption that was seen on the Chrome bug report. It could occur for any time zone that has a non-zero offset, and wouldn't matter if it was during daylight saving time or not.

@jefgen jefgen merged commit a1a1faf into unicode-org:master Jan 26, 2021
@jefgen jefgen deleted the jefgen/fix-ICU-21465_DST_offset branch January 26, 2021 23:43
jefgen added a commit to microsoft/icu that referenced this pull request Feb 2, 2021
ry pushed a commit to denoland/icu that referenced this pull request Feb 10, 2021
upstream PR unicode-org/icu#1539

Bug: 1168528
Change-Id: I118919e0140ce96c4b8a6f5d308d7e2d8584a1d6
TBR: jshin@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/deps/icu/+/2649148
Reviewed-by: Frank Tang <ftang@chromium.org>
imran1008 pushed a commit to bloomberg/chromium.bb that referenced this pull request Apr 27, 2021
upstream PR unicode-org/icu#1539

Bug: 1168528
Change-Id: I118919e0140ce96c4b8a6f5d308d7e2d8584a1d6
TBR: jshin@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/deps/icu/+/2649148
Reviewed-by: Frank Tang <ftang@chromium.org>
(cherry picked from commit 2eefd9a18a2084e02b12691a60ff932f82cfb385)
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/deps/icu/+/2657048
Reviewed-by: Jungshik Shin <jshin@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants