Expose crypto.getRandomValues() to workers.

Expose crypto.getRandomValues() to workers.

It is only available to workers when the "Crypto" runtime feature is on.

-----------------------
Testing details
-----------------------

  * Duplicated all existing tests for crypto.getRandomValues(). In addition to the original flavor, there is now a version of the test that runs on a worker.

  * Removed the 'crypto-' prefix from existing tests. This was redundant, as the path already includes 'crypto/'.

  * Added a completely new test: 'worker-random-values-concurrent.html'. This makes sure that calling getRandomValues() concurrently on workers isn't a problem.

BUG=245025, 142173
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152389

[Ryan Sleevi, 2013-06-12 21:57:15]

https://codereview.chromium.org/16820007/diff/1/Source/modules/crypto/RandomS...
File Source/modules/crypto/RandomSource.idl (right):

https://codereview.chromium.org/16820007/diff/1/Source/modules/crypto/RandomS...
Source/modules/crypto/RandomSource.idl:36: // Web Crypto API spec.
comment updates needed

[eroman, 2013-06-12 22:00:00]

https://codereview.chromium.org/16820007/diff/1/Source/modules/crypto/RandomS...
File Source/modules/crypto/RandomSource.idl (right):

https://codereview.chromium.org/16820007/diff/1/Source/modules/crypto/RandomS...
Source/modules/crypto/RandomSource.idl:36: // Web Crypto API spec.
On 2013/06/12 21:57:15, Ryan Sleevi wrote:
> comment updates needed

I will move some version of this comment to Crypto.idl instead (since that is
where it uses the flag)

[abarth-chromium, 2013-06-12 23:18:16]

This CL looks good.  My one concern is whether you intend the change below.  In
either case, we should add a test to make sure we have the proper behavior.

https://codereview.chromium.org/16820007/diff/1/Source/modules/crypto/Crypto.idl
File Source/modules/crypto/Crypto.idl (right):

https://codereview.chromium.org/16820007/diff/1/Source/modules/crypto/Crypto....
Source/modules/crypto/Crypto.idl:31: ] interface Crypto : RandomSource {
Is this how this works in the spec?  This change is observable in that
getRandomValues is not on window.crypto.__proto__ rather than on window.crypto
directly.

[eroman, 2013-06-12 23:33:32]

Thanks Adam, this CL is still a work in progress (hadn't intended to publish it
yet) :)

I wasn't aware of the window.crypto.__proto__, I will look into it. What
approach do you recommend?

[abarth-chromium, 2013-06-12 23:39:57]

On 2013/06/12 23:33:32, eroman wrote:
> Thanks Adam, this CL is still a work in progress (hadn't intended to publish
it
> yet) :)

Oh, sorry!

> I wasn't aware of the window.crypto.__proto__, I will look into it. What
> approach do you recommend?

If I remember correctly from the spec, this uses a WebIDL idiom that we don't
implement yet.  Normally I would say we should just copy/paste the one line of
IDL, but it's not as easy here because there's a custom binding that we don't
want to copy/paste...

Maybe the expedient thing to do is to copy/paste the IDL declaration and then
have V8Crypto::getRandomValuesMethodCustom call through to
V8RandomSource::getRandomValuesMethodCustom ?

[eroman, 2013-06-13 00:50:23]

Many thanks Adam for pointing out the __proto__ issue!

I have confirmed that my translation was incorrect, and will address this in a
coming patchset.

For posterity sake, and to make sure I am on the right page, here is a summary
of the problem (no response necessary):

The WebCrypto spec defines:
  Crypto implements RandomSource;
  WorkerCrypto implements RandomSource;

This is using "implements" and NOT "inheritance". The two are in fact different,
as documented by the WebIDL spec. [1] vs [2]

This difference can be observed from Javascript by looking at the __proto__
chain:

  crypto.__proto__.hasOwnProperty('getRandomValues')

In this test will return 'true', since getRandomValues() is defined directly on
'crypto'.

However in my patchset, the test would have changed the result to 'false', since
'getRandomValues()' is now defined in the __proto__ for RandomSource. Which is
in fact NOT the intent of the WebCrypto spec (also confirmed with rsleevi).

Per the recommendation, I will be adding this to the test suite.

Again, thanks Adam for the speedy and helpful reviews! I am new to blink and
greatly appreciate these sort of comments.

[1] http://www.w3.org/TR/2012/CR-WebIDL-20120419/#idl-implements-statements

[2] http://www.w3.org/TR/2012/CR-WebIDL-20120419/#dfn-inherit

[eroman, 2013-06-13 21:39:00]

PTAL.

 * Removed RandomSource.idl and the interface inheritance. (Instead introduced
WorkerCrypto).

 * Added 'worker-random-values-concurrent.html' test

 * Added a test to make sure getRandomValues() is defined directly on crypto
(and not something deeper in the __proto__ chain).

[abarth-chromium, 2013-06-13 21:48:33]

lgtm

https://codereview.chromium.org/16820007/diff/20001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8CryptoCustom.cpp (right):

https://codereview.chromium.org/16820007/diff/20001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8CryptoCustom.cpp:40: //   * Must be threadsafe
It's too bad the code generator isn't smart enough to generate this code.

[commit-bot: I haz the power, 2013-06-13 21:53:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/16820007/20001

[commit-bot: I haz the power, 2013-06-13 23:17:14]

Change committed as 152389