deps: backport ICU-20558 to fix Intl crasher by srl295 · Pull Request #27415 · nodejs/node · GitHub

Floating patch for ICU 63.x and 64.x
fixing crash in Intl when ICU data not found.

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: #27379

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3366

fyi @jefgen

Notes:

the '64' patch is direct from the PR
the '63' patch is https://github.com/unicode-org/icu/pull/632.diff applied to the dtptngen.cpp file from the node.js embedded small-icu (which is version 63)

RSLGTM

Added a regression test for this. @srl295 obviously feel free to push it out or change it.

nice test, thanks @refack !

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

Rebased and squashed.

CI: https://ci.nodejs.org/job/node-test-pull-request/22720/

@refack do you want me to squash again?

@refack do you want me to squash again?

No rush, we can squash while landing (I could also force push myself). I'm waiting for CI job 22720 to finish so we could start a resume job for the failing platforms.

CI: https://ci.nodejs.org/job/node-test-pull-request/22722/

@refack want to take this over ?

@refack want to take this over ?

Yeah, I'm on it.

CI: https://ci.nodejs.org/job/node-test-pull-request/22723/

@here now that #27361 landed, we probably should fast-track this since it solves #27379 which is kind of harsh.

Please 👍 if you concur.

RSLGTM

Landed in 617c55e

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

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

Notable changes: * intl: * Update ICU to 64.2. This adds support for Japanese Era (Reiwa). #27361 * Fixes a bug in ICU that affected Node.js 12.0.0 in the case where `new Date().toLocaleString()` was called with a non-default locale. #27415 * C++ API: * Added an `Environment` overload of `EmitAsyncDestroy`. #27255 PR-URL: TODO

Notable changes: * intl: * Update ICU to 64.2. This adds support for Japanese Era (Reiwa). #27361 * Fixes a bug in ICU that affected Node.js 12.0.0 in the case where `new Date().toLocaleString()` was called with a non-default locale. #27415 * C++ API: * Added an overload of `EmitAsyncDestroy` that can be used during garbage collection. #27255 PR-URL: #27440

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

srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Apr 25, 2019

srl295 requested review from refack, ryzokuken and targos April 25, 2019 16:36

srl295 self-assigned this Apr 25, 2019

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Apr 25, 2019

targos approved these changes Apr 25, 2019

View reviewed changes

refack approved these changes Apr 25, 2019

View reviewed changes

srl295 marked this pull request as ready for review April 25, 2019 19:12

srl295 commented Apr 25, 2019

View reviewed changes

srl295 force-pushed the patch-ICU-20558 branch from 5400049 to c369ef0 Compare April 25, 2019 19:39

fixup! Some platforms need the full env

d93152d

refack force-pushed the patch-ICU-20558 branch from 5912d0c to d93152d Compare April 25, 2019 20:54

cjihrig approved these changes Apr 25, 2019

View reviewed changes

refack assigned refack and unassigned srl295 Apr 25, 2019

refack added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 25, 2019

jasnell approved these changes Apr 26, 2019

View reviewed changes

ryzokuken approved these changes Apr 26, 2019

View reviewed changes

This was referenced Apr 26, 2019

Node.js crash with a fatal error on Date#toLocaleString #27379

Closed

deps: backport ICU-20575 to fix err/crasher #27435

Closed

targos closed this Apr 27, 2019

targos mentioned this pull request Apr 27, 2019

v12.1.0 proposal #27440

Merged

targos mentioned this pull request May 1, 2019

v10.16.0 proposal #27514

Merged

elvisbegovic mentioned this pull request May 7, 2019

ng xi18n fails with Node 12.0 (FIXED with Node.js 12.1+) angular/angular-cli#14351

Closed

refack removed their assignment Jun 6, 2019

deepak1556 mentioned this pull request Jan 30, 2020

fix: crash with icu when input locale is invalid electron/electron#21969

Merged

6 tasks

deps: backport ICU-20558 to fix Intl crasher by srl295 · Pull Request #27415 · nodejs/node · GitHub

Uh oh!

Uh oh!

deps: backport ICU-20558 to fix Intl crasher - #27415

deps: backport ICU-20558 to fix Intl crasher#27415srl295 wants to merge 2 commits intonodejs:masternodejs/node:masterfrom srl295:patch-ICU-20558srl295/node:patch-ICU-20558Copy head branch name to clipboard

Conversation

srl295 commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Apr 25, 2019

Uh oh!

srl295 commented Apr 25, 2019

Uh oh!

srl295 commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Apr 25, 2019

Uh oh!

srl295 left a comment

Choose a reason for hiding this comment

Uh oh!

srl295 commented Apr 25, 2019

Uh oh!

nodejs-github-bot commented Apr 25, 2019

Uh oh!

srl295 commented Apr 25, 2019

Uh oh!

refack commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 25, 2019

Uh oh!

srl295 commented Apr 25, 2019 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Apr 25, 2019

Uh oh!

nodejs-github-bot commented Apr 25, 2019

Uh oh!

refack commented Apr 25, 2019

Uh oh!

ryzokuken left a comment

Choose a reason for hiding this comment

Uh oh!

targos commented Apr 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

deps: backport ICU-20558 to fix Intl crasher#27415
srl295 wants to merge 2 commits into
nodejs:masterfrom
srl295:patch-ICU-20558

srl295 commented Apr 25, 2019 •
edited
Loading

srl295 commented Apr 25, 2019 •
edited
Loading

refack commented Apr 25, 2019 •
edited
Loading

srl295 commented Apr 25, 2019 via email •
edited
Loading

targos commented Apr 27, 2019 •
edited
Loading