--with-icu-data crashes if invalid data specified

[sxa]

When node is build with the intl options enabled, if you try to use an external ICU data file with --icu-data-file and point it at a directory which either doesn't exist or does not contain valid ICU data, then node will crash just saying "memory fault (coredump)" and nothing else.

This has been improved in later V8 in , but I'd like to get it backported to Node.js so that a more useful error message can be relayed to the user. We could potentially do a cleaner fix that would return a message at the javascript levell as perhttps://github.com/srl295/node/issues/6#issuecomment-57350215 but in order to avoid diverging more than is necessary from the V8 code, I suggest pulling in the fix from V8 which will at least allow the user to get a message indicating the likely error.

Pulling in the fix (it's in https://github.com/sxa555/node/tree/v0.12-icu-data-crash) gives the following output when the condition occurs which lets the user see a message. (Wording could potentially be changed to indicate a likely problem with the setting of our --icu-data-dir parameter). I have avoided creating a PR from that for now since it's code in the deps directory.

Fatal error in , line 0

Failed to create ICU date format, are ICU data files missing?

==== C stack trace ===============================

1: V8_Fatal
2: v8::internal::DateFormat::InitializeDateTimeFormat(v8::internal::Isolate_, v8::internal::Handlev8::internal::String, v8::internal::Handlev8::internal::JSObject, v8::internal::Handlev8::internal::JSObject)
3: v8::internal::Runtime_CreateDateTimeFormat(int, v8::internal::Object__, v8::internal::Isolate_)
4: ??
Illegal instruction(coredump)

[srl295]

Coredump was a v8 bug, fixed by - https://code.google.com/p/v8/issues/detail?id=3348 - don't crash on intl data missing ( already accepted upstream)

node could detect that the dir doesn't exist, but it's harder to detect that icu data is unusable.

[jasnell]

How much harder would it be to check?

[srl295]

Not much, but it's a stat() type call. I have another PR that adds the check using libuv's stat.

[sxa]

stat() would be nice, but I feel we need to trap the case where the data is present but invalid as per @srl295's comment. I've accidentally put an invalid one in place before (either from the wrong version of ICU, or from the icu customizer online tool) and just getting a memory fault with no explanation isn't especially helpful to the end user.

[srl295]

@sxa555 just to reiterate. The memory fault is a v8 bug. the current thought is to backport the above v8 patch. v8 is the api user and the right place to trap this. Once that is done, v8 will give an error instead of crashing, see commit. I argued that it should be a soft error from the v8 side, v8 considers it a fatal installation error. Perhaps someone more familliar with the v8 side could tweak the patch to just fail the specific operation.

cc @tjfontaine @misterdjules

[sxa]

Agreed - that's my preferred solution too.

[misterdjules]

@sxa555 @srl295 @jasnell Adding to the 0.12.1 milestone. Setting P-3 (nice to have), as it's not really a regression per se since it's a new feature.

[srl295]

@misterdjules so, isn't this at least partially already landed with the v8 patches?

[srl295]

@misterdjules does not look like the above patch has landed.

[srl295]

fyi @mhdawson - will float this patch myself and ping here for review

[srl295]

Note to self: https://github.com/joyent/node/wiki/V8-upgrade-process#how-to-land-a-patch-from-upstream-v8