1504656 - Locale fallback handling for Intl.RelativeTimeFormat doesn't work correctly

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[André Bargull [:anba]]

ICU bug: https://unicode-org.atlassian.net/browse/ICU-20253

Test case:
---
addIntlExtras(Intl);
print(new Intl.RelativeTimeFormat("ak").format(1, "second"));
---

Expected:
- Prints "+1 s"

Actual:
- Throws "Error: internal error while computing Intl data"

Comment 1

[André Bargull [:anba]]

Attachment: bug1504656.patch

ICU4C doesn't fallback to StandardPlural::OTHER (cf. ICU4J at [1]) when no formatter was found, which leads to reporting U_INVALID_FORMAT_ERROR in [2].


[1] https://github.com/unicode-org/icu/blob/e1e5f363a09144aac359b1b68552bb25e4251fd9/icu4j/main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java#L784-L793
[2] https://searchfox.org/mozilla-central/rev/3564dfde3b125878dc5a04fe92629fc5195942df/intl/icu/source/i18n/reldatefmt.cpp#896,898

Comment 2

[André Bargull [:anba]]

Setting the tracking flag for Firefox 65, because this issue blocks bug 1504334, which in turn is slated for Firefox 65 and already checked in.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 65.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Waldo, can you please prioritize this review since it needs to be uplifted to 65 and we're already nearly past the early beta stage?

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 9029203 [details] [diff] [review]
bug1504656.patch

Review of attachment 9029203 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/icu/source/i18n/reldatefmt.cpp
@@ +165,5 @@
> +    while (true) {
> +        int32_t style = fStyle;
> +        do {
> +            if (relativeUnitsFormatters[style][unit][pastFutureIndex][pluralUnit] != nullptr) {
> +                return relativeUnitsFormatters[style][unit][pastFutureIndex][pluralUnit];

eeeeugh that is horrible repetition but must not get sniped changing imported code

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53d23de684a2
Add patch to workaround ICU bug in RelativeDateTimeCacheData::getRelativeDateTimeUnitFormatter.  r=jwalden

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 9029203 [details] [diff] [review]
bug1504656.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1504334

User impact if declined: Certain sorts of relative-time formatting operations will throw an inscrutable internal error.  I don't know how many locales are susceptible to this problem, to say precisely how extensive the impact is.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Unlike most of our local ICU patches, this one hasn't been taken upstream yet.  That isn't entirely ideal.

However, the rationale for the change as given in the ICU ticket seems straightforward, and it looks like this is just redoing an operation with inputs that can already be triggered other ways, so it doesn't seem to open up new risk.

String changes made/needed: N/A

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/53d23de684a2

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9029203 [details] [diff] [review]
bug1504656.patch

[Triage Comment]
Fixes a bug in ICU which causes some relative-time formatting operations to throw an error. Approved for 65.0b8.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d7fff5efe77d