Bug 162010 - Subtle crypto promise functions should not throw
Summary: Subtle crypto promise functions should not throw
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-15 00:36 PDT by youenn fablet
Modified: 2016-09-20 11:16 PDT (History)
8 users (show)

See Also:


Attachments
Patch (101.22 KB, patch)
2016-09-15 00:40 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (885.40 KB, application/zip)
2016-09-15 01:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (916.60 KB, application/zip)
2016-09-15 01:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.49 MB, application/zip)
2016-09-15 01:42 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2 (9.13 MB, application/zip)
2016-09-15 01:51 PDT, Build Bot
no flags Details
Patch (103.73 KB, patch)
2016-09-15 02:49 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-09-15 00:36:24 PDT
Promise-returning functions should reject promises instead of throwing.
Comment 1 youenn fablet 2016-09-15 00:40:07 PDT
Created attachment 288937 [details]
Patch
Comment 2 Build Bot 2016-09-15 01:37:02 PDT
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
Comment 3 Build Bot 2016-09-15 01:37:05 PDT
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 4 Build Bot 2016-09-15 01:40:55 PDT
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
Comment 5 Build Bot 2016-09-15 01:40:58 PDT
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 6 Build Bot 2016-09-15 01:42:56 PDT
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
Comment 7 Build Bot 2016-09-15 01:42:59 PDT
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 8 Build Bot 2016-09-15 01:51:19 PDT
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
Comment 9 Build Bot 2016-09-15 01:51:22 PDT
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
Comment 10 youenn fablet 2016-09-15 02:49:02 PDT
Created attachment 288946 [details]
Patch
Comment 11 Jiewen Tan 2016-09-15 11:14:16 PDT
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.
Comment 12 Jiewen Tan 2016-09-15 11:23:27 PDT
Sorry, I mistake this one with Bug 126023. Might need a better title.
Comment 13 Alexey Proskuryakov 2016-09-15 12:18:14 PDT
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.
Comment 14 youenn fablet 2016-09-15 12:23:21 PDT
(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.
Comment 15 Alexey Proskuryakov 2016-09-15 12:28:25 PDT
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.
Comment 16 Jiewen Tan 2016-09-15 13:20:34 PDT
(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.
Comment 17 youenn fablet 2016-09-16 00:49:48 PDT
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.
Comment 18 Alexey Proskuryakov 2016-09-16 09:35:12 PDT
> 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.
Comment 19 youenn fablet 2016-09-16 10:54:00 PDT
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.
Comment 20 youenn fablet 2016-09-20 11:16:56 PDT
Wontfix for legacy webkitSubtle.
Should be done for new subtle crypto.
If custom binding is needed, it will be handy to use callPromiseFunction.