Add sanity check to generateKey to make sure usages aren't empty

content/child/webcrypto/algorithm_dispatch.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/child/webcrypto/algorithm_dispatch.h"
 
 #include "base/logging.h"
 #include "content/child/webcrypto/algorithm_implementation.h"
 #include "content/child/webcrypto/algorithm_registry.h"
 #include "content/child/webcrypto/crypto_data.h"
@@ skipping 93 matching lines @@
 
 Status GenerateKey(const blink::WebCryptoAlgorithm& algorithm,
                    bool extractable,
                    blink::WebCryptoKeyUsageMask usages,
                    GenerateKeyResult* result) {
   const AlgorithmImplementation* impl = NULL;
   Status status = GetAlgorithmImplementation(algorithm.id(), &impl);
   if (status.IsError())
     return status;
 
-  return impl->GenerateKey(algorithm, extractable, usages, result);
+  status = impl->GenerateKey(algorithm, extractable, usages, result);
+  if (status.IsSuccess()) {

[eroman, 2014/12/05 19:41:55]

style: Can you write this in the opposite direction:

if (status.IsError())
  return status;

That is the style used through-out the rest of the code, and I find it is easier
to follow the code flow when you use early-returns (fire and forget so to
speak!)

[nharper, 2014/12/05 23:15:47]

Done.

+    const blink::WebCryptoKey* key = nullptr;

[eroman, 2014/12/05 19:41:55]

Does this compile on all platforms? Not sure that nullptr can be used in
Chromium yet, but I may be wrong (using NULL for the rest of webcrypto code).

[nharper, 2014/12/05 23:15:47]

Codesearch shows nullptr used in thousands of places (some are comments) in
chromium, including use in the quic code. If the style in the webcrypto code is
to use NULL instead, I'm fine with that.

+    if (result->type() == GenerateKeyResult::TYPE_SECRET_KEY) {
+      key = &result->secret_key();
+    }
+    if (result->type() == GenerateKeyResult::TYPE_PUBLIC_PRIVATE_KEY_PAIR) {
+      key = &result->private_key();
+    }
+    if (key != nullptr) {
+      // This should only fail if an algorithm is implemented incorrectly and
+      // does not do its own check of the usages.
+      DCHECK(key->usages() != 0)

[eroman, 2014/12/05 20:52:45]

also: DCHECK_NE(0, key->usages());

[nharper, 2014/12/05 23:15:47]

Since I need to do other things when the DCHECK condition is met, I ended up
changing it to
if (key->usages() == 0) {
  DCHECK(false)...
  do other things
}

+          << "Key usages for generateKey() must not be empty";

[eroman, 2014/12/05 19:41:55]

Great. Can you also return an ErrorUnexpected here? (DCHECK only triggers in
developpers build, so the return would catch badness from propagating in the
release builds).

[nharper, 2014/12/05 23:15:47]

Done.

+    }
+  }
+  return status;
 }
 
 Status ImportKey(blink::WebCryptoKeyFormat format,
                  const CryptoData& key_data,
                  const blink::WebCryptoAlgorithm& algorithm,
                  bool extractable,
                  blink::WebCryptoKeyUsageMask usages,
                  blink::WebCryptoKey* key) {
   const AlgorithmImplementation* impl = NULL;
   Status status = GetAlgorithmImplementation(algorithm.id(), &impl);
@@ skipping 163 matching lines @@
     return false;
 
   status = impl->DeserializeKeyForClone(algorithm, type, extractable, usages,
                                         key_data, key);
   return status.IsSuccess();
 }
 
 }  // namespace webcrypto
 
 }  // namespace content