WebCrypto: Add interfaces for importKey().

Source/modules/crypto/SubtleCrypto.cpp

Index: Source/modules/crypto/SubtleCrypto.cpp
diff --git a/Source/modules/crypto/SubtleCrypto.cpp b/Source/modules/crypto/SubtleCrypto.cpp
index e0e75ccb25244f1a819295c82288f3ccc6ddaa04..247d3eff0938c14827c383217fe35d0cc89b1f58 100644
--- a/Source/modules/crypto/SubtleCrypto.cpp
+++ b/Source/modules/crypto/SubtleCrypto.cpp
@@ -31,107 +31,132 @@
 #include "config.h"
 #include "modules/crypto/SubtleCrypto.h"
 
+#include "V8Key.h" // NOTE: This must appear before ScriptPromiseResolver to define toV8()
+#include "bindings/v8/ScriptPromiseResolver.h"
 #include "core/dom/ExceptionCode.h"
 #include "modules/crypto/CryptoOperation.h"
+#include "modules/crypto/Key.h"
 #include "modules/crypto/NormalizeAlgorithm.h"
 #include "public/platform/Platform.h"
-#include "public/platform/WebArrayBuffer.h" // FIXME: temporary
 #include "public/platform/WebCrypto.h"
-#include "wtf/ArrayBuffer.h"
 #include "wtf/ArrayBufferView.h"
-#include "wtf/SHA1.h" // FIXME: temporary
-
 
 namespace WebCore {
 
 namespace {
 
-// FIXME: The following are temporary implementations of what *should* go on the
-//        embedder's side. Since SHA1 is easily implemented, this serves as
-//        a useful proof of concept to get layout tests up and running and
-//        returning correct results, until the embedder's side is implemented.
-//------------------------------------------------------------------------------
-class DummyOperation : public WebKit::WebCryptoOperation {
-public:
-    explicit DummyOperation(WebKit::WebCryptoOperationResult* result) : m_result(result) { }
-
-    virtual void process(const unsigned char* bytes, size_t size) OVERRIDE
-    {
-        m_result->completeWithError();
-        delete this;
-    }
-
-    virtual void abort() OVERRIDE
-    {
-        delete this;
+// FIXME: Temporary
+PassRefPtr<CryptoOperation> doDummyOperation(const Dictionary& rawAlgorithm, AlgorithmOperation operationType, ExceptionCode& ec)

[abarth-chromium, 2013/07/23 06:22:07]

Like "get", "do" is a weak verb.  Perhaps "dummyOperation"?

+{
+    WebKit::WebCrypto* platformCrypto = WebKit::Platform::current()->crypto();
+    if (!platformCrypto) {
+        ec = NotSupportedError;
+        return 0;
     }
 
-    virtual void finish() OVERRIDE
-    {
-        m_result->completeWithError();
-        delete this;
-    }
+    WebKit::WebCryptoAlgorithm algorithm;
+    if (!normalizeAlgorithm(rawAlgorithm, operationType, algorithm, ec))
+        return 0;
 
-protected:
-    WebKit::WebCryptoOperationResult* m_result;
-};
+    RefPtr<CryptoOperation> op = CryptoOperation::create(algorithm, &ec);
+    platformCrypto->digest(algorithm, op.get());
+    return op.release();
+}
 
-class MockSha1Operation : public DummyOperation {
+class KeyOperation : public WebKit::WebCryptoKeyOperationResult {
 public:
-    explicit MockSha1Operation(WebKit::WebCryptoOperationResult* result) : DummyOperation(result) { }
-
-    virtual void process(const unsigned char* bytes, size_t size) OVERRIDE
+    KeyOperation(ScriptPromiseResolver* resolver, ExceptionCode* ec)
+        : m_state(Initializing)
+        , m_impl(0)
+        , m_promiseResolver(resolver)
+        , m_exceptionCode(ec)
     {
-        m_sha1.addBytes(bytes, size);
     }
 
-    virtual void finish() OVERRIDE
-    {
-        Vector<uint8_t, 20> hash;
-        m_sha1.computeHash(hash);
+    ~KeyOperation();
 
-        WebKit::WebArrayBuffer buffer = WebKit::WebArrayBuffer::create(hash.size(), 1);
-        memcpy(buffer.data(), hash.data(), hash.size());
-
-        m_result->completeWithArrayBuffer(buffer);
-        delete this;
-    }
+    // Implementation of WebKit::WebCryptoKeyOperationResult.
+    virtual void initializationFailed() OVERRIDE;
+    virtual void initializationSucceeded(WebKit::WebCryptoKeyOperation*) OVERRIDE;
+    virtual void completeWithError() OVERRIDE;
+    virtual void completeWithKey(const WebKit::WebCryptoKey&) OVERRIDE;
 
 private:
-    SHA1 m_sha1;
+    enum State {
+        Initializing,
+        InProgress,
+        Done,
+    };
+
+    State m_state;
+    WebKit::WebCryptoKeyOperation* m_impl;
+    ExceptionCode* m_exceptionCode;

[abarth-chromium, 2013/07/23 06:22:07]

I don't understand how KeyOperation can hold a pointer to m_exceptionCode.  The
ExceptionCode is a stack-allocated object, which means it dies after the stack
unwinds.  The KeyOperation appears to live in the heap and complete its work
asynchronously.  That seems to imply that KeyOperation outlives m_exceptionCode
and therefore cannot keep a raw pointer to m_exceptionCode.

[eroman, 2013/07/23 06:53:41]

It is a little bit tricky, but the m_exceptionCode pointer is cleared before it
goes out of scope.

Upon calling into the Platform key operation, it either calls:
   initializationSucceeded(), initializationFailed(), completeWithKey() or
completeWithError(). These all drop the pointer.

I could instead have them buffer the exception code, and then call another
function to explicitly set it.

[abarth-chromium, 2013/07/23 07:06:14]

That's too tricky.  Is there a simpler design?

+    RefPtr<ScriptPromiseResolver> m_promiseResolver;
 };
 
-class MockPlatformCrypto : public WebKit::WebCrypto {
-public:
-    virtual void digest(const WebKit::WebCryptoAlgorithm& algorithm, WebKit::WebCryptoOperationResult* result) OVERRIDE
-    {
-        if (algorithm.id() == WebKit::WebCryptoAlgorithmIdSha1) {
-            result->initializationSucceded(new MockSha1Operation(result));
-        } else {
-            // Don't fail synchronously, since existing layout tests rely on
-            // digest for testing algorithm normalization.
-            result->initializationSucceded(new DummyOperation(result));
-        }
+KeyOperation::~KeyOperation()
+{
+    // Abort any inprogress operation.
+    switch (m_state) {
+    case Initializing:
+        ASSERT_NOT_REACHED();
+        break;
+    case InProgress:
+        // This will cause m_impl to be deleted.
+        m_state = Done;
+        m_impl->abort();
+        m_impl = 0;
+    case Done:
+        ASSERT(!m_impl);
+        break;
     }
-};
+}
 
-WebKit::WebCrypto* mockPlatformCrypto()
+void KeyOperation::initializationFailed()
 {
-    DEFINE_STATIC_LOCAL(MockPlatformCrypto, crypto, ());
-    return &crypto;
+    ASSERT(m_state == Initializing);
+
+    *m_exceptionCode = NotSupportedError;
+
+    m_exceptionCode = 0;
+    m_state = Done;
+    delete this;

[abarth-chromium, 2013/07/23 06:22:07]

This line is suspicious and indicates a bad memory management pattern...  Is
there a better way to structure the ownership relationships between these
objects?

 }
 
-PassRefPtr<CryptoOperation> doDummyOperation(const Dictionary& rawAlgorithm, AlgorithmOperation operationType, ExceptionCode& ec)
+void KeyOperation::initializationSucceeded(WebKit::WebCryptoKeyOperation* operationImpl)
 {
-    WebKit::WebCryptoAlgorithm algorithm;
-    if (!normalizeAlgorithm(rawAlgorithm, operationType, algorithm, ec))
-        return 0;
+    ASSERT(m_state == Initializing);
+    ASSERT(operationImpl);
+    ASSERT(!m_impl);
 
-    RefPtr<CryptoOperation> op = CryptoOperation::create(algorithm, &ec);
-    op->initializationSucceded(new DummyOperation(op.get()));
-    return op.release();
+    m_exceptionCode = 0;
+    m_impl = operationImpl;
+    m_state = InProgress;
+}
+
+void KeyOperation::completeWithError()
+{
+    ASSERT(m_state == Initializing || m_state == InProgress);
+
+    m_exceptionCode = 0;
+    m_impl = 0;
+    m_state = Done;
+
+    m_promiseResolver->reject(ScriptValue::createNull());
+    delete this;
+}
+
+void KeyOperation::completeWithKey(const WebKit::WebCryptoKey& key)
+{
+    ASSERT(m_state == Initializing || m_state == InProgress);
+
+    m_exceptionCode = 0;
+    m_impl = 0;
+    m_state = Done;
+
+    m_promiseResolver->fulfill(Key::create(key));
+    delete this;
 }
-//------------------------------------------------------------------------------
 
 } // namespace
 
@@ -162,7 +187,7 @@ PassRefPtr<CryptoOperation> SubtleCrypto::verifySignature(const Dictionary& rawA
 
 PassRefPtr<CryptoOperation> SubtleCrypto::digest(const Dictionary& rawAlgorithm, ExceptionCode& ec)
 {
-    WebKit::WebCrypto* platformCrypto = mockPlatformCrypto();
+    WebKit::WebCrypto* platformCrypto = WebKit::Platform::current()->crypto();
     if (!platformCrypto) {
         ec = NotSupportedError;
         return 0;
@@ -177,4 +202,45 @@ PassRefPtr<CryptoOperation> SubtleCrypto::digest(const Dictionary& rawAlgorithm,
     return op.release();
 }
 
+ScriptObject SubtleCrypto::importKey(const String& rawFormat, ArrayBufferView* keyData, const Dictionary& rawAlgorithm, bool extractable, const Vector<String>& rawKeyUsages, ExceptionCode& ec)
+{
+    WebKit::WebCrypto* platformCrypto = WebKit::Platform::current()->crypto();
+    if (!platformCrypto) {
+        ec = NotSupportedError;
+        return ScriptObject();
+    }
+
+    WebKit::WebCryptoKeyUsageMask keyUsages;
+    if (!Key::parseUsageMask(rawKeyUsages, keyUsages)) {
+        ec = TypeError;
+        return ScriptObject();
+    }
+
+    WebKit::WebCryptoKeyFormat format;
+    if (!Key::parseFormat(rawFormat, format)) {
+        ec = TypeError;
+        return ScriptObject();
+    }
+
+    WebKit::WebCryptoAlgorithm algorithm;
+    if (!normalizeAlgorithmForImportKey(rawAlgorithm, algorithm, ec))
+        return ScriptObject();
+
+    const unsigned char* keyDataBytes = static_cast<unsigned char*>(keyData->baseAddress());
+
+    RefPtr<ScriptPromiseResolver> promiseResolver = ScriptPromiseResolver::create();
+
+    // The |op| object is deleted upon completion of the underlying operation
+    // (i.e. when platformCrypto->importKey() notifies completion).
+    //
+    // FIXME: KeyOperation is never aborted. It should probably be aborted when
+    // the SubtleCrypto object that started it gets deleted. The concern being
+    // if the operation eventually does complete, the ScriptPromiseResolver
+    // might no longer be valid because the context it belonged to got torn
+    // down.
+    KeyOperation* op = new KeyOperation(promiseResolver.get(), &ec);

[abarth-chromium, 2013/07/23 06:22:07]

This is a "naked new", which is where the bad memory pattern originates.  All
calls to "new" should be immediately wrapped in either adoptRef (for ref-counted
objects) or adoptPtr (for non-ref-counted objects).  We prefer to avoid these
sorts of complicated lifetime management patterns because they're fragile and
lead to memory corruption or leaks.  You might be smart enough to author this
code correctly, but, as a group, we're not smart enough to maintain them.

[eroman, 2013/07/23 06:53:41]

I'll think about this some more and see what else I can come up with.
I modeled this similarly to CryptoOperation.

I wanted to have a simple interface for the embedder which can allow them to
notify sync/async completion/error in a consistent manner, since there will be
many such operations implemented on top of this.

Because both the embedder and blink hold pointers to each other reference
counting would cause a cycle and I ended up with explicit deletions

[abarth-chromium, 2013/07/23 07:06:14]

We might want to change CryptoOperation to follow whatever pattern we come up
with here.
Rather than returning a raw pointer to the embedder, perhaps we should return a
value type that acts like a smart pointer.  We can implement this with RefPtr
internally to allow the value type to be copied on the embedder's side, but the
expectation is that the embedder will eventually call the success or failure
function on the API object, which will clear any references the WebCore side has
to any other objects (making it a leaf in the object graph and breaking any
cycles).

That approach doesn't add any extra complexity because we already need a similar
guarantee to make sure we resolve the Promise (and thereby breaking the strong
reference from the ScriptPromiseResolver into the JavaScript heap).
Hopefully the approach above resolves any reference cycles.

+    platformCrypto->importKey(format, keyDataBytes, keyData->byteLength(), algorithm, extractable, keyUsages, op);
+    return promiseResolver->promise();
+}
+
 } // namespace WebCore