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)));"
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.
(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 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 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 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 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 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
Created attachment 179149 [details] Patch
Comment on attachment 179149 [details] Patch Attachment 179149 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15319007
Comment on attachment 179149 [details] Patch Attachment 179149 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15314021
Created attachment 179157 [details] Patch
Created attachment 179168 [details] Patch
Created attachment 179175 [details] Patch
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 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 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.
Thanks, I understood your concern. kbr: Do you have any advice?
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.
Created attachment 180257 [details] Patch
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 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 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 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)
Created attachment 180264 [details] Patch
Comment on attachment 180264 [details] Patch Attachment 180264 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15411811
Comment on attachment 180264 [details] Patch Attachment 180264 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15409842
Comment on attachment 180264 [details] Patch Attachment 180264 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15418792
Comment on attachment 180264 [details] Patch Attachment 180264 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/15404908
Comment on attachment 180264 [details] Patch Attachment 180264 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15399921
Created attachment 180267 [details] Patch
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 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?
(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.
Created attachment 180379 [details] Patch
Comment on attachment 180379 [details] Patch Clearing flags on attachment: 180379 Committed r138298: <http://trac.webkit.org/changeset/138298>
All reviewed patches have been landed. Closing bug.