Bug 162010

Created attachment 288937 [details] Patch

Comment on attachment 288937 [details] Patch Attachment 288937 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2077895 New failing tests: fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html http/tests/websocket/tests/hybi/binary-type.html

Created attachment 288939 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 288937 [details] Patch Attachment 288937 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2077906 New failing tests: fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html http/tests/websocket/tests/hybi/binary-type.html

Created attachment 288940 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 288937 [details] Patch Attachment 288937 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2077894 New failing tests: fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html http/tests/websocket/tests/hybi/binary-type.html

Created attachment 288941 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 288937 [details] Patch Attachment 288937 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2077911 New failing tests: fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html http/tests/websocket/tests/hybi/binary-type.html

Created attachment 288943 [details] Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.6

Created attachment 288946 [details] Patch

Hi Youenn, Thank you for making such change. However, I am not quite convinced that this patch should belong to this bug. What this bug means is that all the methods under Crypto.subtle interface should not throw error. Instead, they should reject the promise to give callers the error. I am not seeing this patch is doing what the spec suggests. https://www.w3.org/TR/WebCryptoAPI/#subtlecrypto-interface-methods Maybe I miss something? FYI, I am currently updating our WebCrypto API to match the latest spec, and in my plan I will create a new interface to replace this old one.

Sorry, I mistake this one with Bug 126023. Might need a better title.

Comment on attachment 288946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288946&action=review > Source/WebCore/ChangeLog:10 > + Making use of callPromiseFunction to catch exceptions and reject promise. Does this change the behavior for both legacy and new WebCrypto? This seems like a fairly large compatibility risk.

(In reply to comment #13) > Comment on attachment 288946 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=288946&action=review > > > Source/WebCore/ChangeLog:10 > > + Making use of callPromiseFunction to catch exceptions and reject promise. > > Does this change the behavior for both legacy and new WebCrypto? This seems > like a fairly large compatibility risk. I do not know a lot about this API, maybe Jeiwen knows more about that. I can check what other browsers are doing though. In general, everybody expects promise-returning functions to not throw so I would expect the compatibility issue to be not too high. But this is the web so there might be already code expecting that behavior.

Since WebKit was a very early implementation, and hasn't been updated for a while, the spec and other implementations have changed. I suspect that most sites have a WebKit-only code path, so comparing to other browsers would not be very enlightening.

(In reply to comment #15) > Since WebKit was a very early implementation, and hasn't been updated for a > while, the spec and other implementations have changed. I suspect that most > sites have a WebKit-only code path, so comparing to other browsers would not > be very enlightening. I endorsed Alexey's statement. I prefer to keep the old interface intact to be backward-compatible.

FWIW, bug 162011 will start rejecting promise for custom functions when the callee does not have the expected type. This is needed for all APIs and will apply to this one as well since this is a CodeGeneratorJS.pm change. The risk is usually lower to go from throwing to rejecting than to start throwing. A few additional points I checked: - Web pages seem to use webkitSubtle like other prefixed properties. If subtle property is not defined on crypto object, they use webkitSubtle as if it was subtle. Then they just use it agnostically. - Chrome is doing like WebKit currently (throwing) for the legacy API. Anyway, feel free to mark the patch r- and WontFix if you prefer it this way.

> Then they just use it agnostically. Is this what Netflix does? Did you test whether it fully works after making this change (especially error handling)? > Anyway, feel free to mark the patch r- and WontFix if you prefer it this way. We should definitely do this on modernized WebCrypto code path.

I only did some search on js files. If you have a web page to Netflix that is using crypto I can have a look. I used JS built ins in the past to implement legacy on top of standard API. The meaningful checks could be done and throw in JS buitlins. As of the current crypto binding code, it would be good to move from two callbacks to a DOMPromise. It is usually simpler and a tad more efficient.

Wontfix for legacy webkitSubtle. Should be done for new subtle crypto. If custom binding is needed, it will be handy to use callPromiseFunction.