1484943 - Assertion failure: !IsNaN(aValue) (NaN does not have a sign)

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[André Bargull [:anba]]

Test case:
---
new Intl.NumberFormat("en", {}).formatToParts(-NaN)
---


Asserts with:
---
Assertion failure: !IsNaN(aValue) (NaN does not have a sign), at /home/andre/hg/mozilla-inbound/js/src/build-debug-opt-obj/dist/include/mozilla/FloatingPoint.h:171
---

Comment 1

[Jeff Walden [:Waldo]]

Oh bah, I forgot about that.  Easy and obvious fix, will rs= a landing.

Comment 2

[Jeff Walden [:Waldo]]

Actually, wait.  Something is whack here.  Why should there be a sign-part in the representation of NaN, as exposed by ICU?

I'll rs= a hackaround for now to fix a crash, but this looks like maybe it's an ICU issue too, and that's not going to be a simple fix.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Don't assert when trying to formatToParts a NaN value whose sign bit is set

Well, maybe on second thought a review is a good idea, since fundamentally we're working around an ICU bug and have to have comments that explain and tests that exercise this.

Comment 4

[André Bargull [:anba]]

Couldn't we simply canonicalise NaN before calling ICU in PartitionNumberPattern? Similar to how we changed -0.0 to +0.0 before bug 1483374.

Comment 5

[Jeff Walden [:Waldo]]

Oh hrm, maybe.  Lemme try that.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Patch

Out of the frying pan and into the fire.

Comment 7

[André Bargull [:anba]]

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

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

LGTM!

::: js/src/builtin/intl/NumberFormat.cpp
@@ +441,5 @@
> +            // formatToParts must per spec agree on the overall formatted
> +            // string -- and there's no guarantee that simply removing the
> +            // field is correct for all locales.)
> +            return &JSAtomState::minusSign;
> +        }

Is this part still needed or can we now change it to an assertion?

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2de8069003dc
Don't assert when trying to formatToParts a NaN value whose sign bit is set.  r=anba

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/2de8069003dc