Closed Bug 1484943 Opened Last year Closed Last year

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

Categories

(Core :: JavaScript: Internationalization API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: anba, Assigned: Waldo)

References

Details

Attachments

(1 file, 1 obsolete file)

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
---
Flags: needinfo?(jwalden+bmo)
Oh bah, I forgot about that.  Easy and obvious fix, will rs= a landing.
Flags: needinfo?(jwalden+bmo)
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.
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.
Attachment #9002897 - Flags: review?(andrebargull)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Couldn't we simply canonicalise NaN before calling ICU in PartitionNumberPattern? Similar to how we changed -0.0 to +0.0 before bug 1483374.
Oh hrm, maybe.  Lemme try that.
Attached patch PatchSplinter Review
Out of the frying pan and into the fire.
Attachment #9002939 - Flags: review?(andrebargull)
Attachment #9002897 - Attachment is obsolete: true
Attachment #9002897 - Flags: review?(andrebargull)
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?
Attachment #9002939 - Flags: review?(andrebargull) → review+
Pushed by jwalden@mit.edu:
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
https://hg.mozilla.org/mozilla-central/rev/2de8069003dc
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.