Bug 4026 – Math.random() not seeded

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Finlay Dobbie]

Math.random() appears to be unseeded, and uses the rand() function which is known to perform poorly on 
OS X (IIRC, anyway).

A partial workaround is to srand(time(NULL)) from the host application.

[Finlay Dobbie]

*** Bug 4027 has been marked as a duplicate of this bug. ***

[Geoffrey Garen]

Created attachment 5294 [details]
Fix

FYI, rand() is the *faster* of the random number generators on OS X. (random()
makes the cryptography gods happier, but I don't think we need to worry about
anyone using JavaScript for that...)

[Maciej Stachowiak]

Comment on attachment 5294 [details]
Fix

Is the :: business really needed here? Also, mightn't you want a time value
that has more than second precision? But otherwise, r=me looks like a good
improvement.

[Darin Adler]

Comment on attachment 5294 [details]
Fix

Is it really a good thing for JavaScript to change the global seed used by the
process it's hosted in?

Also, why not use sranddev() instead of srand(time(0))?

And why not switch to random(), at least on OS X? If we used that, we could use
initstate() and setstate() and make the random number sequence for
JavaScriptCore be entirely separate from the random number sequence used by
other callers inside the process.

[Geoffrey Garen]

Created attachment 5304 [details]
Fix 2

Attempt to address some of the comments above.

[Geoffrey Garen]

Comment on attachment 5304 [details]
Fix 2

patch to get rid of ::, use sranddev(), but not use random().

[Geoffrey Garen]

In addition to non-portability, I didn't want to use random() because it's advertised as being slower than 
rand(), and random number generation is a part of the ibench test suite.

One option to avoid trampling the parent process would be to test for rand() == 16807, which is the first 
value returned by an unseeded rand(). I doubt that's portable, though.

[Maciej Stachowiak]

Trampling the parent process is unfortunate but seems unavoidable without a portable, efficient API to do 
it otherwise.

[Maciej Stachowiak]

Comment on attachment 5304 [details]
Fix 2

Actually, now that I look more closely - shouldn't randomSeeded be a static
variable, rather than a field of MathFunctionImp? As it is, every interpreter
will reseed the RNG on the first call to Math.random, that doesn't seem to be
what we want.

[Geoffrey Garen]

Created attachment 5325 [details]
Fix 3

Use a static class boolean to track seeding the RNG.

I think globals get initialized to 0 by default, so the initializer may be
overkill. Can't hurt, I suppose.

[Justin Haygood]

For Windows, we can use rand_s(), which uses the system generated seed. This
will let the 2 OSes have the best OS specific optimizations available.

rand_s is the new Whidbey CRT (the C Runtime Environment used by Visual Studio
2005) that uses Windows XP's (and Windows Vista's) cryptographically secure APIs
for generating random numbers and uses CryptoAPI on older platforms, automagically.

Would this be a suitable #ifdef?

[Darin Adler]

Comment on attachment 5325 [details]
Fix 3

The page load test intentionally seeds the random number generator the same way
every time to give repeatable results. This patch is going to break that
feature.

[Maciej Stachowiak]

Comment on attachment 5325 [details]
Fix 3

r=me, to address Darin's concern, the PLT could do what it does after ensuring
that at least one random number has  been generated, since this latest patch
promises one time only seeding.

Class scope statics do need to be initialized, but you could make it file
scope, then there is no need to mention it in the header. It is better to leave
implementation details out of headers when possible IMO.

[Geoffrey Garen]

Landed. Also sent a patch for review to address the PLT issue.