Bug 157805

Summary: REGRESSION(r192855): Math.random() always produces the same first 7 decimal points the first two invocations
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: JavaScriptCoreAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, cdumez, cmarcelo, commit-queue, fpizlo, ggaren, joepeck, mark.lam, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
[PATCH] Proposed Fix none

Joseph Pecoraro
Reported 2016-05-17 11:48:28 PDT
* SUMMARY Math.random() always produces the same first 7 decimal points the first two invocations * STEPS TO REPRODUCE 1. Inspect about:blank 2. Run Math.random() twice => first 7 decimals are identical * EXAMPLES > Math.random() < 0.39641653952203815 < 0.39641654154667616 > Math.random() < 0.7208129766006761 < 0.7208129575360934 * NOTES Seen in a comment on Hacker News: <https://news.ycombinator.com/item?id=11711912> > 2) Math.random() returns the same result the first two calls on page load.
Attachments
[PATCH] Proposed Fix (4.60 KB, patch)
2016-05-17 14:47 PDT, Joseph Pecoraro
no flags
Filip Pizlo
Comment 1 2016-05-17 11:50:06 PDT
Wow, that's hilarious.
Joseph Pecoraro
Comment 2 2016-05-17 12:01:18 PDT
Geoffrey Garen
Comment 3 2016-05-17 12:01:46 PDT
Probably too many zeroes in the initial seed, since our seed API is 32 bit. You can probably fix this by: uint64_t seed64 = seed; m_low = (seed64 << 32) | seed64; m_high = (seed64 << 32) | seed64; Or you could update the seed API to be 128bit.
Radar WebKit Bug Importer
Comment 4 2016-05-17 12:21:48 PDT
Joseph Pecoraro
Comment 5 2016-05-17 13:33:10 PDT
(In reply to comment #3) > Probably too many zeroes in the initial seed, since our seed API is 32 bit. > You can probably fix this by: > > uint64_t seed64 = seed; > m_low = (seed64 << 32) | seed64; > m_high = (seed64 << 32) | seed64; > After this the first 4 digits stayed nearly identical: >>> Math.random() 0.41647092963078913 >>> Math.random() 0.41646612834590113 >>> Math.random() 0.47840231815551537 >>> Math.random() 0.47839187513838377 How about we just advance once after setting the seed? m_low = seed; m_high = seed; advance(); Do you see any disadvantages to this?
Geoffrey Garen
Comment 6 2016-05-17 14:29:59 PDT
> How about we just advance once after setting the seed? > > m_low = seed; > m_high = seed; > advance(); > > Do you see any disadvantages to this? LOL. Brain fart. It's guaranteed that no better solution is possible. We have 128 bits of state that we want to set based on 32 bits of input. All solutions to this problem include some kind of bias unless they are... random!
Joseph Pecoraro
Comment 7 2016-05-17 14:47:44 PDT
Created attachment 279167 [details] [PATCH] Proposed Fix
Joseph Pecoraro
Comment 8 2016-05-17 14:48:51 PDT
An alternative test approach could be TestWebKitAPI creating a WeakRandom, or a jsc API that allows setting the random seed. But this tests Math.random which is web exposed so probably the safest.
Geoffrey Garen
Comment 9 2016-05-17 15:29:31 PDT
Comment on attachment 279167 [details] [PATCH] Proposed Fix EWS failure seems spurious.
WebKit Commit Bot
Comment 10 2016-05-17 15:51:49 PDT
Comment on attachment 279167 [details] [PATCH] Proposed Fix Clearing flags on attachment: 279167 Committed r201053: <http://trac.webkit.org/changeset/201053>
WebKit Commit Bot
Comment 11 2016-05-17 15:51:57 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.