Last Comment Bug 752223 - NumberValue/DoubleValue are footguns
: NumberValue/DoubleValue are footguns
Status: RESOLVED FIXED
[js:p2:fx17][js:bumped:1]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Eric Faust [:efaust]
:
: Jason Orendorff [:jorendorff]
Mentors:
http://www.youtube.com/watch?v=Xe1a1w...
Depends on:
Blocks: 752224
  Show dependency treegraph
 
Reported: 2012-05-05 08:17 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-09-13 12:41 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement JS_NumberValue() (30.91 KB, patch)
2012-07-31 17:58 PDT, Eric Faust [:efaust]
luke: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2012-05-05 08:17:59 PDT
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.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-05-05 08:20:27 PDT
Oh, and this is keeping new bindings on JS_NewNumberValue for now.
Comment 2 Luke Wagner [:luke] 2012-05-05 08:22:45 PDT
Given that JS_NewNumberValue is no longer fallible, we could just change/rename it to

  jsval JS_NumberValue(double d);
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-05-05 08:32:00 PDT
and inline?  ;)
Comment 4 Luke Wagner [:luke] 2012-05-05 10:24:45 PDT
I was hoping for a JS_NEVER_INLINE, JS_PUBLIC_API, CrossCompartmentWrapper::call, but I guess that's fine too.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-05 11:16:33 PDT
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?
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-05-05 11:27:21 PDT
Runtime-wide, pah.  You need a process-wide lock, under which you do network I/O!
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-05 11:33:00 PDT
Oh, right, you need e10s to load an SVG 1.2 document to do the socket access.  How could I have forgotten that?
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-07-30 12:22:03 PDT
So... DOUBLE_TO_JSVAL is exactly the API I want, actually.  ;)  Except it doesn't produce int values for ints, of course.
Comment 9 Eric Faust [:efaust] 2012-07-31 17:58:35 PDT
Created attachment 647798 [details] [diff] [review]
Implement JS_NumberValue()
Comment 10 Luke Wagner [:luke] 2012-07-31 18:07:49 PDT
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.)
Comment 11 Eric Faust [:efaust] 2012-08-01 14:14:44 PDT
On try after hoist and fixing linker bustage at https://tbpl.mozilla.org/?tree=Try&rev=9ed00d0fe3da
Comment 13 Ed Morley [:emorley] 2012-08-02 06:22:21 PDT
https://hg.mozilla.org/mozilla-central/rev/70d749a0e1ff

Note You need to log in before you can comment on or make changes to this bug.