Closed Bug 1504656 Opened 2 years ago Closed 1 year ago

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

Categories

(Core :: JavaScript: Internationalization API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox65 + fixed
firefox66 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(1 file)

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"
Attached patch bug1504656.patchSplinter Review
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
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #9029203 - Flags: review?(jwalden+bmo)
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.
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?
Flags: needinfo?(jwalden)
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
Attachment #9029203 - Flags: review?(jwalden) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53d23de684a2
Add patch to workaround ICU bug in RelativeDateTimeCacheData::getRelativeDateTimeUnitFormatter.  r=jwalden
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
Flags: needinfo?(jwalden)
Attachment #9029203 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/53d23de684a2
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
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.
Attachment #9029203 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.