Bug 22876 – Should centralize random number handling for JavaScriptCore & WebCore

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Random number handling functionality is duplicated, once in wtf/MathExtras.h (wtf_random / wtf_random_init) and once in HTMLFormElement::randomNumber().

[Nikolas Zimmermann]

Created attachment 26045 [details]
Patch for JavaScriptCore/

[Nikolas Zimmermann]

Created attachment 26046 [details]
Patch for WebCore/

[Nikolas Zimmermann]

Created attachment 26047 [details]
Updated patch for JavaScriptCore

Fixed some stray tabs.

[Nikolas Zimmermann]

Created attachment 26048 [details]
Updated patch for WebCore

Accidently included some unwanted bits in the patch, remove them.

[Darin Adler]

Comment on attachment 26047 [details]
Updated patch for JavaScriptCore

A single patch for both JavaScriptCore and WebCore would be better.

There are two changes here. One is that the random number generator gets initialized differently. The other is that the functions have new names and a new header. I would have preferred to see those changes done independently so we could separately discuss the merits of each.

I think it's a slightly tricky to initialize this correctly. As I understand it, it's an error to call initializeRandomNumberGenerator at all when JSC_MULTIPLE_THREADS  is off and also an error for anyone besides initializeThreads to call it. I think it would be good to make it harder to use wrong. For example, the definition in the header could be inside #if ENABLE(JSC_MULTIPLE_THREADS), or the initialization function should be in its own header -- people who are using the random number generator don't need to know how the initialization works. And we could have some debugging code to assert that it's called correctly. Generally we want functions that are hard to "use wrong", and I think any calls to initializeRandomNumberGenerator are likely to be wrong.

I'm not overjoyed with this, but I'll still say r=me

[Darin Adler]

Comment on attachment 26048 [details]
Updated patch for WebCore

> -#if PLATFORM(QT)
> -#include <QtCore/QFileInfo>
> -#endif

This change isn't mentioned anywhere. You should mention it in the header if there's a good reason for it.

> -#if PLATFORM(DARWIN)
> -    if (!randomSeeded) {
> -        srandomdev();
> -        randomSeeded = true;
> -    }
> -    return random();
> -#else

The srandomdev() technique for initializing the random number generator is stronger than the srand(time(0)) technique, and random() is better than rand(), so the version in JavaScriptCore should be changed to use that under DARWIN. We may want to improve the random number generator further soon due to some feedback I got from some other folks at Apple.

> -        int randomness = randomNumber();
> +        int randomness = static_cast<int>(WTF::randomNumber() * INT_MAX);

This won't work quite as well as the old code. It doesn't cover the entire integer range. We want numbers in the range [INT_MIN, INT_MAX], but this gives numbers in the range [0, INT_MAX). So we'll never get INT_MAX or any negative number. On the other hand, we only really use 24 of the 32 bits of randomness, so it's probably OK to take this basic approach. But it should be INT_MAX + 1, not INT_MAX, that we multiply by. Of course INT_MAX + 1 won't fit into an int, so it's a little tricky to write the code correctly. Something like this is what I'd suggest:

    unsigned randomness = static_cast<unsigned>(randomNumber() * (numeric_limits<unsigned>::max() + 1.0));

I'm going to say review- because I think it's important not to downgrade from random() to rand() on DARWIN.

[Nikolas Zimmermann]

Hey Darin,

thanks for the quick review.
 
> There are two changes here. One is that the random number generator gets
> initialized differently. The other is that the functions have new names and a
> new header. I would have preferred to see those changes done independently so
> we could separately discuss the merits of each.
Oh, okay - I'll take that in mind in future.
 
> I think it's a slightly tricky to initialize this correctly. As I understand
> it, it's an error to call initializeRandomNumberGenerator at all when
> JSC_MULTIPLE_THREADS  is off and also an error for anyone besides
> initializeThreads to call it. I think it would be good to make it harder to use
> wrong.
It's indeed a bit tricky: the idea is that if we're building with ENABLE(JSC_MULTIPLTE_THREADS) that the Threading*.cpp functions call "initializeRandomNumberGenerator" once on thread initialization.

If we're not building with JSC_MULTIPLE_THREADS, then randomNumber() need to track wheter it already seeded the random number generator or not.

So wrapping the initializeRandomNumberGenerator() function in ENABLE(JSC_MULTIPLE_THREADS) is not going to work. What I could do is _restore_ wtf_random_init() in MathExtras.h and use that from randomNumber() and from Threading*...

This way RandomNumber.h would only need to contain the 'randomNumber()' function - which is slightly less confusing than the current solution.

> For example, the definition in the header could be inside #if
> ENABLE(JSC_MULTIPLE_THREADS), or the initialization function should be in its
> own header -- people who are using the random number generator don't need to
> know how the initialization works. And we could have some debugging code to
> assert that it's called correctly. Generally we want functions that are hard to
> "use wrong", and I think any calls to initializeRandomNumberGenerator are
> likely to be wrong.
Okay, do you think my approach above would solve it?

Niko

[Nikolas Zimmermann]

(In reply to comment #6)
> (From update of attachment 26048 [details] [review])
> > -#if PLATFORM(QT)
> > -#include <QtCore/QFileInfo>
> > -#endif
> 
> This change isn't mentioned anywhere. You should mention it in the header if
> there's a good reason for it.
Oh there was no Qt usage at all in this file, so I just killed this include.
 
> > -#if PLATFORM(DARWIN)
> > -    if (!randomSeeded) {
> > -        srandomdev();
> > -        randomSeeded = true;
> > -    }
> > -    return random();
> > -#else
> 
> The srandomdev() technique for initializing the random number generator is
> stronger than the srand(time(0)) technique, and random() is better than rand(),
> so the version in JavaScriptCore should be changed to use that under DARWIN. We
> may want to improve the random number generator further soon due to some
> feedback I got from some other folks at Apple.
Okay, I can add another special case in the JSC patch.
 
> > -        int randomness = randomNumber();
> > +        int randomness = static_cast<int>(WTF::randomNumber() * INT_MAX);
> 
> This won't work quite as well as the old code. It doesn't cover the entire
> integer range. We want numbers in the range [INT_MIN, INT_MAX], but this gives
> numbers in the range [0, INT_MAX). So we'll never get INT_MAX or any negative
> number. On the other hand, we only really use 24 of the 32 bits of randomness,
> so it's probably OK to take this basic approach. But it should be INT_MAX + 1,
> not INT_MAX, that we multiply by. Of course INT_MAX + 1 won't fit into an int,
> so it's a little tricky to write the code correctly. Something like this is
> what I'd suggest:
> 
>     unsigned randomness = static_cast<unsigned>(randomNumber() *
> (numeric_limits<unsigned>::max() + 1.0));

Oops, I shouldn't post patches at 7am. Good catch!
 
> I'm going to say review- because I think it's important not to downgrade from
> random() to rand() on DARWIN.

Ok, I'll wait for your final comments, and come up with a new combined WebCore/JSC patch.
Thanks for the review.

[Nikolas Zimmermann]

Created attachment 26062 [details]
Updated combined patch v2

[Darin Adler]

Comment on attachment 26062 [details]
Updated combined patch v2

r=me

[Nikolas Zimmermann]

Landed in r39337.