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-20558 Fix compat issue with DateTimePatternGenerator fallback to default root locale #632

Merged

Conversation

jefgen
Copy link
Member

@jefgen jefgen commented Apr 23, 2019

(Thanks to @gvictor for tracking down the issue.)

This commit for ICU-13778 improved the error checking in the DateTimePatternGenerator class, adding checks for errors/failures where there previously was none at all. This was done in order to catch catastrophic errors like out-of-memory (OOM), and properly report them to the caller, rather than ignoring/hiding these errors.

However, in doing so it exposed a case where the code was depending on ignoring errors in order to fallback to the Gregorian calendar when the default ICU locale is set to root.

This commit restores the previous behavior, by allowing the error of U_MISSING_RESOURCE_ERROR to fall-though and continue without reporting back an error.

Note: This regression was introduced in ICU 63, and also effects ICU 64 as well.

Checklist

@jefgen jefgen requested review from axelandrejs and daniel-ju April 23, 2019 17:18
axelandrejs
axelandrejs previously approved these changes Apr 23, 2019
@jefgen
Copy link
Member Author

jefgen commented Apr 23, 2019

FYI for @tarekgh.

@tarekgh
Copy link

tarekgh commented Apr 23, 2019

Thanks @jefgen for tagging me to the issue. could you please confirm this fix will fix the scenario mentioned in https://unicode-org.atlassian.net/browse/ICU-20558 when usign LANG=C.UTF-8?

gvictor
gvictor previously approved these changes Apr 23, 2019
Copy link
Collaborator

@gvictor gvictor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but it's a nit comment due to my unfamiliarity with code style

icu4c/source/i18n/dtptngen.cpp Show resolved Hide resolved
@jefgen
Copy link
Member Author

jefgen commented Apr 23, 2019

could you please confirm this fix will fix the scenario mentioned in https://unicode-org.atlassian.net/browse/ICU-20558 when using LANG=C.UTF-8?

FYI: @tarekgh with the change in this PR, the following code snippet now succeeds with U_USING_DEFAULT_WARNING, and no longer returns U_MISSING_RESOURCE_ERROR.

#include <stdio.h>
#include "unicode/udatpg.h"

int main()
{
  UErrorCode er = U_ZERO_ERROR;
  UDateTimePatternGenerator* pGenerator = udatpg_open("C", &er);

  UChar monthDayPattern[100];
  const UChar UDAT_MONTH_DAY_UCHAR[] = {'M', 'M', 'M', 'M', 'd', '\0'};

  udatpg_getBestPattern(pGenerator, UDAT_MONTH_DAY_UCHAR, -1, monthDayPattern, 100, &er);
  udatpg_close(pGenerator);

  printf("udatpg_getBestPattern returns %d -- %s \n", er, u_errorName(er));
  return 0;
}

Running the code:

LANG=C.UTF-8 LD_LIBRARY_PATH=~/icu/icu4c/source/lib ./test

Output:

udatpg_getBestPattern returns -127 -- U_USING_DEFAULT_WARNING

daniel-ju
daniel-ju previously approved these changes Apr 23, 2019
@jefgen jefgen requested review from sffc and markusicu April 24, 2019 17:18
srl295
srl295 previously approved these changes Apr 24, 2019
Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that this fixed the crasher nodejs/node#27379 when floated as a patch against 64.2

srl295
srl295 previously approved these changes Apr 24, 2019
@srl295
Copy link
Member

srl295 commented Apr 24, 2019

@jefgen minor, but if you squash, can you mention the original commit and issue? something like fixes regression introduced in b12a927c9365bb38831afbf76fdd0999f8f33deb ICU-13778 (for history)

you could also make the first line of the commit a bit shorter. And i'll stop bikeshedding there.

@jefgen
Copy link
Member Author

jefgen commented Apr 24, 2019

can you mention the original commit and issue?
you could also make the first line of the commit a bit shorter.

Thanks @srl295, those are good ideas. (I'll limit the first commit line to 70 chars max).

This fixes a regression introduced by commit 
b12a927 for issue ICU-13778.

The above commit improved the error checking in the
DateTimePatternGenerator class, adding checks for errors/failures
where there previously was none at all. This was done in order to
catch catastrophic errors like out-of-memory (OOM), and properly
report them to the caller, rather than ignoring/hiding these errors.

However, in doing so it exposed a case where the code was depending
on ignoring errors in order to fall-back to the Gregorian calendar
when the default ICU locale is set to root.

This restores the previous behavior, by allowing the error of
U_MISSING_RESOURCE_ERROR to fall-though and continue without
reporting back an error to the caller.

Note: This regression was technically introduced in ICU 63, and
also effects ICU 64 as well.
@jefgen jefgen force-pushed the jefgen/DateTimePatternGenerator-compat branch from 1734da6 to 3da02ca Compare April 25, 2019 00:57
@jira-pull-request-webhook
Copy link

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

~ Your Friendly Jira-GitHub PR Checker Bot

@jefgen jefgen merged commit 693adf3 into unicode-org:master Apr 25, 2019
@jefgen jefgen deleted the jefgen/DateTimePatternGenerator-compat branch April 25, 2019 17:52
srl295 added a commit to srl295/node that referenced this pull request Apr 25, 2019
- Floating patch for ICU 63.x and 64.x
- fixing crash in Intl when ICU data not found.
- Regression test from refack included.

Background:
- ICU-13778 (landed in ICU 63.1) fixed a bug but
added a regression.
- a recent v8 land in Node v12 (which one?) exposes
this bug to cause a crash when ICU data is not found.

ICU Bug: https://unicode-org.atlassian.net/browse/ICU-20558
Backport of: unicode-org/icu#632
(Commit not landed yet in ICU)
Fixes: nodejs#27379
Co-authored-by: Refael Ackermann <refack@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 27, 2019
- Floating patch for ICU 63.x and 64.x
- fixing crash in Intl when ICU data not found.
- Regression test from refack included.

Background:
- ICU-13778 (landed in ICU 63.1) fixed a bug but
added a regression.
- a recent v8 land in Node v12 (which one?) exposes
this bug to cause a crash when ICU data is not found.

ICU Bug: https://unicode-org.atlassian.net/browse/ICU-20558
Backport of: unicode-org/icu#632
Fixes: nodejs#27379
Co-authored-by: Refael Ackermann <refack@gmail.com>

PR-URL: nodejs#27415
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Apr 27, 2019
- Floating patch for ICU 63.x and 64.x
- fixing crash in Intl when ICU data not found.
- Regression test from refack included.

Background:
- ICU-13778 (landed in ICU 63.1) fixed a bug but
added a regression.
- a recent v8 land in Node v12 (which one?) exposes
this bug to cause a crash when ICU data is not found.

ICU Bug: https://unicode-org.atlassian.net/browse/ICU-20558
Backport of: unicode-org/icu#632
Fixes: #27379
Co-authored-by: Refael Ackermann <refack@gmail.com>

PR-URL: #27415
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BethGriggs pushed a commit to nodejs/node that referenced this pull request May 10, 2019
- Floating patch for ICU 63.x and 64.x
- fixing crash in Intl when ICU data not found.
- Regression test from refack included.

Background:
- ICU-13778 (landed in ICU 63.1) fixed a bug but
added a regression.
- a recent v8 land in Node v12 (which one?) exposes
this bug to cause a crash when ICU data is not found.

ICU Bug: https://unicode-org.atlassian.net/browse/ICU-20558
Backport of: unicode-org/icu#632
Fixes: #27379
Co-authored-by: Refael Ackermann <refack@gmail.com>

PR-URL: #27415
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request May 16, 2019
- Floating patch for ICU 63.x and 64.x
- fixing crash in Intl when ICU data not found.
- Regression test from refack included.

Background:
- ICU-13778 (landed in ICU 63.1) fixed a bug but
added a regression.
- a recent v8 land in Node v12 (which one?) exposes
this bug to cause a crash when ICU data is not found.

ICU Bug: https://unicode-org.atlassian.net/browse/ICU-20558
Backport of: unicode-org/icu#632
Fixes: #27379
Co-authored-by: Refael Ackermann <refack@gmail.com>

PR-URL: #27415
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
deepak1556 added a commit to electron/electron that referenced this pull request Jan 30, 2020
zcbenz pushed a commit to electron/electron that referenced this pull request Jan 31, 2020
* fix: crash with icu when input locale is invalid

Backports unicode-org/icu#632

* spec: add regression tests
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.

7 participants