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

* Implement the WebIDL interfaces for WebCrypto
  * SubtleCrypto and extension to Crypto
  * Key and *KeyAlgorithm
  * Jwk*Key (http://lists.w3.org/Archives/Public/public-webcrypto/2014Mar/0116.html)

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

This patch stubs out the interface for WebCrypto:
* Almost all of the necessary WebIDL interfaces
* Stub implementations of all WebCrypto methods
* Key objects that are structured clonable
* importKey and exportKey methods to exercise the API

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

Since this is a rather big patch, some notes to help readers.

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

Forgot one line that fixes a crash on Fennec.

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

Comment on attachment 8408206 [details] [diff] [review]
webcrypto-995385.patch

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

I didn't review any of the DOM/JS-specific things, since I don't know enough about that (and I'm not a DOM peer anyway). The NSS-related parts are looking really good. I did have a few concerns about how memory is managed - see comments. Also, make sure this code follows the style guidelines: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style (for the most part it does). The tests look pretty good so far, but it would be nice to have cases that test import failures, as well (unless I missed that?). I would like another look at this, so r- for now.

::: dom/base/nsJSEnvironment.cpp
@@ +2789,5 @@
> +    // Prepare the ImageData internals.
> +    uint32_t width = imageData->Width();
> +    uint32_t height = imageData->Height();
> +    JS::Rooted<JSObject*> dataArray(cx, imageData->GetDataObject());
> +  

nit: unnecessary whitespace (not the linebreak - the spaces on the linebreak)

::: dom/crypto/CryptoBuffer.cpp
@@ +66,5 @@
> +void CryptoBuffer::Assign(const nsString& aBase64)
> +{
> +  nsCString temp = NS_ConvertUTF16toUTF8(aBase64);
> +  temp.StripWhitespace();
> +  

nit: unnecessary whitespace

@@ +92,5 @@
> +
> +nsString CryptoBuffer::AsBase64url() const
> +{
> +  if (Length() == 0) { return NS_LITERAL_STRING(""); }
> +  

nit: whitespace

@@ +113,5 @@
> +    return false;
> +  }
> +
> +  aRetVal = 0;
> +  for (size_t i=0; i<Length(); ++i) {

nit: spaces around operators

::: dom/crypto/HmacKeyAlgorithm.h
@@ +33,5 @@
> +      case CKM_SHA_1: mMechanism = CKM_SHA_1_HMAC; break;
> +      case CKM_SHA224: mMechanism = CKM_SHA224_HMAC; break;
> +      case CKM_SHA256: mMechanism = CKM_SHA256_HMAC; break;
> +      case CKM_SHA384: mMechanism = CKM_SHA384_HMAC; break;
> +      case CKM_SHA512: mMechanism = CKM_SHA512_HMAC; break;

We should probably have a default case to catch/assert on unknown/unimplemented mechanisms.

::: dom/crypto/Key.cpp
@@ +152,5 @@
> +}
> +
> +SECKEYPrivateKey* Key::GetPrivateKey() const
> +{
> +  return mPrivateKey.get();

It looks like the setters (SetPrivateKey, SetPublicKey) copy memory, whereas the getters share memory. This api asymmetry could get confusing and lead to bugs. If the concern is performance, I would make both setters and getters share memory and then do something like "key.SetPublicKey(SECKEY_CopyPublicKey(publicKey));" where appropriate. I'm sure there are other options, as well.

@@ +165,5 @@
> +
> +SECKEYPrivateKey* Key::PrivateKeyFromPkcs8(CryptoBuffer& aKeyData)
> +{
> +  SECKEYPrivateKey* privKey;
> +  ScopedPK11SlotInfo slot(PK11_GetInternalSlot());

We might have a problem with ensuring we're between NSS initialization and shutdown - see nsNSSShutDown.h (there are some NSS functions we can call outside of init/shutdown, but I don't think this is one).

@@ +213,5 @@
> +  return symKey;
> +}
> +
> +const SEC_ASN1Template RSAPrivateKeyTemplate[] = {
> +  {SEC_ASN1_SEQUENCE, 0, NULL, sizeof(RSAPrivateKey)},

nit: space after/before curly braces

@@ +236,5 @@
> +
> +nsresult Key::PrivateKeyToPkcs8(SECKEYPrivateKey* aPrivKey, CryptoBuffer& aRetVal)
> +{
> +  ScopedSECItem pkcs8Item(PK11_ExportDERPrivateKeyInfo(aPrivKey, nullptr));
> +  if (!pkcs8Item.get()) {

Does (!pkcs8Item) not work directly? (On some scoped types it does, but not on others. I can never remember.)

@@ +259,5 @@
> +{
> +  nsresult rv = MapSECStatus(PK11_ExtractKeyValue(aSymKey));
> +  if (NS_FAILED(rv)) { return NS_ERROR_DOM_INVALID_ACCESS_ERR; }
> +
> +  SECItem* keyItem = PK11_GetKeyData(aSymKey);

ScopedSECItem?

::: dom/crypto/Key.h
@@ +42,5 @@
> +
> +   3                   2                   1                   0
> + 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
> ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> +|~~~~~~~~~~~~~~~|     Usage     |     Type      |~~~~~~~~~~~~~|E|

Do we need to be so clever about storing this information?

::: dom/crypto/KeyAlgorithm.cpp
@@ +51,5 @@
> +  } else if (mName.EqualsLiteral(WEBCRYPTO_ALG_RSAES_PKCS1)) {
> +    mMechanism = CKM_RSA_PKCS;
> +  } else if (mName.EqualsLiteral(WEBCRYPTO_ALG_RSASSA_PKCS1)) {
> +    mMechanism = CKM_RSA_PKCS;
> +  }

else {
// some sort of assertion or other failure so we can catch bugs/improper input
}

@@ +98,5 @@
> +      algorithm =  RsaKeyAlgorithm::Create(aReader);
> +      break;
> +    case SCTAG_RSAHASHEDKEYALG:
> +      algorithm =  RsaHashedKeyAlgorithm::Create(aReader);
> +      break;

Add a default assertion/failure case?

@@ +129,5 @@
> +bool KeyAlgorithm::WriteString(JSStructuredCloneWriter* aWriter, const nsString& aString) const
> +{
> +  size_t charSize = sizeof(nsString::char_type);
> +  return JS_WriteUint32Pair( aWriter, aString.Length(), 0 ) &&
> +         JS_WriteBytes( aWriter, aString.get(), aString.Length() * charSize );

nit: no spaces after/before parentheses

::: dom/crypto/WebCryptoTask.cpp
@@ +283,5 @@
> +        return NS_ERROR_DOM_DATA_ERR;
> +      }
> +
> +      if (privKey->keyType != rsaKey) {
> +        SECKEY_DestroyPrivateKey(privKey);

But if privKey is a scoped type, it should take care of destroying itself on function return, right?

@@ +289,5 @@
> +      }
> +
> +      mKey->SetPrivateKey(privKey.get());
> +      mKey->SetType(Key::PRIVATE);
> +      pubKey = SECKEY_ConvertToPublicKey(privKey.get());

Check to see this didn't fail?

@@ +319,5 @@
> +      return NS_ERROR_DOM_SYNTAX_ERR;
> +    }
> +
> +    // Construct an appropriate KeyAlgorithm
> +    uint32_t modulusLength = 8*pubKey->u.rsa.modulus.len;

nit: spaces around operators ("8 * pubKey...")

@@ +440,5 @@
> +  if (NS_FAILED(rv)) {
> +    return new FailureTask(rv);
> +  }
> +
> +  if (algName.EqualsASCII(WEBCRYPTO_ALG_AES_CBC) ||

Maybe EqualsLiteral?

::: dom/crypto/WebCryptoTask.h
@@ +123,5 @@
> +};
> +
> +
> +
> +

Watch out for excess vertical space - I'm fairly sure one empty line will do here.

::: dom/crypto/test/test-array.js
@@ +41,5 @@
> +        // Set result
> +        this.result = result;
> +        // Re-draw the row
> +        this.draw();
> +        

nit: whitespace

@@ +57,5 @@
> +    };
> +
> +    this.draw = function() {
> +        if (!this.row) return;
> +        

nit: whitespace

@@ +152,5 @@
> +        if (i >= this.tests.length) {
> +          if (MOCHITEST) { SimpleTest.finish(); }
> +          return;
> +        }
> +        

nit: whitespace

::: dom/crypto/test/util.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var util = {

In general, I think we want to use let, not var.

::: security/manager/ssl/src/ScopedNSSTypes.h
@@ +113,5 @@
>  MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedNSSCMSSignedData,
>                                            NSSCMSSignedData,
>                                            NSS_CMSSignedData_Destroy)
>  
> +MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedPLArenaPool,

It doesn't look like any of these new scoped types are used yet - let's add them as we need them.

::: xpcom/base/ErrorList.h
@@ +487,5 @@
>    ERROR(NS_ERROR_DOM_ENCODING_NOT_UTF_ERR,         FAILURE(29)),
>    ERROR(NS_ERROR_DOM_ENCODING_DECODE_ERR,          FAILURE(30)),
>    ERROR(NS_ERROR_DOM_INVALID_POINTER_ERR,          FAILURE(31)),
> +  /* WebCrypto API errors from http://www.w3.org/TR/WebCryptoAPI/ */
> +  ERROR(NS_ERROR_DOM_UNKNOWN_ERR,                  FAILURE(31)),

nit: again, reusing FAILURE(31) here (see NS_ERROR_DOM_INVALID_POINTER_ERR)
(hmmm - I see those are two different bodies assigning values - is there a collision here as a result of the specs?)

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

Updates in response to Keeler's review.

(In reply to David Keeler (:keeler) from comment #5)
> The tests look pretty good so far,
> but it would be nice to have cases that test import failures, as well
> (unless I missed that?). 

Added tests for import failures, as well as testing that exportKey refuses to export a key when extractable == false.


> ::: dom/crypto/HmacKeyAlgorithm.h
> @@ +33,5 @@
> > +      case CKM_SHA_1: mMechanism = CKM_SHA_1_HMAC; break;
> > +      case CKM_SHA224: mMechanism = CKM_SHA224_HMAC; break;
> > +      case CKM_SHA256: mMechanism = CKM_SHA256_HMAC; break;
> > +      case CKM_SHA384: mMechanism = CKM_SHA384_HMAC; break;
> > +      case CKM_SHA512: mMechanism = CKM_SHA512_HMAC; break;
> 
> We should probably have a default case to catch/assert on
> unknown/unimplemented mechanisms.

Added:
(1) a default case that sets mMechanism = UNKNOWN_CK_MECHANISM, and 
(2) a check for UNKNOWN_CK_MECHANISM in ImportSymmetricKeyTask (the only place this can be set right now)

> ::: dom/crypto/Key.cpp
> @@ +152,5 @@
> > +}
> > +
> > +SECKEYPrivateKey* Key::GetPrivateKey() const
> > +{
> > +  return mPrivateKey.get();
> 
> It looks like the setters (SetPrivateKey, SetPublicKey) copy memory, whereas
> the getters share memory. This api asymmetry could get confusing and lead to
> bugs. If the concern is performance, I would make both setters and getters
> share memory and then do something like
> "key.SetPublicKey(SECKEY_CopyPublicKey(publicKey));" where appropriate. I'm
> sure there are other options, as well.

It's more of an oversight than a performance concern.  I've changed this to just copy in all cases.  We can always revert it if it's too slow.


> @@ +165,5 @@
> > +
> > +SECKEYPrivateKey* Key::PrivateKeyFromPkcs8(CryptoBuffer& aKeyData)
> > +{
> > +  SECKEYPrivateKey* privKey;
> > +  ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
> 
> We might have a problem with ensuring we're between NSS initialization and
> shutdown - see nsNSSShutDown.h (there are some NSS functions we can call
> outside of init/shutdown, but I don't think this is one).

It should be the case here that all NSS interactions are within CryptoTask::Run (I made a few edits to ensure that's the case), which is already guarded for NSS shutdown.  (CryptoTask already inherits from nsNSSShutDownObject.)  So as long as we stick to that pattern, we should be OK.  (Right?)

It is true that if someone used this method outside a guarded context, there could be trouble. Can I get away with just adding a comment here noting that?


> @@ +213,5 @@
> > +  return symKey;
> > +}
> > +
> > +const SEC_ASN1Template RSAPrivateKeyTemplate[] = {
> > +  {SEC_ASN1_SEQUENCE, 0, NULL, sizeof(RSAPrivateKey)},
> 
> nit: space after/before curly braces

Actually, both RSAPrivateKeyTemplate and RSAPublicKeyTemplate were unnecessary (vestiges of some code that's been removed).  I've removed them both.


> @@ +236,5 @@
> > +
> > +nsresult Key::PrivateKeyToPkcs8(SECKEYPrivateKey* aPrivKey, CryptoBuffer& aRetVal)
> > +{
> > +  ScopedSECItem pkcs8Item(PK11_ExportDERPrivateKeyInfo(aPrivKey, nullptr));
> > +  if (!pkcs8Item.get()) {
> 
> Does (!pkcs8Item) not work directly? (On some scoped types it does, but not
> on others. I can never remember.)

It appears to, in this case, given that the negative test cases work correctly.


> ::: dom/crypto/Key.h
> @@ +42,5 @@
> > +
> > +   3                   2                   1                   0
> > + 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
> > ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > +|~~~~~~~~~~~~~~~|     Usage     |     Type      |~~~~~~~~~~~~~|E|
> 
> Do we need to be so clever about storing this information?

It makes structured cloning simpler, and uses marginally less memory.  One thing to read/write instead of three, and one uint32_t instead of a bool and two enums.


> ::: dom/crypto/KeyAlgorithm.cpp
> @@ +51,5 @@
> > +  } else if (mName.EqualsLiteral(WEBCRYPTO_ALG_RSAES_PKCS1)) {
> > +    mMechanism = CKM_RSA_PKCS;
> > +  } else if (mName.EqualsLiteral(WEBCRYPTO_ALG_RSASSA_PKCS1)) {
> > +    mMechanism = CKM_RSA_PKCS;
> > +  }
> 
> else {
> // some sort of assertion or other failure so we can catch bugs/improper
> input
> }

Changed to set mMechanism to UNKNOWN_CK_MECHANISM, so that errors can be caught in WebCryptoTasks.  (Most of the time, that error should already be caught there anyway.)


> @@ +98,5 @@
> > +      algorithm =  RsaKeyAlgorithm::Create(aReader);
> > +      break;
> > +    case SCTAG_RSAHASHEDKEYALG:
> > +      algorithm =  RsaHashedKeyAlgorithm::Create(aReader);
> > +      break;
> 
> Add a default assertion/failure case?

Added default case returning nullptr, and updated Key.cpp to pass this failure up.  It's then handled correctly in nsJSEnvironment.cpp.


> ::: dom/crypto/test/util.js
> @@ +1,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +var util = {
> 
> In general, I think we want to use let, not var.

In this case, it's running as content JS, which seems unhappy with 'let'.


> ::: xpcom/base/ErrorList.h
> @@ +487,5 @@
> >    ERROR(NS_ERROR_DOM_ENCODING_NOT_UTF_ERR,         FAILURE(29)),
> >    ERROR(NS_ERROR_DOM_ENCODING_DECODE_ERR,          FAILURE(30)),
> >    ERROR(NS_ERROR_DOM_INVALID_POINTER_ERR,          FAILURE(31)),
> > +  /* WebCrypto API errors from http://www.w3.org/TR/WebCryptoAPI/ */
> > +  ERROR(NS_ERROR_DOM_UNKNOWN_ERR,                  FAILURE(31)),
> 
> nit: again, reusing FAILURE(31) here (see NS_ERROR_DOM_INVALID_POINTER_ERR)
> (hmmm - I see those are two different bodies assigning values - is there a
> collision here as a result of the specs?)

Oops, I thought I had gotten that one.  Actually, AFAICT, the numbers in this file greater than 25 are internal, not standard.  I'm hoping that Boris will correct me on that if I'm wrong.

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

Comment on attachment 8409961 [details] [diff] [review]
webcrypto-995385.2.patch

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

Did not take a look at the DOM parts.

Your tests are round trip so we cannot detect if we are actually using crypto or just memcpy. You should add one direction tests to ensure you actually compute what you think you are computing.

::: dom/crypto/CryptoBuffer.cpp
@@ +64,5 @@
> +}
> +
> +void CryptoBuffer::Assign(const nsString& aBase64)
> +{
> +  nsCString temp = NS_ConvertUTF16toUTF8(aBase64);

I am confused by this funcion... (there is no documentation) on what you are trying to accomplish here. Is item supposed to be binary?

::: dom/crypto/CryptoBuffer.h
@@ +36,5 @@
> +  {
> +    Assign(aData);
> +    return *this;
> +  }
> +

no void functions please, make sure internal errors can be reported back or thrown

::: dom/crypto/Key.cpp
@@ +49,5 @@
> +void Key::GetType(nsString& aRetVal) const
> +{
> +  if      (mAttributes & PUBLIC)  { aRetVal.AssignLiteral(WEBCRYPTO_KEY_TYPE_PUBLIC);  }
> +  else if (mAttributes & PRIVATE) { aRetVal.AssignLiteral(WEBCRYPTO_KEY_TYPE_PRIVATE); }
> +  else if (mAttributes & SECRET)  { aRetVal.AssignLiteral(WEBCRYPTO_KEY_TYPE_SECRET);  }

default case?

@@ +72,5 @@
> +  if (mAttributes & VERIFY)     { aRetVal.AppendElement(NS_LITERAL_STRING(WEBCRYPTO_KEY_USAGE_VERIFY)); }
> +  if (mAttributes & DERIVEKEY)  { aRetVal.AppendElement(NS_LITERAL_STRING(WEBCRYPTO_KEY_USAGE_DERIVEKEY)); }
> +  if (mAttributes & DERIVEBITS) { aRetVal.AppendElement(NS_LITERAL_STRING(WEBCRYPTO_KEY_USAGE_DERIVEBITS)); }
> +  if (mAttributes & WRAPKEY)    { aRetVal.AppendElement(NS_LITERAL_STRING(WEBCRYPTO_KEY_USAGE_WRAPKEY)); }
> +  if (mAttributes & UNWRAPKEY)  { aRetVal.AppendElement(NS_LITERAL_STRING(WEBCRYPTO_KEY_USAGE_UNWRAPKEY)); }

default case?

::: dom/crypto/Key.h
@@ +68,5 @@
> +    UNKNOWN = 0x00000000,
> +    SECRET  = 0x00000100,
> +    PUBLIC  = 0x00000200,
> +    PRIVATE = 0x00000400
> +  };

If there can only be one type at a time why is this done with a flag and not a simple enum? (in your struct)

::: dom/crypto/KeyAlgorithm.cpp
@@ +38,5 @@
> +    mMechanism = CKM_AES_CBC_PAD;
> +  } else if (mName.EqualsLiteral(WEBCRYPTO_ALG_AES_CTR)) {
> +    mMechanism = CKM_AES_CTR;
> +  } else if (mName.EqualsLiteral(WEBCRYPTO_ALG_AES_GCM)) {
> +    mMechanism = CKM_AES_CTR;

not CKM_AES_GCM ?

@@ +53,5 @@
> +  } else if (mName.EqualsLiteral(WEBCRYPTO_ALG_RSAES_PKCS1)) {
> +    mMechanism = CKM_RSA_PKCS;
> +  } else if (mName.EqualsLiteral(WEBCRYPTO_ALG_RSASSA_PKCS1)) {
> +    mMechanism = CKM_RSA_PKCS;
> +  } else {

set nName to invalid here?

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

(In reply to Camilo Viecco (:cviecco) from comment #8)

> Your tests are round trip so we cannot detect if we are actually using
> crypto or just memcpy. You should add one direction tests to ensure you
> actually compute what you think you are computing.

For the functionality that these tests are concerned with, it actually doesn't matter whether there's crypto or memcpy going on.  We're not doing, say, an encrypt/decrypt round trip, we're just testing that we can store something in a Key object and get it back.  Doing crypto is in later patches.

So it's not clear to me what value unidirectional tests would add.  The round-trip tests already check for all attributes that are visible to JS.  The actual key values are deliberately hidden from JS -- that's part of the security design.  There are only two ways to observe them:
1. Do a crypto operation (encrypt, sign, etc.), which this patch doesn't implement.
2. Export the key again, which is what these tests do.

I've extended the tests so that they check the values of all properties visible to JS, instead of just checking that they exist.  I also fixed a bug that allowed keys to be created with usages that did not match their algorithms (e.g., "encrypt" for "RSASSA-PKCS1-v1_5").


> ::: dom/crypto/CryptoBuffer.cpp
> @@ +64,5 @@
> > +}
> > +
> > +void CryptoBuffer::Assign(const nsString& aBase64)
> > +{
> > +  nsCString temp = NS_ConvertUTF16toUTF8(aBase64);
> 
> I am confused by this funcion... (there is no documentation) on what you are
> trying to accomplish here. Is item supposed to be binary?

I've removed this method, as well as CryptoBuffer::ToBase64Url().  These were needed for JWK support, which has been removed.


> ::: dom/crypto/CryptoBuffer.h
> @@ +36,5 @@
> > +  {
> > +    Assign(aData);
> > +    return *this;
> > +  }
> > +
> 
> no void functions please, make sure internal errors can be reported back or
> thrown

We don't need return values in this case, because there are no errors to report.  If you look at the various flavors of Assign(), none of them have failure modes other than "there's no data in this", in which case it's appropriate to just reset the CryptoBuffer to zero length.

For example, in CryptoBuffer::Assign(const ArrayBufferViewOrArrayBuffer& aData), there's an "else" clause, but that only hits if aData has the value "eUninitialized" (vs. eArrayBuffer / eArrayBufferView) -- which means it has no value.  So assigning this to a CryptoBuffer is the same as setting it to the empty buffer.


> ::: dom/crypto/Key.cpp
> @@ +49,5 @@
> > +void Key::GetType(nsString& aRetVal) const
> > +{
> > +  if      (mAttributes & PUBLIC)  { aRetVal.AssignLiteral(WEBCRYPTO_KEY_TYPE_PUBLIC);  }
> > +  else if (mAttributes & PRIVATE) { aRetVal.AssignLiteral(WEBCRYPTO_KEY_TYPE_PRIVATE); }
> > +  else if (mAttributes & SECRET)  { aRetVal.AssignLiteral(WEBCRYPTO_KEY_TYPE_SECRET);  }
> 
> default case?

Nope.  It's tempting to make this return nsresult, and return say NS_ERROR_ILLEGAL_VALUE if there's a weird value.  But the WebIDL calling convention requires the return type to be void.  So we just have to make sure nothing weird gets in there, which I think we do adequately in SetType().


> @@ +72,5 @@
> > +  if (mAttributes & VERIFY)     { aRetVal.AppendElement(NS_LITERAL_STRING(WEBCRYPTO_KEY_USAGE_VERIFY)); }
> > +  if (mAttributes & DERIVEKEY)  { aRetVal.AppendElement(NS_LITERAL_STRING(WEBCRYPTO_KEY_USAGE_DERIVEKEY)); }
> > +  if (mAttributes & DERIVEBITS) { aRetVal.AppendElement(NS_LITERAL_STRING(WEBCRYPTO_KEY_USAGE_DERIVEBITS)); }
> > +  if (mAttributes & WRAPKEY)    { aRetVal.AppendElement(NS_LITERAL_STRING(WEBCRYPTO_KEY_USAGE_WRAPKEY)); }
> > +  if (mAttributes & UNWRAPKEY)  { aRetVal.AppendElement(NS_LITERAL_STRING(WEBCRYPTO_KEY_USAGE_UNWRAPKEY)); }
> 
> default case?

None needed.  Default action is to do nothing to aRetVal. (Note that these are not "if ... else if", like a switch; rather they're parallel if statements.)


> ::: dom/crypto/Key.h
> @@ +68,5 @@
> > +    UNKNOWN = 0x00000000,
> > +    SECRET  = 0x00000100,
> > +    PUBLIC  = 0x00000200,
> > +    PRIVATE = 0x00000400
> > +  };
> 
> If there can only be one type at a time why is this done with a flag and not
> a simple enum? (in your struct)

Well, it is an enum :)  Just with values that correspond to flags.

I've changed the value of PRIVATE to 3 so it looks more like an enum (i.e., | doesn't combine values, at least not properly).


> ::: dom/crypto/KeyAlgorithm.cpp
> @@ +38,5 @@
> > +    mMechanism = CKM_AES_CBC_PAD;
> > +  } else if (mName.EqualsLiteral(WEBCRYPTO_ALG_AES_CTR)) {
> > +    mMechanism = CKM_AES_CTR;
> > +  } else if (mName.EqualsLiteral(WEBCRYPTO_ALG_AES_GCM)) {
> > +    mMechanism = CKM_AES_CTR;
> 
> not CKM_AES_GCM ?

Heh.  Good point.  Fixed.


> @@ +53,5 @@
> > +  } else if (mName.EqualsLiteral(WEBCRYPTO_ALG_RSAES_PKCS1)) {
> > +    mMechanism = CKM_RSA_PKCS;
> > +  } else if (mName.EqualsLiteral(WEBCRYPTO_ALG_RSASSA_PKCS1)) {
> > +    mMechanism = CKM_RSA_PKCS;
> > +  } else {
> 
> set nName to invalid here?

I don't think that's appropriate.  This code only covers algorithms that can be mapped to an NSS mechanism based on their name alone.  Other algorithms require additional information to look up the proper mechanism.

For example, this code does not set mMechanism for HMAC algorithms, since you need to also need the hash name (in that case aName is just "HMAC").  So the constructor for HmacKeyAlgorithm sets mMechanism (HmacKeyAlgorithm.h:34).

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

Comment on attachment 8410707 [details] [diff] [review]
webcrypto-995385.3.patch

>+++ b/dom/base/Crypto.h
>+#include "SubtleCrypto.h"

mozilla/dom/SubtleCrypto.h.

Which also means you should be exporting the header to mozilla/dom/, not to mozilla/, and should remove the then-pointless headerFile bit from Bindings.conf.  The same applies to the other headers this patch is adding.  The headerFile annotation is there for legacy stuff that for some reason isn't exported to mozilla/dom yet, not for new code.

I don't see any particular reason to put the new getter in the header.

You also need to add the new member to cycle collection for Crypto.  And presumably add a test that would have caught that leak.  Something like this ought to be enough to leak with the patch as is:

  crypto.subtle.foo = crypto;

>+++ b/dom/base/DOMException.cpp
>+  /* WebCrypto errors https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#dfn-DataError */
>+  OperationError           = 0,

I assume the TAG has signed off on this and whatnot...  Code-wise it's correct, but adding new DOMException types is generally fishy from a platform perspective.

Is there a reason you didn't add this to the end of the enum?

>+++ b/dom/base/StructuredCloneTags.h
>+  // This tag is for WebCrypto keys
>+  SCTAG_DOM_KEY,

How abotu SCTAG_DOM_WEBCRYPTO_KEY, to make it clear what's going on?

>+++ b/dom/base/SubtleCrypto.cpp
>+#include "SubtleCrypto.h"

mozilla/dom/SubtleCrypto.h

>+#include "jsapi.h"
>+#include "jsfriendapi.h"

Why are these needed here?  They don't seem to be.

>+#include "mozilla/dom/TypedArray.h"

Same for this header.

>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(SubtleCrypto)

This needs to cycle collect mWindow.  As things stand, I'm pretty sure that:

  var subtle = crypto.subtle;

in window scope leaks.

>+  MOZ_COUNT_CTOR(SubtleCrypto);
>+  MOZ_COUNT_DTOR(SubtleCrypto);

No need for that in a refcounted object: all the addrefs/releases are already logged.

>+void SubtleCrypto::Init(nsPIDOMWindow* aWindow)

If this is infallible, why is it not just a constructor argument?  Imo it should be.

>+#define SUBTLECRYPTO_METHOD_BODY(Operation, ...) \

It's not clear to me why we 

>+already_AddRefed<Promise> SubtleCrypto::Encrypt(
>+                                  JSContext* cx,

Standard DOM style is:

already_AddRefed<Promise>
SubtleCrypto::Encrypt(JSContext* cx,

and with the other arguments lined up with the first one.  This applies to a bunch of functions in this file and elsewhere.  I notice some of the code here _is_ following that style already, too.

>+++ b/dom/base/SubtleCrypto.h
>+#ifndef mozilla_dom_SubtleCrypto_h__

Please drop the trailing double underscores.  Identifiers containing a double underscore are reserved for compilers.

>+#include "mozilla/ErrorResult.h"

This seems totally unnecessary here.

>+#include "Key.h"

mozilla/dom/Key.h

I do wonder about using this generic a name for it in the spec...

>+#include "mozilla/dom/SubtleCryptoBinding.h"

This should not be needed.  Why is it here?

>+struct JSContext;

Probably better to #include "js/TypeDecls.h"

>+                                    mozilla::dom::Key& key,

You're already in the mozilla::dom namespace, right?  Why is this qualifier needed?  That comes up multiple places in this header.

>+  already_AddRefed<Promise> ImportKey(JSContext* cx,
>+                                      bool extractable, const Sequence<nsString>& keyUsages);

Please wrap before that last arg.  Same for GenerateKey and DeriveKey.

>+  already_AddRefed<Promise> ExportKey(const nsAString& format, mozilla::dom::Key& key);

And here.

>+  Promise* CreatePromise() const;

There is no such function.

>+++ b/dom/base/nsJSEnvironment.cpp

The changes here let one postMessage a Key to a window, right?

Is leaving out postMessage to/from workers purposeful here?  If not, please file a folowup to add that?

>+++ b/dom/bindings/Bindings.conf

None of the changes to this file should be needed if the headers are exported to the right place.

>+++ b/dom/crypto/AesKeyAlgorithm.cpp
>+#include "AesKeyAlgorithm.h"

mozilla/dom

>+#include "nsContentUtils.h"

Not needed.

>+++ b/dom/crypto/AesKeyAlgorithm.h
>+#ifndef mozilla_dom_AesKeyAlgorithm_h__

Drop the "__".

>+#include "mozilla/ErrorResult.h"
>+#include "nsCycleCollectionParticipant.h"
>+#include "nsWrapperCache.h"

These all seem uused here.

>+#include "KeyAlgorithm.h"

mozilla/dom

>+struct JSContext;

js/TypeDecls.h

>+class AesKeyAlgorithm MOZ_FINAL : public KeyAlgorithm
>+  //NS_DECL_ISUPPORTS_INHERITED
>+  //NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(AesKeyAlgorithm, KeyAlgorithm)

Drop those.

>+    SetIsDOMBinding();

The superclass does this already.

>+  // TODO: return something sensible here, and change the return type

This needs to be fixed before landing.  You can presumably just do this in KeyAlgorithm and have all the subclasses inherit the GetParentObject.

>+  virtual bool WriteStructuredClone(JSStructuredCloneWriter* aWriter) const

MOZ_OVERRIDE, right?

Also, putting virtual method implementations in the header is pointless and just makes it harder to read.  Just put them in the .cpp?

>+  static KeyAlgorithm* Create(JSStructuredCloneReader* aReader) {
>+    JS_ReadUint32Pair(aReader, &length, &zero);

That can fail, no?  This code needs to handle that.

>+    nsString name = KeyAlgorithm::ReadString(aReader);

And that _should_ be able to fail.  It should have an nsString& ourparam and a boolean return value indicatint success or failure.

>+++ b/dom/crypto/CryptoBuffer.cpp

>+void CryptoBuffer::Assign(const uint8_t* aData, uint32_t aLength)
>+  SetLength(aLength);

This is an infallible array.  So it seems pretty trivial to trigger OOM crashes by passing large buffers to this code...

It's probably better for this to be a fallible array and propagate OOM to callers who then convert it into exceptions or promise rejections or something.

Also, why d you need the SetLength() call?  ReplaceElementsAt() will adjust the length as needed, no?

>+  ReplaceElementsAt(0, aLength, aData, aLength);

And this should then be:

  ReplaceElementsAt(0, Length(), aData, aLength);

>+void CryptoBuffer::Assign(const ArrayBufferView& aData)
>+  AutoJSContext cx;
>+  Assign(ArrayBuffer(JS_GetArrayBufferViewBuffer(cx, aData.Obj())));

I doubt that's what you want: that will assign the entire arraybuffer, not just the part this view corresponds to.  You probably want:

  Assign(aData.Data(), aData.Length());

instead.

>+void CryptoBuffer::Assign(const ArrayBufferViewOrArrayBuffer& aData)
>+    Assign(aData.GetAsArrayBufferView().Obj());

Just Assign(aData.GetAsArrayBufferView());

>+  } else {
>+    SetLength(0);

Should that have some sort of notreached assert?

>+void CryptoBuffer::Assign(const OwningArrayBufferViewOrArrayBuffer& aData)
>+    Assign(aData.GetAsArrayBufferView().Obj());

As above.

>+    SetLength(0);

And here.

>+void CryptoBuffer::Assign(JSObject* aObj)

This can probably go away entirely.

>+SECItem CryptoBuffer::AsSECItem()
>+  return {siBuffer, Elements(), Length()};

This seems pretty dangerous.  What makes sure no one calls Assign() on us while someone else is working with that SECItem, leaving the pointer dangling?

>+bool CryptoBuffer::GetBigIntValue(unsigned long& aRetVal)

unsigned long is not very BigInt...

This also seems unused; I'd have to look at uses to see whether this makes sense.

>+  for (size_t i=0; i < Length(); ++i) {
>+    aRetVal = (aRetVal << 8) + ElementAt(i);

So this assumes our data is big-endian, right?  Is that documented somewhere?  Note that typed array views are native-endian or little-endian....

>+++ b/dom/crypto/CryptoBuffer.h
>+#ifndef mozilla_dom_CryptoBuffer_h__

You know the drill.

>+#include "mozilla/dom/TypedArray.h"

This could be some forward-decls instead, but either way.

I'm up to the end of CryptoBuffer.h so far.  More comments tomorrow.

Also, I'd really appreciate an interdiff when you update the patch, not just an updated patch...

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

Trimmed a bunch of stuff that I just fixed.  See inter-diff.

One high-level point:

Several of these issues came out of the WebIDL code generator:
* Addition of a trailing "__" to #ifndef guards on header files
* Use of "mozilla::dom::Key" instead of "Key"
* Inclusion of nsContentUtils.h

We should consider opening bugs to fix those in the code generator.


(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #10)
> Comment on attachment 8410707 [details] [diff] [review]
> webcrypto-995385.3.patch
>
> >+++ b/dom/base/DOMException.cpp
> >+  /* WebCrypto errors https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#dfn-DataError */
> >+  OperationError           = 0,
>
> I assume the TAG has signed off on this and whatnot...  Code-wise it's
> correct, but adding new DOMException types is generally fishy from a
> platform perspective.
>
> Is there a reason you didn't add this to the end of the enum?

Moved to the end of the enum.  As far as new exception types, I'm just following the spec...


> >+#define SUBTLECRYPTO_METHOD_BODY(Operation, ...) \
>
> It's not clear to me why we

... need a macro for this?  Was just trying to avoid repetition.


> >+++ b/dom/base/SubtleCrypto.h
> >+#ifndef mozilla_dom_SubtleCrypto_h__
>
> Please drop the trailing double underscores.  Identifiers containing a
> double underscore are reserved for compilers.

Done.

Note: The "__" was added by the WebIDL code generator.  So we should fix that there.


> I do wonder about using this generic a name for it in the spec...

Again, just following the spec here.  There are some lurking issues with IndexedDB, which also has a Key class.


> >+                                    mozilla::dom::Key& key,
>
> You're already in the mozilla::dom namespace, right?  Why is this qualifier
> needed?  That comes up multiple places in this header.

That was added by the WebIDL code generator.  Don't know why.  Seems to work without it, so I've removed it.


> >+++ b/dom/base/nsJSEnvironment.cpp
>
> The changes here let one postMessage a Key to a window, right?
>
> Is leaving out postMessage to/from workers purposeful here?  If not, please
> file a folowup to add that?

I *thought* that the changes here would address all the different ways that structured clone are used, including postMessage but also IndexedDB.  (The latter being the main use case I had in mind.)  I'm not sure if it covers postMessage to workers or not.

It wouldn't be terribly useful to send a Key to a worker, though, since we don't yet expose the crypto API to workers.  See Bug 842818.


> >+    SetIsDOMBinding();
>
> The superclass does this already.
>
> >+  // TODO: return something sensible here, and change the return type
>
> This needs to be fixed before landing.  You can presumably just do this in
> KeyAlgorithm and have all the subclasses inherit the GetParentObject.

Can you provide some guidance on what this should be?  It's not clear to me what the desired semantic is.


> >+++ b/dom/crypto/CryptoBuffer.cpp
>
> >+void CryptoBuffer::Assign(const uint8_t* aData, uint32_t aLength)
> >+  SetLength(aLength);
>
> This is an infallible array.  So it seems pretty trivial to trigger OOM
> crashes by passing large buffers to this code...
>
> It's probably better for this to be a fallible array and propagate OOM to
> callers who then convert it into exceptions or promise rejections or
> something.

I've changed the base class to nsFallibleTArray, and updated the Assign() methods to return uint8_t*, since that's how ReplaceElementsAt signals its failure to allocate.

That said, a lot of the current usage of Assign is implicit via operator=, so I would need to refactor stuff to check for the error indication in all cases.  For now, though I think the current checking captures the essential checks.  Something to keep an eye out for in the remainder of your review.


> >+  } else {
> >+    SetLength(0);
>
> Should that have some sort of notreached assert?
>
> >+void CryptoBuffer::Assign(const OwningArrayBufferViewOrArrayBuffer& aData)
> >+    Assign(aData.GetAsArrayBufferView().Obj());
>
> As above.
>
> >+    SetLength(0);
>
> And here.

I'm not sure.  You reach this case if aData isn't set to a value, in which case, setting the array to length zero seems like a sensible semantic.  Otherwise, would it be sufficient to just pass a nullptr, which is the same behavior as if there were an allocation failure?


> >+SECItem CryptoBuffer::AsSECItem()
> >+  return {siBuffer, Elements(), Length()};
>
> This seems pretty dangerous.  What makes sure no one calls Assign() on us
> while someone else is working with that SECItem, leaving the pointer
> dangling?

Changed this to copy data, returning a fresh SECItem*.


> >+bool CryptoBuffer::GetBigIntValue(unsigned long& aRetVal)
>
> unsigned long is not very BigInt...
>
> This also seems unused; I'd have to look at uses to see whether this makes
> sense.

It will get used in Bug 998803 for RSA public exponents.  Which means we're really only interested in the values 0x03 and 0x010001.


> >+  for (size_t i=0; i < Length(); ++i) {
> >+    aRetVal = (aRetVal << 8) + ElementAt(i);
>
> So this assumes our data is big-endian, right?  Is that documented
> somewhere?  Note that typed array views are native-endian or
> little-endian....

WebCrypto BigIntegers are big-endian.
https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#big-integer

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

> We should consider opening bugs to fix those in the code generator.

Yes, please!

> As far as new exception types, I'm just following the spec...

Yes, I know.  Hence my question about the TAG.

> ... need a macro for this?

Actually I was going to ask why we have a separate class for implementing the actual functionality.

> Again, just following the spec here.  

Yes, I know.  The question was whether the spec makes sense here.  I'll send in a spec issue.

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

Er, going on...

> Don't know why. 

Because the example generator is just not that smart.  It's supposed to generate something to get people started, not production code.  But this particular issue we can fix.  Please file a bug.

> I *thought* that the changes here would address all the different ways that
> structured clone are used, including postMessage but also IndexedDB. 

Hmm.  So this will affect places that either pass no other callbacks or chain up to the default runtime callbacks for the main-thread runtime.

That does include window.postMessage and IndexedDB.  We should add tests for those too.

But it does not include workers (esp. because on the worker thread the JSRuntime is different).

> It wouldn't be terribly useful to send a Key to a worker, though

That's true, but I _think_ as the spec is currently written a basic round-trip though a worker is expected to work.  I agree it can be done in a followup, though.

> It's not clear to me what the desired semantic is.

See https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Adding_WebIDL_bindings_to_a_class item 2.  The idea is that walking the GetParentObject() chain should eventually terminate in a Window.

> You reach this case if aData isn't set to a value, in which
> case, setting the array to length zero seems like a sensible semantic. 

Sure.  But people shouldn't be passing an uninitialized union around.  So it seems perfectly reasonable to me to SetLength(0) just to be safe _and_ assert that your caller is broken.

> Which means we're really only interested in the values 0x03 and 0x010001.

OK.  Given that, is GetBigIntValue the right name? If that's because of the spec's usage of the term, it might be worth just commenting that.  That would also make the fact that the endianness comes from the spec clearer.

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

> Yes, please!

I filed bug 1000675.

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

Comment on attachment 8410707 [details] [diff] [review]
webcrypto-995385.3.patch

>+++ b/dom/crypto/CryptoBuffer.h
>+  nsString AsBase64url() const;

Doesn't exist.

>+++ b/dom/crypto/HmacKeyAlgorithm.cpp
>+#include "HmacKeyAlgorithm.h"

mozilla/dom

>+#include "nsContentUtils.h"

Not needed.

>+++ b/dom/crypto/HmacKeyAlgorithm.h
>+#ifndef mozilla_dom_HmacKeyAlgorithm_h__

The usual.

>+#include "mozilla/ErrorResult.h"

Not needed.

>+#include "nsCycleCollectionParticipant.h"

This _is_ needed, though.  And this class needs to implement cycle collection and cycle collect mHash, like the TODO comment says.

>+#include "nsWrapperCache.h"

Not needed.

>+#include "KeyAlgorithm.h"
>+#include "WebCryptoCommon.h"

mozilla/dom for both of those.

>+struct JSContext;

js/TypeDecls.h

>+  HmacKeyAlgorithm(const nsString& aName, const uint32_t& aLength, const nsString& aHash)

Passing aLength by value would make more sense, no?

>+    SetIsDOMBinding();

Superclass does this already.

>+  // TODO: return something sensible here, and change the return type
>+  HmacKeyAlgorithm* GetParentObject() const

Yes, please.

>+  already_AddRefed<KeyAlgorithm> Hash() const
>+  {
>+    nsRefPtr<KeyAlgorithm> hash = mHash;
>+    return hash.forget();

  KeyAlgorithm* Hash() const
  {
    return mHash;
  }

>+  virtual bool WriteStructuredClone(JSStructuredCloneWriter* aWriter) const

Again, this should perhaps go in the .cpp

>+  static KeyAlgorithm* Create(JSStructuredCloneReader* aReader) {

Curly on next line.

>+    JS_ReadUint32Pair(aReader, &length, &zero);

That can fail.

>+    nsString hash = KeyAlgorithm::ReadString(aReader);
>+    nsString name = KeyAlgorithm::ReadString(aReader);

And those should be able to.

>+++ b/dom/crypto/Key.cpp
>+#include "Key.h"
>+#include "WebCryptoCommon.h"

mozilla/dom for both of those.

>+#include "nsContentUtils.h"

Not needed.

>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(Key)

Need to CC mAlgorithm here, no?

>+void Key::GetType(nsString& aRetVal) const

Is there a reason KeyType is not an enum in the spec?

It's worth thinking about using c++ bitfields here, in a union with uint32_t (and a static assert that sizeof == 4) which might simplify some of this code.

Finally, this function and others in this file should put the return type on a separate line.

>+already_AddRefed<KeyAlgorithm> Key::Algorithm() const
>+{
>+  nsRefPtr<KeyAlgorithm> algorithm = mAlgorithm;
>+  return algorithm.forget();

  KeyAlgorithm*
  Key::Algorithm() const
  {
    return mAlgorithm;
  }

>+void Key::GetUsages(nsTArray<nsString>& aRetVal) const

KeyUsage seems like it should be an enum too....

>+Key::KeyType Key::GetKeyType() const

This seems like a complicated way of writing

  return KeyType(mAttributes & TYPE_MASK);

>+nsresult Key::SetType(const nsString& aType)
>+  if      (aType.EqualsLiteral(WEBCRYPTO_KEY_TYPE_SECRET)) {

Weird whitespace there.

>+  } else { return NS_ERROR_DOM_SYNTAX_ERR; }

And here.  Body should go on next line.

Do we still want to reset our type to UNKNOWN in addition to returning the error?

>+void Key::SetExtractable(bool aExtractable) {
>+  if (aExtractable) { mAttributes |= EXTRACTABLE; }

if body on next line.

>+nsresult Key::SetUsage(const nsString& aUsage)

Should this be called AddUsage instead?

>+void Key::SetUsage(Key::KeyUsage aUsage)

Likewise.

>+bool Key::HasUsageOtherThan(uint32_t aUsages)
>+  return !!(mAttributes & USAGES_MASK & !aUsages);

You mean ^aUsages?

>+SECKEYPrivateKey* Key::GetPrivateKey() const
>+  if (!mPrivateKey) { return nullptr; }

If body on next line.

>+  return SECKEY_CopyPrivateKey(mPrivateKey.get());

The header needs to document that this is returning a copy that the caller will need to free via SECKEY_DestroyPrivateKey if not null.

>+SECKEYPublicKey* Key::GetPublicKey() const
>+  if (!mPublicKey) { return nullptr; }
>+  return SECKEY_CopyPublicKey(mPublicKey.get());

Same for both lines here.

>+SECKEYPrivateKey* Key::PrivateKeyFromPkcs8(CryptoBuffer& aKeyData)

I really hope the other reviewer reviewed this bit.  Though it's worth a comment to explain why aKeyData is not const and another comment explaining the ownership model of the return value.

>+SECKEYPublicKey* Key::PublicKeyFromSpki(CryptoBuffer& aKeyData)

Likewise.

>+PK11SymKey* Key::SymKeyFromRaw(CryptoBuffer& aKeyData)
>+    PK11_ExtractKeyValue(symKey);

Document why that call is there (presumably for its side effects?) and why we do not care about its return value.

>+nsresult Key::SymKeyToRaw(PK11SymKey* aSymKey, CryptoBuffer& aRetVal)
>+  if (NS_FAILED(rv)) { return NS_ERROR_DOM_INVALID_ACCESS_ERR; }
>+  if (!keyItem) { return NS_ERROR_DOM_INVALID_ACCESS_ERR; }

If bodies on separate lines, please.

>+bool WriteBuffer(JSStructuredCloneWriter* aWriter, const CryptoBuffer& aBuffer)

Should this method be static?

>+    ret = ret && JS_WriteBytes(aWriter, aBuffer.Elements(), aBuffer.Length());

ret is known true before this line runs, so no need to put "ret &&" on the RHS.


>+bool ReadBuffer(JSStructuredCloneReader* aReader, CryptoBuffer& aBuffer)

And this one.

>+  bool ret = JS_ReadUint32Pair(aReader, &length, &zero);
>+  aBuffer.SetLength(length);
>+  if (length > 0) {

If ret is false, that's reading potentially uninitialized memory.  Just return early if !ret after JS_ReadUint32Pair, please.

>+bool Key::WriteStructuredClone(JSStructuredCloneWriter* aWriter) const
>+  CryptoBuffer sym, priv, pub;

"sym" is unused.

>+  if (mPrivateKey) { Key::PrivateKeyToPkcs8(mPrivateKey, priv); }
>+  if (mPublicKey) { Key::PublicKeyToSpki(mPublicKey, pub); }

If bodies on next line, please.

>+Key* Key::Create(JSStructuredCloneReader* aReader)
>+  Key* ret = new Key();
>+  JS_ReadUint32Pair(aReader, &attributes, &zero);

What if this fails?

>+  ReadBuffer(aReader, sym);
>+  ReadBuffer(aReader, priv);
>+  ReadBuffer(aReader, pub);

Likewise.

>+  if (!algorithm) {
>+    return nullptr;

That leaks "ret".  "ret" should probably be an nsAutoPtr, with forget() when you return it.

And "algorithm" should probably be nsRefPtr, for that matter.

>+  if (sym.Length() > 0)  { ret->mSymKey = sym; }
>+  if (priv.Length() > 0) { ret->mPrivateKey = Key::PrivateKeyFromPkcs8(priv); }
>+  if (pub.Length() > 0)  { ret->mPublicKey = Key::PublicKeyFromSpki(priv); }

If bodies on next line, but more importantly, can PrivateKeyFromPkcs8 and PublicKeyFromSpki fail and return null?

>+++ b/dom/crypto/Key.h
>+#ifndef mozilla_dom_Key_h__
>+#include "mozilla/ErrorResult.h"

You know the drill.

>+#include "nsPIDOMWindow.h"

Unused.

>+#include "KeyAlgorithm.h"

mozilla/dom

>+#include "CryptoBuffer.h"

mozilla/dom?

>+struct JSContext;

js/TypeDecls.h

>+  // TODO: return something sensible here, and change the return type

Please.

>+++ b/dom/crypto/KeyAlgorithm.cpp

Again, various functions with return types on same line here.

>+#include "KeyAlgorithm.h"

mozilla/dom

>+#include "nsContentUtils.h"

Not needed.

>+#include "WebCryptoTask.h"
>+#include "WebCryptoCommon.h"
>+#include "AesKeyAlgorithm.h"
>+#include "HmacKeyAlgorithm.h"
>+#include "RsaKeyAlgorithm.h"
>+#include "RsaHashedKeyAlgorithm.h"

mozilla/dom for all of those?

>+KeyAlgorithm::KeyAlgorithm(const nsString& aName)
>+  : mName(aName)
...
>+  mName.Assign(aName);

That's been done already.

>+  if (mName.EqualsLiteral(WEBCRYPTO_ALG_AES_CBC)) {

Really sort of wish mName were also a WebIDL enum here.  Then this whole thing would be data, not code.

>+KeyAlgorithm* KeyAlgorithm::Create(JSStructuredCloneReader* aReader)
>+  nsString name;

This is only needed in the SCTAG_KEYALG branch, no?

>+    default:
>+      algorithm = nullptr;

It's already nullptr

>+bool KeyAlgorithm::WriteBuffer(JSStructuredCloneWriter* aWriter, const CryptoBuffer& aBuffer) const
>+bool KeyAlgorithm::ReadBuffer(JSStructuredCloneReader* aReader, CryptoBuffer& aBuffer)

The same issues apply here as in the Key versions of these.  In fact, could we share them?

>+nsString KeyAlgorithm::ReadString(JSStructuredCloneReader* aReader)

As we said before, this should be able to fail.

>+  const nsString::char_type* name = new nsString::char_type[nameLength];

This buffer is leaked, no?

What you really want to do here, given an nsString& argument is call SetCapacity on it, then BeginWriting() to get a pointer to write to.

When you do that, watch out for null-termination!  nsString promises a trailing null, but you're not storing that null in the structured clone data.  Which is fine, but you'll need to set capacity to length+1 and null-terminate manually.

>+++ b/dom/crypto/KeyAlgorithm.h
>+#ifndef mozilla_dom_KeyAlgorithm_h__
>+#include "mozilla/ErrorResult.h"

The usual.

>+#include "CryptoBuffer.h"

mozilla/dom?

>+struct JSContext;

The usual.

>+++ b/dom/crypto/RsaHashedKeyAlgorithm.cpp
>+#include "RsaHashedKeyAlgorithm.h"
>+#include "nsContentUtils.h"

Usual comments.

>+++ b/dom/crypto/RsaHashedKeyAlgorithm.h
>+#ifndef mozilla_dom_RsaHashedKeyAlgorithm_h__
>+#include "mozilla/ErrorResult.h"

And here.

>+#include "nsWrapperCache.h"

Not needed.

>+#include "RsaKeyAlgorithm.h"

mozilla/dom

>+struct JSContext;

js/TypeDecls.h

>+    SetIsDOMBinding();

Superclass handles that.

>+  // TODO: return something sensible here, and change the return type

Yes, please.

>+  // Mark this as resultNotAddRefed to return raw pointers
>+  already_AddRefed<KeyAlgorithm> Hash() const

Just return the raw pointer.

>+  virtual bool WriteStructuredClone(JSStructuredCloneWriter* aWriter) const

Usual comment.

>+  static KeyAlgorithm* Create(JSStructuredCloneReader* aReader) {

Curly on next line and need to handle failures here.

>+  // TODO: Add cycle collection for this
>+  nsRefPtr<KeyAlgorithm> mHash;

Yes, that needs to happen.

>+++ b/dom/crypto/RsaKeyAlgorithm.cpp
>+#include "RsaKeyAlgorithm.h"
>+#include "nsContentUtils.h"

Usual comments.

>+++ b/dom/crypto/RsaKeyAlgorithm.h
>+#ifndef mozilla_dom_RsaKeyAlgorithm_h__
>+#include "mozilla/ErrorResult.h"

Usual comments.

>+#include "nsCycleCollectionParticipant.h"
>+#include "nsWrapperCache.h"
>+#include "nsAutoPtr.h"

All three not needed, afaict.

>+#include "KeyAlgorithm.h"

mozilla/dom

>+#include "RsaKeyAlgorithm.h"

This is including itself?  Nix that line, please.

>+struct JSContext;

js/TypeDecls.h

>+    SetIsDOMBinding();

Handled by the superclass.

>+  // TODO: return something sensible here, and change the return type

Please, yes.

>+  virtual bool WriteStructuredClone(JSStructuredCloneWriter* aWriter) const

Usual comment.

>+  static KeyAlgorithm* Create(JSStructuredCloneReader* aReader) {

Curly on next line?  And needs error handling.

>+  // TODO: Add cycle collection for this

No cycle collection needed here, as far as I can tell.

>+++ b/dom/crypto/WebCryptoCommon.h

I did not cross-check these strings against the spec, fwiw.  I assume they're correct and the spec will have test coverage that would catch any issues.

>+++ b/dom/crypto/WebCryptoTask.cpp
>+#include "Key.h"
>+#include "KeyAlgorithm.h"
>+#include "AesKeyAlgorithm.h"
>+#include "HmacKeyAlgorithm.h"
>+#include "RsaKeyAlgorithm.h"
>+#include "CryptoBuffer.h"
>+#include "WebCryptoCommon.h"

mozilla/dom on all of those

>+static nsresult GetAlgorithmName(JSContext* aCx, const T& aAlgorithm, nsString& aName)

Return type on separate line here and elsewhere in this file.

Also, aAlgorithm is just ObjectOrString, no?  Why is this a template?

>+    // Coerce to algorithm and extract name

I assume the spec will catch up here at some point, right?

> +    JS::RootedValue value(aCx, OBJECT_TO_JSVAL(aAlgorithm.GetAsObject()));

JS::RootedValue(aCx, JS::ObjectValue(*aAlgorithm.GetAsObject()));

>+      return NS_ERROR_DOM_SYNTAX_ERR;
>+    } else {

No else after return, please.

>+template<class T, class OOS>
>+static nsresult Coerce(JSContext* aCx, T& aTarget, const OOS& aAlgorithm)

aAlgorithm is always ObjectOrString, no?  Why are we templating on its type?

I did not cross-check this part against the spec, since the spec here will likely change.

>+  JS::RootedValue value(aCx, OBJECT_TO_JSVAL(aAlgorithm.GetAsObject()));

JS::ObjectValue.

>+    return NS_ERROR_DOM_SYNTAX_ERR;
>+  } else {

No else after return, please.

>+class ReturnArrayBufferViewTask : public WebCryptoTask
>+  virtual void Resolve() MOZ_OVERRIDE
>+    mResultPromise.forget();

How is that not a leak?  Did you mean to do "mResultPromise = nullptr" instead?

>+class ImportKeyTask : public WebCryptoTask
>+    for (uint32_t i=0; i<aKeyUsages.Length(); ++i) {

Spaces around '=' and '<', please.

>+  virtual void Cleanup() MOZ_OVERRIDE
>+    mKey.forget();

How is _this_ not a leak?  Again, did you mean "mKey = nullptr"?

>+class ImportSymmetricKeyTask : public ImportKeyTask

I did not verify that this is following the spec, fwiw, like most of this file.

>+      if (aKeyData.IsArrayBufferView()) {

Why can't you just use the ArrayBufferViewOrArrayBuffer overload of mKeyData.Assign?

>+      } else {
>+        mEarlyRv = NS_ERROR_DOM_DATA_ERR;

This will not happen if this code is being called from WebIDL bindings.

>+      return;
>+    } else {

Maybe comment that this else after return is here because we don't have a forced return in the first branch of the if cascade?

Or just reverse things so that the known-returning thing is first, of course.

>+      HmacImportParams params;

HmacImportParams has an AlgorithmIdentifier member, which is a union containing "object".  Which means this whole thing needs rooting.  You want RootedDictionary<HmacImportParams> params.

>+  virtual nsresult BeforeCrypto() MOZ_OVERRIDE
>+    KeyAlgorithm* algorithm;

nsRefPtr, please, and drop the manual delete below.

>+    uint32_t length = 8 * mKeyData.Length();

Document where the 8 comes from?

>+class ImportRsaKeyTask : public ImportKeyTask
>+    if (aKeyData.IsArrayBufferView()) {

Again, just mKeyData.Assign(aKeyData) seems like it would do the right thing.

>+      RsaHashedImportParams params;

RootedDictionary.

>+      mEarlyRv = Coerce(aCx, params, aAlgorithm);

Pretty similar code popping up here.  Not sure whether it's worth factoring out into a common method....

>+  virtual nsresult AfterCrypto() MOZ_OVERRIDE
>+    uint32_t modulusLength = 8 * mPubKey->u.rsa.modulus.len;

Again, document the 8?

>+      RsaHashedKeyAlgorithm* algorithm = new RsaHashedKeyAlgorithm(mAlgName, modulusLength,

nsRefPtr and drop the manual delete.

Also, watch the 80th column here?

>+class UnifiedExportKeyTask : public ReturnArrayBufferViewTask
>+  UnifiedExportKeyTask(const nsAString& aFormat, mozilla::dom::Key& aKey)

No need for mozilla::dom.

>+WebCryptoTask* WebCryptoTask::EncryptDecryptTask(JSContext* aCx,

Usual thing about return value on separate line for all of these.

>+WebCryptoTask* WebCryptoTask::ExportKeyTask(const nsAString& aFormat,
>+                 mozilla::dom::Key& aKey)

No need for mozilla::dom, and fix the weird indent.

>+++ b/dom/crypto/WebCryptoTask.h
>+#include "jsapi.h"
>+#include "jsfriendapi.h"

Why are these needed?

>+#include "Key.h"

mozilla/dom

>+The execution of a WebCryptoTask happens in several phases

This should probably mention Cleanup().

>+class WebCryptoTask : public CryptoTask

So this will spin up a separate thread for every single API call?  That seems like a DoS waiting to happen...  Can we file a followup to use some sort of threadpool here, perhaps?

>+  static WebCryptoTask* EncryptDecryptTask(JSContext* aCx,

This should probably be protected, right?

>+  static WebCryptoTask* SignVerifyTask(JSContext* aCx,

Likewise.

>+  void FailWithError(nsresult aRv)
>+    nsRefPtr<Exception> exception = DOMException::Create(aRv);
>+    mResultPromise->MaybeReject(exception);

Just mResultPromise->MaybeReject(aRv) should work now that bug 996831 has landed.

>+    mResultPromise.forget();

How is this not a leak?

>+  virtual void ReleaseNSSResources() MOZ_OVERRIDE {}

That seems wrong to me if I read the docs for CryptoTask correctly.  For example, ImportRsaKeyTask needs to drop its mPubKey in here, no?  Or drop it in DoCrypto or something...  Similar for UnifiedExportKeyTask.

>+  virtual nsresult CalculateResult() MOZ_OVERRIDE

And MOZ_FINAL?

>+  virtual void CallCallback(nsresult rv) MOZ_OVERRIDE

And MOZ_FINAL?

>+    mResultPromise.forget();

Another leak.

>+++ b/dom/crypto/moz.build
>+LOCAL_INCLUDES += [
>+    '/dom/base',

Why is that needed?

>+++ b/dom/crypto/test/mochitest.ini

I didn't really look at the tests.  Please let me know if you want me to (e.g. if no one else has reviewed them).

>+++ b/dom/crypto/test/tests.js
>+function exists(x) {
>+  return (x !== undefined) && (x !== null);

Why is this special-casing null?

We really need to fix all the leaks here.  :(

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

Comment on attachment 8411381 [details] [diff] [review]
webcrypto-995385.3-4.interdiff

Thank you for posring this!

>+++ b/dom/crypto/AesKeyAlgorithm.cpp
>+bool AesKeyAlgorithm::WriteStructuredClone(JSStructuredCloneWriter* aWriter) const

Return type on separate line.

>+KeyAlgorithm* AesKeyAlgorithm::Create(JSStructuredCloneReader* aReader)

And here.

>+  if (!read) { return nullptr; }
>+  if (!read) { return nullptr; }

And the if bodies.

Similar comments apply to HmacKeyAlgorithm, KeyAlgorithm, RsaHashedKeyAlgorithm, RsaKeyAlgorithm

>+++ b/dom/crypto/AesKeyAlgorithm.h
>   AesKeyAlgorithm(const nsString& aName, uint16_t aLength)
>     SetIsDOMBinding();

That call is still unnecessary.

>+++ b/dom/crypto/CryptoBuffer.cpp
>+uint8_t* CryptoBuffer::Assign(const ArrayBufferViewOrArrayBuffer& aData)
>+    return Assign(aData.GetAsArrayBufferView());
>   } else if (aData.IsArrayBuffer()) {
...
>+    return Assign(aData.GetAsArrayBuffer());
>   } else {

No else after return, here and in the owning version.

>+SECItem* CryptoBuffer::ToSECItem()
>+  item->data = new uint8_t[Length()];

Should that be a fallible allocation?

>+++ b/dom/crypto/KeyAlgorithm.cpp
> bool KeyAlgorithm::ReadBuffer(JSStructuredCloneReader* aReader, CryptoBuffer& aBuffer)
>+  if (ret && length > 0) {

If !ret we've already passed a garbage length to aBuffer.SetLength().  We really do need to check ret earlier.

>+bool KeyAlgorithm::ReadString(JSStructuredCloneReader* aReader, nsString& aString)

Return type on separate line?

>+  if (!read) { return false; }

And the return here.

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

> Thank you for posring this!

"posting".  That's what I get for commenting at 2am.  :(

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

(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #12)
> > ... need a macro for this?
> 
> Actually I was going to ask why we have a separate class for implementing
> the actual functionality.

The thought was to have a pretty clean interface between the WebIDL-facing part of things an the crypto part of things.  We could move the content of the static methods of WebCryptoTask up into SubtleCrypto, but that would require bringing along all the logic about demuxing by algorithm, constants, etc.  Rather than having the WebIDL know about all the different flavors of WebCryptoTasks, it seemed sensible to just give it a method to say "Give me the right one for these parameters".

The actual crypto stuff wants to be in a separate class so that it can run off the main thread by inheriting from CryptoTask.

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

(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #13)
> > I *thought* that the changes here would address all the different ways that
> > structured clone are used, including postMessage but also IndexedDB. 
> 
> Hmm.  So this will affect places that either pass no other callbacks or
> chain up to the default runtime callbacks for the main-thread runtime.
> 
> That does include window.postMessage and IndexedDB.  We should add tests for
> those too.
> 
> But it does not include workers (esp. because on the worker thread the
> JSRuntime is different).
> 
> > It wouldn't be terribly useful to send a Key to a worker, though
> 
> That's true, but I _think_ as the spec is currently written a basic
> round-trip though a worker is expected to work.  I agree it can be done in a
> followup, though.

I agree.  Considering this tabled for now.  Filed Bug 1001434.


> > It's not clear to me what the desired semantic is.
> 
> See
> https://developer.mozilla.org/en-US/docs/Mozilla/
> WebIDL_bindings#Adding_WebIDL_bindings_to_a_class item 2.  The idea is that
> walking the GetParentObject() chain should eventually terminate in a Window.

Ok.  Unless you tell me otherwise, I'm just going to pass the Window down from SubtleCrypto to any subsidiary objects, for objects created via SubtleCrypto.  Open to suggestions as to what to do for NS_DOMReadStructuredClone.  The other object handled there (ImageData) somehow gets away without a parent object.


> > You reach this case if aData isn't set to a value, in which
> > case, setting the array to length zero seems like a sensible semantic. 
> 
> Sure.  But people shouldn't be passing an uninitialized union around.  So it
> seems perfectly reasonable to me to SetLength(0) just to be safe _and_
> assert that your caller is broken.

Will do.


> > Which means we're really only interested in the values 0x03 and 0x010001.
> 
> OK.  Given that, is GetBigIntValue the right name? If that's because of the
> spec's usage of the term, it might be worth just commenting that.  That
> would also make the fact that the endianness comes from the spec clearer.

Will do.

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

Comment on attachment 8410707 [details] [diff] [review]
webcrypto-995385.3.patch

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

LGTM,,Minor issue, again you will need a full review from a PSM peer. (keeler back from pto monday)

::: dom/crypto/Key.cpp
@@ +66,5 @@
> +  return algorithm.forget();
> +}
> +
> +void Key::GetUsages(nsTArray<nsString>& aRetVal) const
> +{

usages will have no separator? ie
verifysingencyrpt (where those tree bits are unabled?)

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

> I'm just going to pass the Window down from SubtleCrypto to any subsidiary objects

Sounds great.  You may want to pass it as an nsIGlobalObject*; see below.

> Open to suggestions as to what to do for NS_DOMReadStructuredClone

Use xpc::GetNativeForGlobal(JS::CurrentGlobalOrNull(cx)) to get the global that the clone is happening into.

> The other object handled there (ImageData) somehow gets away without a parent object.

That's because it's not wrappercached.

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

Again, trimmed a bunch of things that I just implemented.

Note that the new interdiff is sequential with the old one; it does not include the changes from the first interdiff.

Pulling out a few significant questions:
* Need advice on how to destroy mResultPromise and other values to which JS also has a reference
* Need advice on whether we need to ReleaseNSSResources


> nsIGlobalObject *global = nsJSUtils::GetDynamicScriptGlobal(cx);
> if (!global) {
>   return nullptr;
> }

But global comes back as nullptr, apparently because JS::ContextOptionsRef(cx).privateIsNSISupports() returns false.  (I don't know how to intepret that.)  Suggestions for other ways to set a parent for the Key in this case very welcome.



(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #15)
> Comment on attachment 8410707 [details] [diff] [review]
> webcrypto-995385.3.patch
>
> >+void Key::GetType(nsString& aRetVal) const
>
> Is there a reason KeyType is not an enum in the spec?

There's an open spec bug to do this:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25198

But for now, we have strings.


> It's worth thinking about using c++ bitfields here, in a union with uint32_t
> (and a static assert that sizeof == 4) which might simplify some of this
> code.

Can we kick this to a separate bug?  Especially given that there seems to be some change likely in how types/usages get handled.  The complexity doesn't seem unbearable here, but I agree it could be worth looking at.


> >+void Key::GetUsages(nsTArray<nsString>& aRetVal) const
>
> KeyUsage seems like it should be an enum too....

No, usages can combine.  For example, you can have a symmetric key that has both ENCRYPT and DECRYPT usages.


> >+bool Key::HasUsageOtherThan(uint32_t aUsages)
> >+  return !!(mAttributes & USAGES_MASK & !aUsages);
>
> You mean ^aUsages?

No, I mean ~aUsages  :)


> >+SECKEYPrivateKey* Key::PrivateKeyFromPkcs8(CryptoBuffer& aKeyData)
>
> I really hope the other reviewer reviewed this bit.  Though it's worth a
> comment to explain why aKeyData is not const and another comment explaining
> the ownership model of the return value.

Added some notes to the header for all of the To/From methods.


> >+PK11SymKey* Key::SymKeyFromRaw(CryptoBuffer& aKeyData)
> >+    PK11_ExtractKeyValue(symKey);
>
> Document why that call is there (presumably for its side effects?) and why
> we do not care about its return value.

Yep, side effects.  By default, NSS hides the key octets away somewhere internal, so even though there's a key there, PK11_GetKeyData returns an empty SECItem.  PK11_ExtractKeyValue puts the key data into the right place so that PK11_GetKeyData behaves as you expect.  Added a comment to this effect.


> >+  if (sym.Length() > 0)  { ret->mSymKey = sym; }
> >+  if (priv.Length() > 0) { ret->mPrivateKey = Key::PrivateKeyFromPkcs8(priv); }
> >+  if (pub.Length() > 0)  { ret->mPublicKey = Key::PublicKeyFromSpki(priv); }
>
> If bodies on next line, but more importantly, can PrivateKeyFromPkcs8 and
> PublicKeyFromSpki fail and return null?

They can, but that's tolerated.  It's detected when you actually try to do something with the key.

I've added a check to make sure that the right kind of key is non-null, e.g., if the attributes indicate SECRET key, make sure that the symmetric key is non-null.  If that fails, return nullptr, which will trigger a DataCloneError, which seems like a correct outcome.


> >+static nsresult GetAlgorithmName(JSContext* aCx, const T& aAlgorithm, nsString& aName)
>
> Return type on separate line here and elsewhere in this file.
>
> Also, aAlgorithm is just ObjectOrString, no?  Why is this a template?

It can also be OwningObjectOrString.  Same for Coerce.


> >+class ReturnArrayBufferViewTask : public WebCryptoTask
> >+  virtual void Resolve() MOZ_OVERRIDE
> >+    mResultPromise.forget();
>
> How is that not a leak?  Did you mean to do "mResultPromise = nullptr"
> instead?

I honestly don't know the answer here.  The situation is:
1. SubtleCrypto::Foo creates a Promise and returns it to JS
2. SubtleCrypto::Foo creates a WebCryptoTask that resolves/rejects the Promise

I thought that mResultPromise.forget() was the idiom for "I don't care about this any more, so you can GC it if nobody else is holding a reference".  What should I be doing instead?

(This instance is actually unnecessary, becase mResultPromise is handled by WebCryptoTask.  But I still need to know the answer.)


> >+  virtual void Cleanup() MOZ_OVERRIDE
> >+    mKey.forget();
>
> How is _this_ not a leak?  Again, did you mean "mKey = nullptr"?

Same story as above.  Please tell me what to do.


> >+      if (aKeyData.IsArrayBufferView()) {
>
> Why can't you just use the ArrayBufferViewOrArrayBuffer overload of
> mKeyData.Assign?
>
> >+      } else {
> >+        mEarlyRv = NS_ERROR_DOM_DATA_ERR;
>
> This will not happen if this code is being called from WebIDL bindings.

This is forward-looking, being prepared for JWK support.  If that happens as currently proposed, aKeyData will have another type in addition to AB and ABV, so we'll need to check that the incoming data is AB or ABV, not the other thing.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=24963


> >+class WebCryptoTask : public CryptoTask
>
> So this will spin up a separate thread for every single API call?  That
> seems like a DoS waiting to happen...  Can we file a followup to use some
> sort of threadpool here, perhaps?

Filed Bug 1001691.


> >+  virtual void ReleaseNSSResources() MOZ_OVERRIDE {}
>
> That seems wrong to me if I read the docs for CryptoTask correctly.  For
> example, ImportRsaKeyTask needs to drop its mPubKey in here, no?  Or drop it
> in DoCrypto or something...  Similar for UnifiedExportKeyTask.

I'm not sure.

Keeler/Camilo: Is it bad for SECKEY_DestroyPrivateKey() and SECKEY_DestroyPublicKey() to be called after NSS shutdown?

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

> * Need advice on how to destroy mResultPromise and other values to which JS also has a
> reference

mResultPromise is an nsRefPtr.  nsRefPtr (and nsCOMPtr) are reference-counting helpers, similar to std::shared_ptr.

So the right way to "destroy" it is to simply have the nsRefPtr destructor be called: that will decrement the refcount.  The only time you need to manually do something is if cycles are involved or if you want to explicitly drop references for some reason.  The former should not be a problem since nothing except the event loop holds refs to the CryptoTask, and I don't see anything that would necessitate the latter here, but maybe I'm missing something.

> But global comes back as nullptr

nsJSUtils::GetDynamicScriptGlobal will only return non-null if the global is a Window.  Furthermore, the naming sucks: "dynamic" in this case means "wherever we first entered JS", not "whatever compartment our caller is in".  On the off chance you wanted the calling Window, you're want GetStaticScriptGlobal.  In any case, comment 21 is what we should really do when we don't have a Window to start with as a member of whatever object we're working with.  When we do, we should use the one we have.

> Can we kick this to a separate bug? 

Yes.

> No, usages can combine. 

Sure, but that doesn't mean they need to be strings instead of enums.  enum just means the set of valid values is fixed.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=25198 tracks this in the spec.  We should consider implementing ahead of spec if we're sure it will change, to avoid shipping behavior and then changing it...

> No, I mean ~aUsages  :)

Er, yes.  ;)

> trigger a DataCloneError, which seems like a correct outcome.

Yes, that's much much better than silently corrupting the clone!

> It can also be OwningObjectOrString.

Ah, I see.  That's pretty annoying, I agree.  :(

> I thought that mResultPromise.forget() was the idiom for "I don't care about this any
> more, so you can GC it if nobody else is holding a reference"

No, .forget() on nsRefPtr/nsCOMPtr is the idiom for "Give away your reference to someone else who will now be in charge of releasing it".  I was looking for an std::shared_ptr equivalent but there just isn't one...

If you just want to force-drop a reference, calling Release in the process, the idiom
would be "mResultPromise = nullptr;".

> This is forward-looking, being prepared for JWK support.

Worth a code comment.

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

> is to simply have the nsRefPtr destructor be called

Except in runnables, of course, because you have no idea which thread the destructor of the runnable will run on, as Richard pointed out to me on irc.

So in that case it totally makes sense to explicitly assign null when you know you're running on the correct thread (in this case the main thread).

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

(In reply to Richard Barnes [:rbarnes] from comment #22)
> Keeler/Camilo: Is it bad for SECKEY_DestroyPrivateKey() and
> SECKEY_DestroyPublicKey() to be called after NSS shutdown?

We spoke in person, but for posterity: I believe so. The concern is that the key db may be corrupted if it isn't shut down properly, and I think these functions can affect the db.

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

Ok, I think this patch covers all review comments to date.  Regarding the last two open technical issues:

* nsRefPtrs are now explicitly released using, e.g., "mResultPromise = null"
* Key implements nsNSSShutDownObject and WebCryptoTasks make appropriate use of ReleaseNSSResources

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

A small fix on top of 5.patch to:
* Make the patch work with the tip of mozilla-central (changing some macros)
* Change from nsJSUtils::GetDynamicScriptGlobal to xpc::GetNativeForGlobal

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

Comment on attachment 8415010 [details] [diff] [review]
webcrypto-995385.5.patch

>+++ b/dom/crypto/AesKeyAlgorithm.h
>+struct JSContext;

Not needed.

>+++ b/dom/crypto/CryptoBuffer.h
>+  template<class T>
>+  CryptoBuffer(const T& aData)

Please make that constructor explicit?  Yay C++ footguns.

>+++ b/dom/crypto/Key.cpp
>+Key::SetExtractable(bool aExtractable) {

Curly on next line.

>+  if (aExtractable) { mAttributes |= EXTRACTABLE; }

If body on next line.

>+void Key::SetSymKey(const CryptoBuffer& aSymKey)
>+{
>+  mSymKey = aSymKey;

So on OOM this will set an empty mSymKey.  Do things handle that safely?

>+Key::PublicKeyFromSpki(CryptoBuffer& aKeyData)
>+  SECKEYPublicKey* pubKey = SECKEY_ExtractPublicKey(spki.get());
>+  if (!pubKey) {
>+    return nullptr;
>+  }
>+
>+  return pubKey;

How about:

  return SECKEY_ExtractPublicKey(spki.get());

>+Key::Create(nsIGlobalObject* aGlobal, JSStructuredCloneReader* aReader)
>+  nsAutoPtr<Key> ret(new Key(aGlobal));

So this is OK now because we don't hand ret to anyone until the end of the method, right?  Worth documenting that.

>+    printf("\n\n Failed to read all attributes \n\n");

Remove before landing.

>+  if (!(((ret->GetKeyType() == SECRET) && (ret->mSymKey.Length() > 0)) ||
>+        ((ret->GetKeyType() == PRIVATE) && (ret->mPrivateKey)) ||
>+        ((ret->GetKeyType() == PUBLIC) && (ret->mPublicKey)))) {

If found this a bit hard to read because of all the parens...  How about this:

  if (!((ret->GetKeyType() == SECRET && ret->mSymKey.Length() > 0) ||
        (ret->GetKeyType() == PRIVATE && ret->mPrivateKey) ||
        (ret->GetKeyType() == PUBLIC && ret->mPublicKey))) {

?  I considered also distributing the '!' further down like so:

  if ((ret->GetKeyType() != SECRET || ret->mSymKey.Length() == 0) &&
      (ret->GetKeyType() != PRIVATE || !ret->mPrivateKey) &&
      (ret->GetKeyType() != PUBLIC || !ret->mPublicKey))) {

but I'm not sure that's easier to read with all the negatives...

>+++ b/dom/crypto/Key.h
>+#include "mozilla/dom/SubtleCryptoBinding.h"

Why is that include needed here?

>+++ b/dom/crypto/KeyAlgorithm.cpp
>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(KeyAlgorithm)

What about mGlobal?

>+++ b/dom/crypto/WebCryptoCommon.h
>+ReadBuffer(JSStructuredCloneReader* aReader, CryptoBuffer& aBuffer)
>+    aBuffer.SetLength(length);

That can fail, right?  If it does, need to return false from here, I'd think.

>+++ b/dom/crypto/WebCryptoTask.cpp
>+static nsresult GetAlgorithmName(JSContext* aCx, const OOS& aAlgorithm, nsString& aName)

Return type on separate line, please.

>+class ImportKeyTask : public WebCryptoTask
>+    for (uint32_t i=0; i < aKeyUsages.Length(); ++i) {

  i = 0

>+    mEarlyRv = GetAlgorithmName(aCx, aAlgorithm, mAlgName);

So I just realized there's a problem here.

GetAlgorithmName can throw on aCx.  But nothing here is communicating to the JS engine that an exception is getting thrown, so it will fail to notice that and later either fail some fatal asserts (in debug builds) or end up with weird uncatchable exceptions (in opt builds).

What needs to happen here is that every method that takes a JSContext* and calls out to untrusted JS on that JSContext (so for example anything that does GetAlgorithmName() or Coerce() needs to take an ErrorResult& argument which is passed in from the bindings code (by annotating the relevant methods as [Throws]) and throw errors on that argument as needed.  And we need to add test coverage for cases where Init() on the dictionaries here throws.

Alternately, we need to explicitly clear the pending exception on aCx on early returns from these constructors (presumably via some RAII helper; Console.cpp has one that we may want to factor out and share), since presumably we'll go ahead and reject our returned promise anyway.  Or I suppose you could put that helper in WebCryptoTask::ImportKeyTask for now.

One other thought: it might be good to add NS_IsMainThread() assertions to all the Resolve(), Cleanup(), BeforeCrypto(), AfterCrypto(), methods, just in case.  And a !NS_IsMainThread() assert for DoCrypto(), perhaps?

>+class ImportRsaKeyTask : public ImportKeyTask
>+  virtual nsresult AfterCrypto() MOZ_OVERRIDE
>+        delete algorithm;

I'm a bit bummed that compiles.  :(  Please take that line out.  Otherwise the ~nsRefPtr will try to call Release() on a deleted object, which is clearly bad.

>+++ b/dom/crypto/WebCryptoTask.h
>+  virtual void DispatchWithPromise(Promise* aResultPromise)

Assert that this is on the main thread.

>+  void FailWithError(nsresult aRv)

Likewise.

>+  virtual nsresult CalculateResult() MOZ_OVERRIDE MOZ_FINAL

And that this is not.

>+  virtual void CallCallback(nsresult rv) MOZ_OVERRIDE MOZ_FINAL

And that this is.

>+++ b/dom/crypto/test/tests.js
>+  return (x !== undefined) && (x !== null);

Again, why the null bit here?

r=me with those fixed.

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

(In reply to Boris Zbarsky [:bz] from comment #28)
> Comment on attachment 8415010 [details] [diff] [review]
> webcrypto-995385.5.patch

> >+void Key::SetSymKey(const CryptoBuffer& aSymKey)
> >+{
> >+  mSymKey = aSymKey;
> 
> So on OOM this will set an empty mSymKey.  Do things handle that safely?

Yes.  Recall that Key::GetSymKey() returns by copy, so callers will need to check that Length() > 0 at that point to make sure the latter copy worked.  Conveniently, that will also catch the failure of this copy.


> >+    mEarlyRv = GetAlgorithmName(aCx, aAlgorithm, mAlgName);
> 
> So I just realized there's a problem here.
> 
> Alternately, we need to explicitly clear the pending exception on aCx on
> early returns from these constructors (presumably via some RAII helper;
> Console.cpp has one that we may want to factor out and share), since
> presumably we'll go ahead and reject our returned promise anyway.  Or I
> suppose you could put that helper in WebCryptoTask::ImportKeyTask for now.

I think the latter one is more coherent with the spec, and really with the use of promises.  These WebCrypto methods should recognize that something has gone wrong, and pass the error type in the spec to Promise::MaybeReject (via FailWithError).

I've copied over ClearExcpetion from Console.cpp and added instances to GetAlgorithmName and Coerce to implement this behavior.  Also, added a test case to cover this case.

> One other thought: it might be good to add NS_IsMainThread() assertions to
> all the Resolve(), Cleanup(), BeforeCrypto(), AfterCrypto(), methods, just
> in case.  And a !NS_IsMainThread() assert for DoCrypto(), perhaps?

I've done this at the WebCryptoTask layer.  That seem sufficient to me, since that's the only way these methods get called.


> r=me with those fixed.

Thanks!

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

Comment on attachment 8415010 [details] [diff] [review]
webcrypto-995385.5.patch

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

Sorry for taking a while to get to this. I reviewed the use of NSS functionality. It looks good, but there are a couple of significant issues, so r- for now. I'll be around until ~5pm Pacific time, so if you have another version by then, I'll try to get another review done by the end of the day.

::: dom/crypto/Key.cpp
@@ +204,5 @@
> +  if (!aPrivateKey) {
> +    mPrivateKey = nullptr;
> +    return;
> +  }
> +  mPrivateKey = SECKEY_CopyPrivateKey(aPrivateKey);

So, normally this would need to be protected by an NSS shutdown check. However, since *at the moment* SetPrivateKey is only called inside a CryptoTask, this should be fine. However, we need to be careful about subsequently adding more calls to this. Consider adding a parameter that's a reference to an nsNSSShutDownPreventionLock as a proof that the check has been made.

@@ +214,5 @@
> +  if (!aPublicKey) {
> +    mPublicKey = nullptr;
> +    return;
> +  }
> +  mPublicKey = SECKEY_CopyPublicKey(aPublicKey);

Same here.

@@ +226,5 @@
> +
> +SECKEYPrivateKey*
> +Key::GetPrivateKey() const
> +{
> +  if (!mPrivateKey || isAlreadyShutDown()) {

I think we need to actually need to acquire an nsNSSShutDownPreventionLock here.

@@ +235,5 @@
> +
> +SECKEYPublicKey*
> +Key::GetPublicKey() const
> +{
> +  if (!mPublicKey || isAlreadyShutDown()) {

Same here.

@@ +257,5 @@
> +// Serialization and deserialization convenience methods
> +
> +SECKEYPrivateKey*
> +Key::PrivateKeyFromPkcs8(CryptoBuffer& aKeyData)
> +{

Unfortunately, for any of these functions in Key that call NSS functions, we still need to have the isAlreadyShutDown check (as far as I can tell, for example, some of them can be called during structured cloning, which isn't in a CryptoTask or other NSS shutdown-preventing mechanism, right?). This means they can't be static, either. Let me know if you want more elaboration on this. If it's the case that the function is always called where an nsNSSShutDownPreventionLock has already been acquired and the check has already been done, you can pass a reference to the lock like with nsNSSCertificateDB::getCertsFromPackage: https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCertificateDB.cpp#252 .

@@ +270,5 @@
> +  unsigned int usage = KU_ALL;
> +
> +  nsresult rv = MapSECStatus(PK11_ImportDERPrivateKeyInfoAndReturnKey(
> +                slot.get(), pkcs8Item.get(), nullptr, nullptr, false, false,
> +                usage, &privKey, nullptr));

nit: maybe indent these two lines two more spaces

@@ +312,5 @@
> +  // These don't matter; they're overridden later
> +  CK_MECHANISM_TYPE mechanism = CKM_SHA_1_HMAC;
> +  CK_ATTRIBUTE_TYPE type = CKA_SIGN;
> +  PK11SymKey* symKey = PK11_ImportSymKey(slot, mechanism, PK11_OriginUnwrap,
> +                           type, keyItem.get(), nullptr);

nit: indentation

::: dom/crypto/Key.h
@@ +86,5 @@
> +    WRAPKEY    = 0x00400000,
> +    UNWRAPKEY  = 0x00800000
> +  };
> +
> +public:

nit: "public:" is in here twice in a row - not sure it's necessary

::: dom/crypto/KeyAlgorithm.h
@@ +27,5 @@
> +  SCTAG_RSAKEYALG,
> +  SCTAG_RSAHASHEDKEYALG
> +};
> +
> +class KeyAlgorithm  : public nsISupports,

nit: extra space before ":"

::: dom/crypto/RsaHashedKeyAlgorithm.cpp
@@ +31,5 @@
> +         JS_WriteUint32Pair(aWriter, mModulusLength, 0) &&
> +         WriteBuffer(aWriter, mPublicExponent) &&
> +         WriteString(aWriter, hashName) &&
> +         WriteString(aWriter, mName);
> +

nit: unnecessary blank line

::: dom/crypto/WebCryptoTask.cpp
@@ +250,5 @@
> +      mKeyData.Assign(aKeyData.GetAsArrayBufferView());
> +    } else if (aKeyData.IsArrayBuffer()) {
> +      mKeyData.Assign(aKeyData.GetAsArrayBuffer());
> +    } else {
> +      // XXX This will need to be changed for JWK

maybe "TODO: bug <number>"?

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

Sorry, one more go-round.

keeler: Added a bunch of locks and ifAlreadyShutDown guards.  See what you think.

bz: Please approve the modifications to test_interfaces.html

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

Comment on attachment 8416704 [details] [diff] [review]
webcrypto-995385.6.patch

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

Great - r=me for the NSS parts (with comments addressed).

::: dom/crypto/Key.cpp
@@ +336,5 @@
> +{
> +  if (isAlreadyShutDown()) {
> +    return false;
> +  }
> +  nsNSSShutDownPreventionLock locker;

this goes before the isAlreadyShutDown() check :)

@@ +367,5 @@
> +{
> +  if (isAlreadyShutDown()) {
> +    return false;
> +  }
> +  nsNSSShutDownPreventionLock locker;

same here

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

Comment on attachment 8416704 [details] [diff] [review]
webcrypto-995385.6.patch

The test_interfaces bits look fine, but shouldn't we still return false if we fail to set the length in ReadBuffer?

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

One other thought.  CryptoBuffer is very copy-happy.  It's worth filing a followup to see if we can reduce the number of such copies and replace them with moves from one CryptoBuffer to another, assuming CryptoBuffer can hold large chunks of data.

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

* Addresses Comment 34 and Comment 35
* Adds dependency on Bug 1005375 to address some failures in try [1]
* Adds some #includes to address failures due to non-unified build (also from try [1]

[1] https://tbpl.mozilla.org/?tree=Try&rev=25e74c45ca8d

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

Minor tweaks to rebase on current mozilla-central.

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

Ensure that NSS is initialized for CryptoTasks

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

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

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

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

Comment on attachment 8423115 [details] [diff] [review]
nss-init-cryptotask.patch

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

This looks like the right thing to do, but I have a couple of concerns to address before we call it good.

::: security/manager/ssl/src/CryptoTask.cpp
@@ +32,5 @@
> +
> +  // Can't add 'this' as the event to run, since mThread may not be set yet
> +  rv = NS_NewThread(getter_AddRefs(mThread),
> +                             nullptr,
> +                             nsIThreadManager::DEFAULT_STACK_SIZE);

nit: indentation (and, in fact, I think nullptr can go on the line before where it is)

@@ +33,5 @@
> +  // Can't add 'this' as the event to run, since mThread may not be set yet
> +  rv = NS_NewThread(getter_AddRefs(mThread),
> +                             nullptr,
> +                             nsIThreadManager::DEFAULT_STACK_SIZE);
> +  if (NS_SUCCEEDED(rv)) {

nit: handle the exceptional case in the if body (so "if (NS_FAILED(rv)) { return rv; }" but not all on one line)
(Looks like the original code did things the way it is now, but this is better.)

@@ +34,5 @@
> +  rv = NS_NewThread(getter_AddRefs(mThread),
> +                             nullptr,
> +                             nsIThreadManager::DEFAULT_STACK_SIZE);
> +  if (NS_SUCCEEDED(rv)) {
> +    NS_SetThreadName(mThread, taskThreadName);

Can NS_SetThreadName fail? We should handle that if so.

::: security/manager/ssl/src/CryptoTask.h
@@ +54,3 @@
>    }
>  
> +  nsresult Dispatch(const nsACString& taskThreadName);

I wonder if we should ensure this version of Dispatch also limits thread names to <= 15 chars.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +128,5 @@
>  bool nsPSMInitPanic::isPanic = false;
>  
> +// This function can be called from chrome or content processes
> +// to ensure that NSS is initialized.
> +bool InitNSS()

What was the problem with calling this from PeerConnectionImpl.cpp?

@@ +130,5 @@
> +// This function can be called from chrome or content processes
> +// to ensure that NSS is initialized.
> +bool InitNSS()
> +{
> +  if (NSS_IsInitialized()) {

I think it may be possible for this to return true when the rest of PSM init hasn't completed, which could be bad. Since getting the PSM service is idempotent, why don't we unconditionally do that if we're the parent process, and move this call to the child process case (instead of the static bool that's currently there).

@@ +136,5 @@
> +  }
> +
> +  nsresult rv;
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +    nsCOMPtr<nsISupports> nssDummy = do_GetService(PSM_COMPONENT_CONTRACTID, &rv);

nit: is this line <=80 chars?

@@ +142,5 @@
> +      return false;
> +    }
> +
> +    return true;
> +  } else {

nit: no else after return

@@ +147,5 @@
> +    if (!NS_IsMainThread()) {
> +      return false;
> +    }
> +
> +    static bool nssStarted = false;

If we have the call to NSS_IsInitialized, why do we need this? (Oh, I see - it's from PeerConnectionImpl.cpp. I don't think we need it here.)

::: security/manager/ssl/src/nsNSSComponent.h
@@ +60,5 @@
>    nssEnsure = 100,
>    nssEnsureOnChromeOnly = 101
>  };
>  
> +extern bool InitNSS();

Maybe something like "EnsureNSSInitializedFromChromeOrContent" would be a better name (although that's quite a mouthful).

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

Fixed a bunch of the straightforward stuff.

(In reply to David Keeler (:keeler) [needinfo? is a good way to get my attention] from comment #40)
> Comment on attachment 8423115 [details] [diff] [review]
> nss-init-cryptotask.patch
> 
> Review of attachment 8423115 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +34,5 @@
> > +  rv = NS_NewThread(getter_AddRefs(mThread),
> > +                             nullptr,
> > +                             nsIThreadManager::DEFAULT_STACK_SIZE);
> > +  if (NS_SUCCEEDED(rv)) {
> > +    NS_SetThreadName(mThread, taskThreadName);
> 
> Can NS_SetThreadName fail? We should handle that if so.

Nope.  Returns void.


> ::: security/manager/ssl/src/nsNSSComponent.cpp
> @@ +128,5 @@
> >  bool nsPSMInitPanic::isPanic = false;
> >  
> > +// This function can be called from chrome or content processes
> > +// to ensure that NSS is initialized.
> > +bool InitNSS()
> 
> What was the problem with calling this from PeerConnectionImpl.cpp?

Adding #include "nsNSSComponent.h" caused a file not found error, and it wasn't immediately obvious to me how to fix it.  This should be a temporary condition, easily solved by someone familiar with how things work inside /media/.

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

Comment on attachment 8424177 [details] [diff] [review]
nss-init-cryptotask.1.patch

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

In terms of getting PeerConnectionImpl.cpp to use this, it looks like you'll have to declare EnsureNSSInitializedChromeOrContent in security/manager/ssl/src/PublicSSL.h, like with InitializeCipherSuite: https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/PublicSSL.h#19
Looks like we're halfway there again :/

::: security/manager/ssl/src/CryptoTask.cpp
@@ +21,5 @@
>  
> +nsresult
> +CryptoTask::Dispatch(const nsACString& taskThreadName)
> +{
> +  nsresult rv;

nit: declare this where it's used

@@ +32,5 @@
> +
> +  // Can't add 'this' as the event to run, since mThread may not be set yet
> +  rv = NS_NewThread(getter_AddRefs(mThread), nullptr,
> +                    nsIThreadManager::DEFAULT_STACK_SIZE);
> +  if (NS_SUCCEEDED(rv)) {

I guess I wasn't clear - this should be:

if (NS_FAILED(rv)) {
  return rv;
}

NS_SetThreadName...

@@ +33,5 @@
> +  // Can't add 'this' as the event to run, since mThread may not be set yet
> +  rv = NS_NewThread(getter_AddRefs(mThread), nullptr,
> +                    nsIThreadManager::DEFAULT_STACK_SIZE);
> +  if (NS_SUCCEEDED(rv)) {
> +    NS_SetThreadName(mThread, taskThreadName);

I suppose I could have investigated if this could return failure for myself :)

@@ +35,5 @@
> +                    nsIThreadManager::DEFAULT_STACK_SIZE);
> +  if (NS_SUCCEEDED(rv)) {
> +    NS_SetThreadName(mThread, taskThreadName);
> +    // Note: event must not null out mThread!
> +    rv = mThread->Dispatch(this, NS_DISPATCH_NORMAL);

We can probably just "return mThread->Dispatch..." here (since if you make the above change this will only be inner to the scope of the function, not an if block).

@@ +41,5 @@
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +  }
> +  return rv;

... and then all of this isn't necessary anymore

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

Comment on attachment 8424216 [details] [diff] [review]
nss-init-cryptotask.2.patch

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

Great! Is there a bug on moving webrtc to using this? Please file one if not.

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

Jeff, this is the patch I was mentioning on IRC earlier.  I looked through it again, and think it's safe; it'll just need a ComputeLengthAndData in two places in CryptoBuffer.  But figured you should keep an eye on it.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ea38414bb31
https://hg.mozilla.org/integration/mozilla-inbound/rev/43114cb99480

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

https://hg.mozilla.org/mozilla-central/rev/7ea38414bb31
https://hg.mozilla.org/mozilla-central/rev/43114cb99480