[webcrypto] Add error messages for failed operations.

content/renderer/webcrypto/webcrypto_util.h

Index: content/renderer/webcrypto/webcrypto_util.h
diff --git a/content/renderer/webcrypto/webcrypto_util.h b/content/renderer/webcrypto/webcrypto_util.h
index e3ea94b865ecce0d1dd84aa6c67265059fcad7b4..acda3581cdb44a66a9d9e19db680857817dae14f 100644
--- a/content/renderer/webcrypto/webcrypto_util.h
+++ b/content/renderer/webcrypto/webcrypto_util.h
@@ -17,6 +17,58 @@ namespace content {
 
 namespace webcrypto {
 
+// |Status| indicates whether an operation completed successfully, or with an

[Ryan Sleevi, 2014/01/24 01:21:50]

nit: No | | - this is not a variable, it's a class
nit: "an operation" is a bit vague, even though it's in content::webcrypto.

// Status holds the result of a WebCrypto operation. For failing operations, it
carries additional details explaining why the operation failed.

design: Why have you designed this akin to std::exception(), when the majority
of Chromium code uses enums? This seems more of a Blink-ism than a Chromium-ism.

[eroman, 2014/01/24 03:19:21]

Shrug. There are examples of both models in Chromium. URLRequestStatus is the
first one that comes to mind.

I chose a class rather than an enum because I prefer the encapsulation of having
an object:

  * Utility functions like "IsError/IsSuccess/ToString" are scoped to the class
  * Room to add extra parameters for finer granularity error messages.
  * Provides a central point to histogram errors, attach a debugger

When I was playing around debugging unittests I shoved the NSS error code into
the Status object, as well as the stacktrace of where the error was created
from. Was useful when debugging

My current preference is for a class here, but I am not strongly committed to
that approach.

[Ryan Sleevi, 2014/01/24 20:27:01]

I didn't have strong objections, I just wanted to understand the design more. In
particular, I wasn't sure if you were expecting the callers extend this via
adding new static creators (much in the same way you'd add new enum values) or
perhaps derive from this (as done on the JS exceptions, AIUI, and similar to
std::exception()).

The "just a number" approach is 'nice' in that you can reserve ranges for
implementation-specific details (eg: 1000 - 2000 = some component foo), whereas
with the design as written, you'd either need to change to returning a
heap-allocated object (so you can avoid type slicing) or you'd have to extend
this base object.

Again, to be clear, not an objection to the design, just trying to get better
clarification.

+// error.
+class CONTENT_EXPORT Status {
+ public:
+  bool IsError() const;
+  bool IsSuccess() const;
+
+  // Returns a UTF-8 error message (non-localized) describing the error.
+  std::string ToString() const;
+
+  static Status Success();
+  static Status Error();
+
+  static Status ErrorEmptyKeyData();
+  static Status ErrorEmptyData();
+  static Status ErrorEmptySignatureData();
+  static Status ErrorUnexpectedKeyType();
+
+  static Status ErrorJwkNotDictionary();
+  static Status ErrorJwkMissingKty();
+  static Status ErrorJwkExtractableInconsistent();
+  static Status ErrorJwkUnrecognizedAlgorithm();
+  static Status ErrorJwkAlgorithmInconsistent();
+  static Status ErrorJwkAlgorithmMissing();
+  static Status ErrorJwkUnrecognizedUsage();
+  static Status ErrorJwkUsageInconsistent();
+  static Status ErrorJwkDecodeK();
+  static Status ErrorJwkDecodeN();
+  static Status ErrorJwkDecodeE();
+  static Status ErrorJwkRsaPrivateKeyUnsupported();
+  static Status ErrorJwkUnrecognizedKty();
+
+  static Status ErrorIncorrectSizedIv();
+  static Status ErrorDataTooBig();
+  static Status ErrorUnsupported();
+  static Status ErrorUnexpected();
+  static Status ErrorInvalidAesGcmTagLength();
+  static Status ErrorPublicExponent();
+  static Status ErrorMissingAlgorithmRawKey();
+  static Status ErrorEmptyModulus();
+  static Status ErrorEmptyExponent();
+  static Status ErrorKeyNotExtractable();
+  static Status ErrorKeyLength();

[Ryan Sleevi, 2014/01/24 20:27:01]

At the risk of being a real PITA, can I ask you to document some of these?

For example, if importing a JWK, and the modulus is empty, is it an
ErrorJwkDecodeN or an ErrorEmptyModulus?

[eroman, 2014/01/25 00:53:10]

Done. I have added comments for each of the errors, this is probably the easiest
place for you to start the error review.

In the process I noticed some inconsistencies so did some renaming, an also:

 * Got rid of ErrorEmptyData() --> instead encrypt/decrypt will fail with a
generic Error. (Seemed arbitrary which cases did this and which didn't).

 * Got rid of ErrorEmptySignatureData() --> I don't believe it was consistent
for this to be an error in the first place, since other truncated inputs would
result in a success + verification failure. Changed to that behavior, and added
some corresponding tests.

+
+ private:
+  // |error_details_utf8| can be NULL to indicate there was no error.
+  // Otherwise it is a UTF-8 string literal (the pointer must remain valid).
+  explicit Status(const char* error_details_utf8);
+
+  const char* error_details_;
+};
+
 // Returns a pointer to the start of |data|, or NULL if it is empty. This is a
 // convenience function for getting the pointer, and should not be used beyond
 // the expected lifetime of |data|.