Bug 104845 - window.crypto.getRandomValues should return the input ArrayBufferView
Summary: window.crypto.getRandomValues should return the input ArrayBufferView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ryan Sleevi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-12 14:32 PST by Ryan Sleevi
Modified: 2012-12-20 14:13 PST (History)
14 users (show)

See Also:


Attachments
Patch to fix the IDL and implementation. Does not compile (3.49 KB, patch)
2012-12-12 15:00 PST, Ryan Sleevi
no flags Details | Formatted Diff | Diff
Patch (9.89 KB, patch)
2012-12-12 16:42 PST, Ryan Sleevi
no flags Details | Formatted Diff | Diff
Patch (14.34 KB, patch)
2012-12-12 17:24 PST, Ryan Sleevi
no flags Details | Formatted Diff | Diff
Patch (18.16 KB, patch)
2012-12-12 17:55 PST, Ryan Sleevi
no flags Details | Formatted Diff | Diff
Patch (18.15 KB, patch)
2012-12-12 18:32 PST, Ryan Sleevi
no flags Details | Formatted Diff | Diff
Patch (20.93 KB, patch)
2012-12-19 18:21 PST, Ryan Sleevi
no flags Details | Formatted Diff | Diff
Patch (21.11 KB, patch)
2012-12-19 19:55 PST, Ryan Sleevi
no flags Details | Formatted Diff | Diff
Patch (21.11 KB, patch)
2012-12-19 20:31 PST, Ryan Sleevi
no flags Details | Formatted Diff | Diff
Patch (21.01 KB, patch)
2012-12-20 11:55 PST, Ryan Sleevi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Sleevi 2012-12-12 14:32:54 PST
window.crypto.getRandomValues, as previously specified in WHATWG ( http://wiki.whatwg.org/index.php?title=Crypto&oldid=7945 ) and as presently specified by the W3C Web Crypto WG ( http://www.w3.org/2012/webcrypto/ ) has the following WebIDL for getRandomValues

ArrayBufferView getRandomValues(ArrayBufferView)

This was changed from the previous

void getRandomValues(ArrayBufferView) 

in response to feedback on WHATWG ( http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2012-February/034915.html ) about the proposed interface.

Currently in WebCore, getRandomValues is returning void. It should return the input (modified) ArrayBufferView to allow chaining such as "doStuff(window.crypto.getRandomValues(new Uint8Array(16)));"
Comment 1 Ryan Sleevi 2012-12-12 15:00:13 PST
Created attachment 179131 [details]
Patch to fix the IDL and implementation. Does not compile

The above patch does not (yet) compile, presumably due to html/canvas/ArrayBufferView.idl having the attribute CustomToJSObject and causing the bindings generator to not generate a custom wrap() function.

Guidance on how to fix the bindings would be appreciated, as simply stubbing in a wrap() for ArrayBufferView seems incorrect.

It seems that it might be necessary to do something like the CSSRule bindings, where wrap(ArrayBufferView*, ...) invokes the appropriate wrapper for the derived type (eg: wrap(Uint8Array*, ...) , but that seems like a hierarchy layering violation.
Comment 2 Kentaro Hara 2012-12-12 15:35:02 PST
(In reply to comment #1)
> Created an attachment (id=179131) [details]
> Patch to fix the IDL and implementation. Does not compile
> 
> The above patch does not (yet) compile, presumably due to html/canvas/ArrayBufferView.idl having the attribute CustomToJSObject and causing the bindings generator to not generate a custom wrap() function.
> 
> Guidance on how to fix the bindings would be appreciated, as simply stubbing in a wrap() for ArrayBufferView seems incorrect.
> 
> It seems that it might be necessary to do something like the CSSRule bindings, where wrap(ArrayBufferView*, ...) invokes the appropriate wrapper for the derived type (eg: wrap(Uint8Array*, ...) , but that seems like a hierarchy layering violation.

I think that the basic approach is to implement wrap() somewhere. Would you upload a thorough patch even if it has a compile error? Without concrete build error messages, I can't give you helpful advice. By clicking the "Submit for EWS analysis" button, you can get results from all build bots.
Comment 3 Early Warning System Bot 2012-12-12 15:48:23 PST
Comment on attachment 179131 [details]
Patch to fix the IDL and implementation. Does not compile

Attachment 179131 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15272947
Comment 4 Early Warning System Bot 2012-12-12 15:50:08 PST
Comment on attachment 179131 [details]
Patch to fix the IDL and implementation. Does not compile

Attachment 179131 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15272949
Comment 5 Build Bot 2012-12-12 16:17:09 PST
Comment on attachment 179131 [details]
Patch to fix the IDL and implementation. Does not compile

Attachment 179131 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15312014
Comment 6 WebKit Review Bot 2012-12-12 16:28:09 PST
Comment on attachment 179131 [details]
Patch to fix the IDL and implementation. Does not compile

Attachment 179131 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15296343
Comment 7 Peter Beverloo (cr-android ews) 2012-12-12 16:37:17 PST
Comment on attachment 179131 [details]
Patch to fix the IDL and implementation. Does not compile

Attachment 179131 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15313008
Comment 8 Ryan Sleevi 2012-12-12 16:42:57 PST
Created attachment 179149 [details]
Patch
Comment 9 Early Warning System Bot 2012-12-12 16:58:44 PST
Comment on attachment 179149 [details]
Patch

Attachment 179149 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15319007
Comment 10 Early Warning System Bot 2012-12-12 17:07:51 PST
Comment on attachment 179149 [details]
Patch

Attachment 179149 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15314021
Comment 11 Ryan Sleevi 2012-12-12 17:24:05 PST
Created attachment 179157 [details]
Patch
Comment 12 Ryan Sleevi 2012-12-12 17:55:13 PST
Created attachment 179168 [details]
Patch
Comment 13 Ryan Sleevi 2012-12-12 18:32:02 PST
Created attachment 179175 [details]
Patch
Comment 14 Kentaro Hara 2012-12-17 16:17:37 PST
Comment on attachment 179175 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179175&action=review

> Source/WebCore/ChangeLog:7
> +

Here please describe the rationale for your change.

> Source/WebCore/ChangeLog:17
> +        * GNUmakefile.list.am:
> +        * Target.pri:
> +        * UseJSC.cmake:
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * bindings/js/JSArrayBufferViewCustom.cpp: Added.
> +        (WebCore):
> +        (WebCore::toJS): Added toJS() for ArrayBufferView
> +        * bindings/js/JSBindingsAllInOne.cpp:

I'm not sure why we need to add custom toJS() for JSC. What happens if you change the [CustomToJSObject] to [V8CustomToJSObject] ? Then toJS() for JSC will be auto-generated as expected, won't it?

> Source/WebCore/ChangeLog:18
> +        * bindings/v8/custom/V8ArrayBufferViewCustom.cpp:

What happens if you simply remove [CustomToJSOObject] ? Then the wrap() will be auto-generated as expected, won't it? (I'm not sure. This might not work.)
Comment 15 Kentaro Hara 2012-12-17 16:18:27 PST
Comment on attachment 179175 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179175&action=review

> LayoutTests/security/crypto-random-values-types.html:34
> +function shouldBeType(_a, _type) {
> +  var exception;
> +  var _av;
> +  try {
> +    _av = eval(_a);
> +  } catch (e) {
> +    exception = e;
> +  }
> +
> +  var _typev = eval(_type);
> +  if (_av instanceof _typev) {
> +    testPassed(_a + " is an instance of " + _type);
> +  } else {
> +    testFailed(_a + " is not an instance of " + _type);
> +  }
> +}

You can add this to fast/js/resources/js-test-pre.js.
Comment 16 Ryan Sleevi 2012-12-17 16:50:11 PST
Comment on attachment 179175 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179175&action=review

>> Source/WebCore/ChangeLog:18
>> +        * bindings/v8/custom/V8ArrayBufferViewCustom.cpp:
> 
> What happens if you simply remove [CustomToJSOObject] ? Then the wrap() will be auto-generated as expected, won't it? (I'm not sure. This might not work.)

Yes, but I'm a little nervous about such a change because I don't fully understand it.

However, what I do seem to understand is that the TypedArray subclasses to seem to want to have a custom toJS/toV8. I'm nervous about providing an overload - at all - because it allows new TypedArray subclasses to be introduced incorrectly, by matching this parent ArrayBufferView wrapping, rather than the sub-type specific wrapper generated by the bindings scripts.

In particular, by defining a custom wrap/toJS/toV8, I'm potentially failing to properly define the indexed property indexer ( http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h ). In particular, compare this generic wrap(), which matches the generator's default wrap(), with the wrap() emitted for specific TypedArray subclasses at

http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm#L2716

This is why I'm not convinced this code is entirely correct.
Comment 17 Kentaro Hara 2012-12-17 16:52:50 PST
Thanks, I understood your concern.

kbr: Do you have any advice?
Comment 18 Kenneth Russell 2012-12-17 18:31:30 PST
Comment on attachment 179175 [details]
Patch

Adding a custom toJS/wrap function for ArrayBufferView* is definitely not correct as written. If this wrap() variant were ever called, it would return a useless object to JavaScript, and prevent a wrapper of the correct type from ever being created for that C++ object.

I don't think there any WebGL bindings that do something similar to this function. Calls like getUniform, getVertexAttrib, etc., can end up returning ArrayBufferView subclasses, but they all call the toV8 overload taking a concrete subclass such as Float32Array.

What I would suggest is to turn getRandomValues into a custom binding, and in both the JSC and V8 versions, just return the incoming first argument from the V8 or JSC callback. That way you are actually returning the same value to JavaScript that came in, rather than relying on the DOMDataStore to look up and return the wrapper for the ArrayBufferView C++ object. If this is unpalatable, then doing multiple type-tests and downcasting the ArrayBufferView to the right concrete subclass before calling the autogenerated toJS/toV8 function seems to me like the only other correct solution.
Comment 19 Ryan Sleevi 2012-12-19 18:21:08 PST
Created attachment 180257 [details]
Patch
Comment 20 Kenneth Russell 2012-12-19 18:29:28 PST
Comment on attachment 180257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180257&action=review

Despite the introduction of custom code, this looks much better and safer to me. r=me, but needs a couple of minor updates before committing.

> Source/WebCore/bindings/js/JSCryptoCustom.cpp:14
> + *     * Neither the name of Google Inc. nor the names of its

Wrong form of BSD license. Please use the two-clause license in Source/WebKit/LICENSE.

> Source/WebCore/bindings/v8/custom/V8CryptoCustom.cpp:14
> + *     * Neither the name of Google Inc. nor the names of its

Same license issue.

> LayoutTests/ChangeLog:9
> +        * security/crypto-random-values-types.html:

Please regenerate ChangeLog so that change to js-test-pre.js is included.

> LayoutTests/security/crypto-random-values-types.html:45
> +checkNonIntegerTypes();

Would be nice to test DataView too and ensure that it throws an exception.
Comment 21 Kentaro Hara 2012-12-19 18:37:43 PST
Comment on attachment 180257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180257&action=review

> Source/WebCore/bindings/js/JSCryptoCustom.cpp:50
> +    ArrayBufferView* arrayBufferView = toArrayBufferView(buffer);

Why does it need to be custom binding? I guess you can auto-generate the code by a bit modifying JSValueToNative() in CodeGeneratorJS.pm.

> Source/WebCore/bindings/v8/custom/V8CryptoCustom.cpp:52
> +    ArrayBufferView* arrayBufferView = V8ArrayBufferView::toNative(v8::Handle<v8::Object>::Cast(buffer));

Ditto. I guess you can auto-generate the code by a bit modifying CodeGeneratorV8.pm.
Comment 22 Ryan Sleevi 2012-12-19 18:49:49 PST
Comment on attachment 180257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180257&action=review

>> Source/WebCore/bindings/js/JSCryptoCustom.cpp:50
>> +    ArrayBufferView* arrayBufferView = toArrayBufferView(buffer);
> 
> Why does it need to be custom binding? I guess you can auto-generate the code by a bit modifying JSValueToNative() in CodeGeneratorJS.pm.

Can you please explain what you're proposing?

I have not written Perl before, and while I "suspect" I understand what's going on in CodeGenerator*.pm, that's probably a bit unrealistic.

This is a custom binding because, as shown in the previous patch, there is no 'generic' wrapper-creator for returning ArrayBufferViews - it must always be wrapped through it's more derived type, so that the indexing accessor for the array is created.

kbr explained more about why it would not be proper to call toV8/toJS on a "raw" ArrayBufferView*.

The alternative to this custom binding is to create a dispatch system based on ArrayBufferView::ViewType that downcasts an ArrayBufferView* into its derived type, and then supplies that to the toJS/toV8 call. While this seems to be used for the CSSRule switching, it seems a much more brittle approach and doesn't represent "good" encapsulation between ArrayBufferView and its Derived types.

Unless you're proposing some IDL syntax to be able to indicate "return the argument at position X", I'm not sure what other alternatives exist, so I'm hoping you can clarify.
Comment 23 Ryan Sleevi 2012-12-19 19:01:02 PST
Comment on attachment 180257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180257&action=review

> Source/WebCore/bindings/js/JSCryptoCustom.cpp:62
> +    return buffer;

In case it wasn't clear, *this* line is why it's a custom binding. It's returning a JSValue directly - that is, it does not have to use toJS(arrayBufferView), but rather can directly return the associated JSValue.

> Source/WebCore/bindings/v8/custom/V8CryptoCustom.cpp:62
> +    return buffer;    

Same here - it's returning buffer, not toV8(arrayBufferView)
Comment 24 Ryan Sleevi 2012-12-19 19:55:48 PST
Created attachment 180264 [details]
Patch
Comment 25 Early Warning System Bot 2012-12-19 20:03:05 PST
Comment on attachment 180264 [details]
Patch

Attachment 180264 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15411811
Comment 26 Early Warning System Bot 2012-12-19 20:04:50 PST
Comment on attachment 180264 [details]
Patch

Attachment 180264 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15409842
Comment 27 EFL EWS Bot 2012-12-19 20:05:45 PST
Comment on attachment 180264 [details]
Patch

Attachment 180264 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15418792
Comment 28 kov's GTK+ EWS bot 2012-12-19 20:08:12 PST
Comment on attachment 180264 [details]
Patch

Attachment 180264 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15404908
Comment 29 Build Bot 2012-12-19 20:12:15 PST
Comment on attachment 180264 [details]
Patch

Attachment 180264 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15399921
Comment 30 Ryan Sleevi 2012-12-19 20:31:28 PST
Created attachment 180267 [details]
Patch
Comment 31 Kentaro Hara 2012-12-19 22:35:16 PST
Comment on attachment 180267 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180267&action=review

Actually I'm not so happy with the approach, because you're trying to solve ArrayBufferView's problem by adding some hacks to V8Crypto. That being said, I cannot come up with a better idea to solve ArrayBufferView's problem, so I'm not objecting to landing the patch for now.

> Source/WebCore/bindings/js/JSCryptoCustom.cpp:51
> +    if (UNLIKELY(ec)) {

Remove UNLIKELY(). It would be useless.

> Source/WebCore/bindings/v8/custom/V8CryptoCustom.cpp:54
> +    if (UNLIKELY(ec))

Remove UNLIKELY(). It would be useless.
Comment 32 Ryan Sleevi 2012-12-19 23:58:44 PST
Comment on attachment 180267 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180267&action=review

>> Source/WebCore/bindings/js/JSCryptoCustom.cpp:51
>> +    if (UNLIKELY(ec)) {
> 
> Remove UNLIKELY(). It would be useless.

Respectfully, I do not think your request is consistent with existing code - especially given that the IDL Code Generators already make extensive use of this macro.

If you believe it to not provide value, that seems like a different and unrelated bug. I would suggest that consistency with the codebase - *especially* the code this is replacing - is more valuable here.

Can I request you reconsider, or point me to a discussion where it was decided to stop following this pattern?
Comment 33 Kentaro Hara 2012-12-20 00:02:27 PST
(In reply to comment #32)
> Can I request you reconsider, or point me to a discussion where it was decided to stop following this pattern?

We've reached a consensus that we don't newly add LIKELY()/UNLIKELY() unless we can confirm its performance gain by adding LIKELY()/UNLIKELY().

Actually I've been trying to remove useless LIKELY()/UNLIKELY()s from generated code (https://bugs.webkit.org/show_bug.cgi?id=101932), though the patch is not yet landed.
Comment 34 Ryan Sleevi 2012-12-20 11:55:36 PST
Created attachment 180379 [details]
Patch
Comment 35 WebKit Review Bot 2012-12-20 14:13:48 PST
Comment on attachment 180379 [details]
Patch

Clearing flags on attachment: 180379

Committed r138298: <http://trac.webkit.org/changeset/138298>
Comment 36 WebKit Review Bot 2012-12-20 14:13:56 PST
All reviewed patches have been landed.  Closing bug.