Bug 4026 - Math.random() not seeded
Summary: Math.random() not seeded
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 412
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
: 4027 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-07-16 04:51 PDT by Finlay Dobbie
Modified: 2005-12-29 03:07 PST (History)
0 users

See Also:


Attachments
Fix (2.61 KB, patch)
2005-12-26 20:23 PST, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Fix 2 (2.58 KB, patch)
2005-12-27 11:45 PST, Geoffrey Garen
mjs: review-
Details | Formatted Diff | Diff
Fix 3 (1.75 KB, patch)
2005-12-27 22:30 PST, Geoffrey Garen
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Finlay Dobbie 2005-07-16 04:51:53 PDT
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.
Comment 1 Finlay Dobbie 2005-07-16 04:53:38 PDT
*** Bug 4027 has been marked as a duplicate of this bug. ***
Comment 2 Geoffrey Garen 2005-12-26 20:23:27 PST
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...)
Comment 3 Maciej Stachowiak 2005-12-27 01:35:27 PST
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.
Comment 4 Darin Adler 2005-12-27 10:29:51 PST
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.
Comment 5 Geoffrey Garen 2005-12-27 11:45:01 PST
Created attachment 5304 [details]
Fix 2

Attempt to address some of the comments above.
Comment 6 Geoffrey Garen 2005-12-27 11:45:49 PST
Comment on attachment 5304 [details]
Fix 2

patch to get rid of ::, use sranddev(), but not use random().
Comment 7 Geoffrey Garen 2005-12-27 11:53:15 PST
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.
Comment 8 Maciej Stachowiak 2005-12-27 14:26:29 PST
Trampling the parent process is unfortunate but seems unavoidable without a portable, efficient API to do 
it otherwise.
Comment 9 Maciej Stachowiak 2005-12-27 14:31:36 PST
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.
Comment 10 Geoffrey Garen 2005-12-27 22:30:47 PST
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.
Comment 11 Justin Haygood 2005-12-28 10:37:00 PST
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?
Comment 12 Darin Adler 2005-12-28 10:42:01 PST
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.
Comment 13 Maciej Stachowiak 2005-12-28 16:36:22 PST
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.
Comment 14 Geoffrey Garen 2005-12-29 03:07:48 PST
Landed. Also sent a patch for review to address the PLT issue.