Closed
Bug 752223
Opened 12 years ago
Closed 12 years ago
NumberValue/DoubleValue are footguns
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: bzbarsky, Assigned: efaust)
References
()
Details
(Whiteboard: [js:p2:fx17][js:bumped:1])
Attachments
(1 file)
30.91 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
Oh, and this is keeping new bindings on JS_NewNumberValue for now.
Comment 2•12 years ago
|
||
Given that JS_NewNumberValue is no longer fallible, we could just change/rename it to jsval JS_NumberValue(double d);
Reporter | ||
Comment 3•12 years ago
|
||
and inline? ;)
Comment 4•12 years ago
|
||
I was hoping for a JS_NEVER_INLINE, JS_PUBLIC_API, CrossCompartmentWrapper::call, but I guess that's fine too.
Comment 5•12 years ago
|
||
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?
Reporter | ||
Comment 6•12 years ago
|
||
Runtime-wide, pah. You need a process-wide lock, under which you do network I/O!
Comment 7•12 years ago
|
||
Oh, right, you need e10s to load an SVG 1.2 document to do the socket access. How could I have forgotten that?
Updated•12 years ago
|
Updated•12 years ago
|
Whiteboard: [js:p2:fx16][js:ni]
Updated•12 years ago
|
Whiteboard: [js:p2:fx16][js:ni] → [js:p2:fx17][js:bumped:1][js:ni]
Reporter | ||
Comment 8•12 years ago
|
||
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•12 years ago
|
||
Comment 10•12 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•12 years ago
|
||
On try after hoist and fixing linker bustage at https://tbpl.mozilla.org/?tree=Try&rev=9ed00d0fe3da
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/70d749a0e1ff
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/70d749a0e1ff
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Updated•12 years ago
|
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.
Description
•