Bug 104845 – window.crypto.getRandomValues should return the input ArrayBufferView

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ryan Sleevi]

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)));"

[Ryan Sleevi]

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.

[Kentaro Hara]

(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.

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Build Bot]

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

[WebKit Review Bot]

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

[Peter Beverloo (cr-android ews)]

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

[Ryan Sleevi]

Created attachment 179149 [details]
Patch

[Early Warning System Bot]

Comment on attachment 179149 [details]
Patch

Attachment 179149 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15319007

[Early Warning System Bot]

Comment on attachment 179149 [details]
Patch

Attachment 179149 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15314021

[Ryan Sleevi]

Created attachment 179157 [details]
Patch

[Ryan Sleevi]

Created attachment 179168 [details]
Patch

[Ryan Sleevi]

Created attachment 179175 [details]
Patch

[Kentaro Hara]

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.)

[Kentaro Hara]

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.

[Ryan Sleevi]

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.

[Kentaro Hara]

Thanks, I understood your concern.

kbr: Do you have any advice?

[Kenneth Russell]

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.

[Ryan Sleevi]

Created attachment 180257 [details]
Patch

[Kenneth Russell]

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.

[Kentaro Hara]

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.

[Ryan Sleevi]

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.

[Ryan Sleevi]

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)

[Ryan Sleevi]

Created attachment 180264 [details]
Patch

[Early Warning System Bot]

Comment on attachment 180264 [details]
Patch

Attachment 180264 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15411811

[Early Warning System Bot]

Comment on attachment 180264 [details]
Patch

Attachment 180264 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15409842

[EFL EWS Bot]

Comment on attachment 180264 [details]
Patch

Attachment 180264 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15418792

[kov's GTK+ EWS bot]

Comment on attachment 180264 [details]
Patch

Attachment 180264 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15404908

[Build Bot]

Comment on attachment 180264 [details]
Patch

Attachment 180264 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15399921

[Ryan Sleevi]

Created attachment 180267 [details]
Patch

[Kentaro Hara]

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.

[Ryan Sleevi]

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?

[Kentaro Hara]

(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.

[Ryan Sleevi]

Created attachment 180379 [details]
Patch

[WebKit Review Bot]

Comment on attachment 180379 [details]
Patch

Clearing flags on attachment: 180379

Committed r138298: <http://trac.webkit.org/changeset/138298>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.