The default bug view has changed. See this FAQ.

NumberValue/DoubleValue are footguns

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: efaust)

Tracking

unspecified
mozilla17
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:p2:fx17][js:bumped:1], URL)

Attachments

(1 attachment)

They don't canonicalize NaN, so it's easy for consumers to screw up if they end up with a NaN somehow.  And if I understand right, such a screwup could lead to security bugs...

I'd love an API that has the ergonomics of NumberValue()/DoubleValue() (including the inlining, ideally!) but the safety of JS_NewNumberValue.
Blocks: 752224
Oh, and this is keeping new bindings on JS_NewNumberValue for now.

Comment 2

5 years ago
Given that JS_NewNumberValue is no longer fallible, we could just change/rename it to

  jsval JS_NumberValue(double d);
and inline?  ;)

Comment 4

5 years ago
I was hoping for a JS_NEVER_INLINE, JS_PUBLIC_API, CrossCompartmentWrapper::call, but I guess that's fine too.
Don't we need a runtime-wide lock to make sure the memory location isn't being changed by another thread or by incremental GC?
Runtime-wide, pah.  You need a process-wide lock, under which you do network I/O!
Oh, right, you need e10s to load an SVG 1.2 document to do the socket access.  How could I have forgotten that?
Whiteboard: [js:p2:fx16][js:ni]
Whiteboard: [js:p2:fx16][js:ni] → [js:p2:fx17][js:bumped:1][js:ni]
So... DOUBLE_TO_JSVAL is exactly the API I want, actually.  ;)  Except it doesn't produce int values for ints, of course.
(Assignee)

Comment 9

5 years ago
Created attachment 647798 [details] [diff] [review]
Implement JS_NumberValue()
Assignee: general → efaust
Status: NEW → ASSIGNED
Attachment #647798 - Flags: review?(luke)

Comment 10

5 years ago
Comment on attachment 647798 [details] [diff] [review]
Implement JS_NumberValue()

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

::: js/src/jsapi.h
@@ +2241,5 @@
> +JS_ALWAYS_INLINE JS_PUBLIC_API(jsval)
> +JS_NumberValue(double d)
> +{
> +    d = JS_CANONICALIZE_NAN(d);
> +    int32_t i;

Since this is technically C code, you'll need to hoist this decl.  (Building js/jsd should have caught this; I recommend doing a try build to make sure you've caught it all.)
Attachment #647798 - Flags: review?(luke) → review+
(Assignee)

Comment 11

5 years ago
On try after hoist and fixing linker bustage at https://tbpl.mozilla.org/?tree=Try&rev=9ed00d0fe3da
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/70d749a0e1ff
https://hg.mozilla.org/mozilla-central/rev/70d749a0e1ff
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Whiteboard: [js:p2:fx17][js:bumped:1][js:ni] → [js:p2:fx17][js:bumped:1]
You need to log in before you can comment on or make changes to this bug.