1495244 - Crash in icu_62::DecimalFormat::setGroupingUsed

Status: RESOLVED WORKSFORME

(Core :: JavaScript: Internationalization API, defect, P3)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-2e83d66f-5161-4839-a53e-527490180929.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll icu_62::DecimalFormat::setGroupingUsed intl/icu/source/i18n/decimfmt.cpp:350
1 xul.dll static void icu_62::fixNumberFormatForDates intl/icu/source/i18n/smpdtfmt.cpp:257
2 xul.dll void icu_62::SimpleDateFormat::initialize intl/icu/source/i18n/smpdtfmt.cpp:864
3 xul.dll void icu_62::SimpleDateFormat::construct intl/icu/source/i18n/smpdtfmt.cpp:839
4 xul.dll icu_62::SimpleDateFormat::SimpleDateFormat intl/icu/source/i18n/smpdtfmt.cpp:499
5 xul.dll static class icu_62::DateFormat* icu_62::DateFormat::create intl/icu/source/i18n/datefmt.cpp:526
6 xul.dll icu_62::DateFormat::createDateInstance intl/icu/source/i18n/datefmt.cpp:402
7 xul.dll void icu_62::DateTimePatternGenerator::addICUPatterns intl/icu/source/i18n/dtptngen.cpp:664
8 xul.dll void icu_62::DateTimePatternGenerator::initData intl/icu/source/i18n/dtptngen.cpp:480
9 xul.dll void icu_62::DateTimePatternGenerator::DateTimePatternGenerator intl/icu/source/i18n/dtptngen.cpp:350

=============================================================

this is a cross-platform but low volume crash signature appearing in firefox 63, apparently related to the icu update in bug 1466471.
these crashes are mostly taking place in the content crashes and skew towards 32bit installations.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Andre, can you please take a look at this?

Comment 2

[André Bargull [:anba]]

ecx is 0x00000000, so most likely a null-pointer crash; probably the OOM issue reported at https://unicode-org.atlassian.net/browse/ICU-13847.

Comment 3

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Is there anything actionable here for 64/65?

Comment 4

[Ryan VanderMeulen [:RyanVM]]

The upstream ticket doesn't seem to be going anywhere. Is there any chance we could hack around this on our end for now?

Comment 5

[André Bargull [:anba]]

To make this reproducible for me, I've built a shell with JS_SetICUMemoryFunctions setting ICU's allocation functions to use js_malloc and friends, which allows to create an oomTest function test case:

oomTest(function() {
    for (var i = 0; i < 10; ++i) {
        new Intl.DateTimeFormat("de").format(0)
    }
})

And I got the crash from comment #0 right away. \o/

Added some fixes to catch the case when fields is nullptr when created from here and that should be it, right? Rebuild ICU and reran the oomTest test to ensure the newly added workarounds did indeed fix this crash. Nope, next crash happened, now fields->formatter created here is nullptr. Added more workarounds, which, by the way, involved making the DecimalFormat class non-functional, because not all users of fields->formatter have a way to report an error. And again, rebuild ICU and reran the oomTest. Next crash because Calendar::fZone created here is nullptr, but all Calendar users expect this field is never null.

After that crash I stopped playing whack-a-mole with ICU. /o\

So I don't think we can (successfully) hack around this without changing dozens of places in ICU. :-/

Comment 6

[Ryan VanderMeulen [:RyanVM]]

OK, thanks for trying :(. Doesn't sound like this bug is actionable until upstream gets around to fixing it.

Comment 7

[Jeff Walden [:Waldo]]

I've looked into this sort of problem before -- André could fill people in on the details if it mattered -- and reached the same conclusion about lots of places in ICU requiring a fix, and that fix being too complicated to take anywhere but upstream first.

I don't think this is so critical as to prioritize over other work, but we always could if OOM issues that will mostly manifest on 32-bit are worth caring about super-strongly. It's hard for me to see why we would think that.

Comment 8

[André Bargull [:anba]]

Filed https://unicode-org.atlassian.net/browse/ICU-20366, https://unicode-org.atlassian.net/browse/ICU-20367, and https://unicode-org.atlassian.net/browse/ICU-20368.

Comment 9

[Timea Cernea [:tbabos][inactive]]

There was a very low volume of crashes in the last 6 months and all of them happened on ~FX67, not the latest Firefox versions.
Closing this as resolved:worksforme, please do re-open it if this crash will appear on the latest version of Firefox.