912701 - Handling of Unicode Locale Extension Sequences in Intl.js incorrect, leading to assertions

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision e3785e299ab6 (run with --fuzzing-safe --ion-eager):


var validValues = [{}];
validValues.forEach(function (value) {
            return new Intl.NumberFormat("x" + "-u-cu-krw");
});

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Jan de Mooij [:jandem]]

Looks like an assertion failure in self-hosted Intl code:

Self-hosted JavaScript assertion info: "BestAvailableLocale"

Comment 3

[Jeff Walden [:Waldo]]

This doesn't crash for me when I try it.  Nothing super-obviously wrong shows up on cursory code inspection, either.  I'll look into this more soon and see if I can't get it to reproduce -- likely error on my end, since it sounds (and looks) straightforward.

Comment 4

[Jeff Walden [:Waldo]]

Oops, I think I ran this with an opt shell, because it reproduces with a debug build now.

Basically, removeUnicodeExtensions is removing what it thinks are Unicode locale extension sequences, but because those sequences are in a privateuse section, they can't actually be removed.  And because in this case the entire tag is a privateuse, the result is "removing" these "Unicode sequences" produces the tag "x": obviously not a tag.

Here's a shorter testcase:

js> Intl.NumberFormat("x-u-foo")
Self-hosted JavaScript assertion info: "Invalid BestAvailableLocale locale structure: x"
Assertion failure: false, at /home/jwalden/moz/slots/js/src/vm/SelfHosting.cpp:201
Segmentation fault (core dumped)

As to the consequences of this, outside debug builds: every use of removeUnicodeExtensions is to create a string/undefined to pass to BestAvailableLocales.  "x" isn't an available locale, and there's no "-" in it, so BestAvailableLocales will always return |undefined|.  That value percolates into all the callers, who already sensibly handle it.  So the only problem here is a failed assertion, and possibly wrong results, but not truly bad behavior.

This could probably be solved by throwing more regular expressions at the problem, of course.  Or maybe it could be solved by stopping the removeUnicodeExtensions loop when the string starts with "x-".  I'm not 100% sure offhand, I'd have to check the ABNF.  It does seem to me that we're bumping up against the limits of what regular expressions should be sensibly used to do, in any case.

Comment 5

[Jeff Walden [:Waldo]]

I elaborated the assertion messages slightly to help diagnose this, figured I might as well land them for general use, now and in the future:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4db5b4909059

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4db5b4909059

Comment 7

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 2c85e4d1d678).

Comment 8

[Christian Holler (:decoder)]

JSBugMon: Fix Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/1d6b119b7281
user:        Jeff Walden
date:        Fri Sep 27 16:07:00 2013 -0700
summary:     Bug 919872 - Compute the internal properties of Collator, NumberFormat, and DateTimeFormat objects only when they're actually needed -- not when the objects are constructed.  r=till

This iteration took 365.356 seconds to run.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Waldo, is bug 919872 a possible fix? (NumberFormat seems involved both ways)

Comment 10

[Jeff Walden [:Waldo]]

The patch there just puts off the day of reckoning, by making internals-resolution lazy.  This testcase no longer crashes, sure:

  Intl.NumberFormat("x-u-foo");

But this one, that forces resolution, does:

  Intl.NumberFormat("x-u-foo").format(5);

Comment 11

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision f2adb62d07eb).

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Intl.NumberFormat("x-u-foo").format(5);

asserts js debug shell on m-c rev b17e7747d3fb with --fuzzing-safe --no-threads --no-ion at Assertion failure: false, at vm/SelfHosting.cpp

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Comment 14

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 767ea640b2f1).

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Patch

Really, we need to not handle locales using regexes, because this is madness.  But that's a large rewrite for not a whole lot of gain, and not yet truly worth it, IMO, so do whatever works.

Comment 17

[André Bargull [:anba]]

Comment on attachment 8627005 [details] [diff] [review]
Patch

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

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> Really, we need to not handle locales using regexes, because this is
> madness. 

Definitely. For example it took me some time to understand why the "removeUnicodeExtensions" function contains a while-loop, until I remembered there's also bug 866596. :-/

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88e918fce0e3

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/88e918fce0e3

Comment 20

[Jeff Walden [:Waldo]]

Oops, this had a stray leave-open on it.  We're done here.