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

RESOLVED FIXED in Firefox 65

Status

()

defect
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla66
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox65+ fixed, firefox66 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

7 months ago
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"
(Assignee)

Comment 1

6 months ago
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)
(Assignee)

Comment 2

6 months ago
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+

Comment 6

5 months ago
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?

Comment 8

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/53d23de684a2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months 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.