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

RESOLVED FIXED in mozilla42

Status

()

--
critical
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: decoder, Assigned: Waldo)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla42
assertion, testcase
Points:
---

Firefox Tracking Flags

(firefox37 affected)

Details

(Whiteboard: [jsbugmon:update,origRev=b17e7747d3fb,testComment=12,ignore])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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");
});
(Reporter)

Comment 1

5 years ago
Created attachment 799754 [details]
[crash-signature] Machine-readable crash signature
Looks like an assertion failure in self-hosted Intl code:

Self-hosted JavaScript assertion info: "BestAvailableLocale"
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 5

5 years ago
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
Whiteboard: [leave open]
(Reporter)

Updated

5 years ago
Whiteboard: [leave open] → [leave open][jsbugmon:update]
(Reporter)

Updated

5 years ago
Whiteboard: [leave open][jsbugmon:update] → [leave open] [jsbugmon:update,ignore]
(Reporter)

Comment 7

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

Updated

5 years ago
Whiteboard: [leave open] [jsbugmon:update,ignore] → [leave open] [jsbugmon:bisectfix]
(Reporter)

Updated

5 years ago
Whiteboard: [leave open] [jsbugmon:bisectfix] → [leave open] [jsbugmon:]
(Reporter)

Comment 8

5 years ago
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.
Waldo, is bug 919872 a possible fix? (NumberFormat seems involved both ways)
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 10

5 years ago
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);
Flags: needinfo?(jwalden+bmo)
(Reporter)

Updated

5 years ago
Whiteboard: [leave open] [jsbugmon:] → [leave open] [jsbugmon:update,origRev=e3785e299ab6,testComment=10]
(Reporter)

Updated

5 years ago
Whiteboard: [leave open] [jsbugmon:update,origRev=e3785e299ab6,testComment=10] → [leave open] [jsbugmon:update,origRev=e3785e299ab6,testComment=10,ignore]
(Reporter)

Comment 11

5 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision f2adb62d07eb).
Assignee: general → nobody
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
status-firefox37: --- → affected
Whiteboard: [leave open] [jsbugmon:update,origRev=e3785e299ab6,testComment=10,ignore] → [leave open] [jsbugmon:update,origRev=b17e7747d3fb,testComment=12]
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1109966
(Reporter)

Updated

3 years ago
Whiteboard: [leave open] [jsbugmon:update,origRev=b17e7747d3fb,testComment=12] → [leave open] [jsbugmon:update,origRev=b17e7747d3fb,testComment=12,ignore]
(Reporter)

Comment 14

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

Updated

3 years ago
Component: JavaScript Engine → JavaScript: Internationalization API
(Assignee)

Updated

3 years ago
Summary: Assertion failure: false, at vm/SelfHosting.cpp:201 → Handling of Unicode Locale Extension Sequences in Intl.js incorrect, leading to assertions
(Assignee)

Updated

3 years ago
Duplicate of this bug: 839372
(Assignee)

Comment 16

3 years ago
Created attachment 8627005 [details] [diff] [review]
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.
Attachment #8627005 - Flags: review?(andrebargull)
(Assignee)

Updated

3 years ago
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
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. :-/
Attachment #8627005 - Flags: review?(andrebargull) → review+
(Assignee)

Comment 20

3 years ago
Oops, this had a stray leave-open on it.  We're done here.
OS: Linux → All
Hardware: x86 → All
Whiteboard: [leave open] [jsbugmon:update,origRev=b17e7747d3fb,testComment=12,ignore] → [jsbugmon:update,origRev=b17e7747d3fb,testComment=12,ignore]
Target Milestone: --- → mozilla41
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Target Milestone: mozilla41 → mozilla42
You need to log in before you can comment on or make changes to this bug.