Richard Barnes [:rbarnes] Assignee
	Description • 10 years ago

      No description provided.

	Richard Barnes [:rbarnes] Assignee
	Comment 1 • 10 years ago

Note: Assumes Bug 995385

	Camilo Viecco (:cviecco)
	Comment 4 • 10 years ago

Comment on attachment 8409967 [details] [diff] [review]
webcrypto-998804.patch

Review of attachment 8409967 [details] [diff] [review]:
-----------------------------------------------------------------

Again only PSM layer

	Richard Barnes [:rbarnes] Assignee
	Comment 5 • 10 years ago

Minor update to match latest patch to Bug 998385.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 10 years ago

Comment on attachment 8416525 [details] [diff] [review]
webcrypto-998804.1.patch

>+++ b/dom/crypto/WebCryptoTask.cpp
>+class SimpleDigestTask : public ReturnArrayBufferViewTask
>+  SimpleDigestTask(JSContext* aCx,
>+          const ObjectOrString& aAlgorithm,
>+          const CryptoOperationData& aData)

Please fix the weird indent.

>+    : mData(aData)

If this OOMs, shouldn't we fail the task instead of digesting empty data?

>+  virtual nsresult DoCrypto() MOZ_OVERRIDE
>+    mResult.SetLength(hashLen);

If this fails, we need to fail out, instead of overrunning the buffer.

r=me with those fixed.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 7 • 10 years ago

Comment on attachment 8416525 [details] [diff] [review]
webcrypto-998804.1.patch

Review of attachment 8416525 [details] [diff] [review]:
-----------------------------------------------------------------

NSS parts look good. r=me with nits addressed.

::: dom/crypto/WebCryptoTask.cpp
@@ +110,5 @@
> +{
> +public:
> +  SimpleDigestTask(JSContext* aCx,
> +          const ObjectOrString& aAlgorithm,
> +          const CryptoOperationData& aData)

nit: indentation

::: dom/crypto/test/test-vectors.js
@@ +38,5 @@
>  
>    // Truncated form of the PKCS#8 stucture above
>    negative_spki: util.hex2abv("30819F300D06092A864886F70D010101050003"),
> +
> +  // From the NESSIE project 

nit: trailing space

::: dom/crypto/test/tests.js
@@ +313,5 @@
> +TestArray.addTest(
> +  "SHA-256 digest",
> +  function() {
> +    var that = this;
> +    crypto.subtle.digest("SHA-256", tv.sha256.data).then(

How about a sha-1 test as well? In fact, I think it would be best to have a test for each algorithm we support. Also, is there an "unknown algorithm" test?

	Richard Barnes [:rbarnes] Assignee
	Comment 8 • 10 years ago

* Addressed nits in Comment 6 and Comment 7
* Added a test with an unknown algorithm name

	Richard Barnes [:rbarnes] Assignee
	Comment 9 • 10 years ago

Clean try run covering:
* Bug 1005375
* Bug 995385
* Bug 998804

https://tbpl.mozilla.org/?tree=Try&rev=9e24926f0fe8

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/739a582c405c

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/739a582c405c