WebCrypto: Remove support for multi-part operations.

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 f800a50d42a5ad1758a56abd58b0f2c98aa8ce5e..dddc30cc0572b7796f1edc7603590534e3d6a781 100644
--- a/Source/modules/crypto/SubtleCrypto.cpp
+++ b/Source/modules/crypto/SubtleCrypto.cpp
@@ -31,26 +31,102 @@
 #include "config.h"
 #include "modules/crypto/SubtleCrypto.h"
 
+#include "V8Key.h" // NOTE: This must appear before ScriptPromiseResolver to define toV8()
 #include "bindings/v8/ExceptionState.h"
+#include "bindings/v8/custom/V8ArrayBufferCustom.h" // MUST precede ScriptPromiseResolver for compilation to work.

[abarth-chromium, 2013/08/16 04:53:07]

We should fix these include order dependencies.

[eroman, 2013/08/16 22:59:41]

The issue is that the header file inlines this templetized function:

template<typename T>
void ScriptPromiseResolver::fulfill(PassRefPtr<T> value)
{
    ASSERT(v8::Context::InContext());
    fulfill(toV8(value.get(), v8::Object::New(), m_isolate));
}

Depending what type T you call it with, it needs to be able to resolve the
appropriate toV8(T*) overload. Since there is no global header which declares
all the toV8() functions this causes a header ordering dependency.

Another solution would be to remove the function and have callers do the toV8()
part themselves.

+#include "bindings/v8/ScriptPromiseResolver.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/platform/NotImplemented.h"
-#include "modules/crypto/CryptoOperation.h"
 #include "modules/crypto/Key.h"
-#include "modules/crypto/KeyOperation.h"
 #include "modules/crypto/NormalizeAlgorithm.h"
 #include "public/platform/Platform.h"
+#include "public/platform/WebArrayBuffer.h"
 #include "public/platform/WebCrypto.h"
 #include "public/platform/WebCryptoAlgorithmParams.h"
 #include "wtf/ArrayBufferView.h"
 
 namespace WebCore {
 
-// FIXME: Outstanding KeyOperations and CryptoOperations should be aborted when
-// tearing down SubtleCrypto (to avoid problems completing a
-// ScriptPromiseResolver which is no longer valid).
+// FIXME: Orphan outstanding CryptoOperationResults once SubtleCrypto gets deleted
+// (to avoid problems completing a ScriptPromiseResolver which is no longer valid).
+
+// FIXME: Asynchronous completion needs to re-enter the v8::Context before trying
+//        to fulfill the Promise or it will hit
+//        ASSERT(v8::Context::InContext());
 
 namespace {
 
+// Wrapper around the Promise which the embedder calls into.
+class CryptoOperationResult : public WebKit::WebCryptoOperationResult {
+public:
+    CryptoOperationResult()
+        : m_completed(false)
+        , m_async(false)
+        , m_promiseResolver(ScriptPromiseResolver::create()) { }
+
+    virtual void completeWithError() OVERRIDE
+    {
+        m_promiseResolver->reject(WebCore::ScriptValue::createNull());
+        finish();
+    }
+
+    virtual void completeWithBuffer(const WebKit::WebArrayBuffer& buffer) OVERRIDE
+    {
+        m_promiseResolver->fulfill(PassRefPtr<ArrayBuffer>(buffer));
+        finish();
+    }
+
+    virtual void completeWithBoolean(bool b) OVERRIDE
+    {
+        m_promiseResolver->fulfill(WebCore::ScriptValue::createBoolean(b));
+        finish();
+    }
+
+    virtual void completeWithKey(const WebKit::WebCryptoKey& key) OVERRIDE
+    {
+        m_promiseResolver->fulfill(WebCore::Key::create(key));

[abarth-chromium, 2013/08/16 04:53:07]

WebCore::Key -> Key

We're in the WebCore namespace here.

+        finish();
+    }
+
+    virtual void willFinishLater() OVERRIDE
+    {
+        ASSERT(!m_completed);
+        ASSERT(!m_async);
+        m_async = true;
+    }
+
+    ScriptObject releasePromise()
+    {
+        if (!m_completed && !m_async) {
+            // The embedder forgot to set the result.
+            ASSERT_NOT_REACHED();

[abarth-chromium, 2013/08/16 04:53:07]

Rather than handling this unreachable condition, why not RELEASE_ASSERT?

+            completeWithError();
+        }
+
+        ScriptObject promise = m_promiseResolver->promise();
+
+        if (m_completed)
+            delete this;

[abarth-chromium, 2013/08/16 04:53:07]

:(

I don't understand why we keep needing this |delete this| pattern.  Why not just
have the Chromium side own the object and delete it when its done?

[eroman, 2013/08/16 06:45:58]

This is the same model as callbacks, in which the callback deletes itself after
it has run.

There are only 2 locations where deletion happens now, which I think is an
improvement.

If the ownership is moved over to the Chromium side, then each implementation
which invokes the callback would have to additionally delete the result object
after it is done, which is more error prone.

i.e.
  result->completeWithBuffer(x);
  delete result;

The pattern above could then be wrapped in another object, but just seems like
more complexity.

+
+        return promise;
+    }
+
+private:
+    void finish()
+    {
+        ASSERT(!m_completed);
+        m_completed = true;
+
+        if (m_async)
+            delete this;
+    }
+
+    bool m_completed;
+    bool m_async;
+
+    RefPtr<ScriptPromiseResolver> m_promiseResolver;
+};
+
 WebKit::WebCryptoKeyUsageMask toKeyUsage(AlgorithmOperation operation)
 {
     switch (operation) {
@@ -101,64 +177,71 @@ bool keyCanBeUsedForAlgorithm(const WebKit::WebCryptoKey& key, const WebKit::Web
     return false;
 }
 
-PassRefPtr<CryptoOperation> createCryptoOperation(const Dictionary& rawAlgorithm, Key* key, AlgorithmOperation operationType, ArrayBufferView* signature, ExceptionState& es)
+ScriptObject startCryptoOperation(const Dictionary& rawAlgorithm, Key* key, AlgorithmOperation operationType, ArrayBufferView* signature, ArrayBufferView* dataBuffer, ExceptionState& es)
 {
     WebKit::WebCrypto* platformCrypto = WebKit::Platform::current()->crypto();
     if (!platformCrypto) {
         es.throwDOMException(NotSupportedError);
-        return 0;
+        return ScriptObject();
     }
 
     WebKit::WebCryptoAlgorithm algorithm;
     if (!normalizeAlgorithm(rawAlgorithm, operationType, algorithm, es))
-        return 0;
+        return ScriptObject();
 
     // All operations other than Digest require a valid Key.
     if (operationType != Digest) {
         if (!key) {
             es.throwTypeError();
-            return 0;
+            return ScriptObject();
         }
 
         if (!keyCanBeUsedForAlgorithm(key->key(), algorithm, operationType)) {
             es.throwDOMException(NotSupportedError);
-            return 0;
+            return ScriptObject();
         }
     }
 
     // Only Verify takes a signature.
     if (operationType == Verify && !signature) {
         es.throwTypeError();
-        return 0;
+        return ScriptObject();
     }
 
-    RefPtr<CryptoOperationImpl> opImpl = CryptoOperationImpl::create();
-    WebKit::WebCryptoOperationResult result(opImpl.get());
+    if (!dataBuffer) {
+        es.throwTypeError();
+        return ScriptObject();
+    }
+
+    const unsigned char* data = static_cast<const unsigned char*>(dataBuffer->baseAddress());
+    size_t dataSize = dataBuffer->byteLength();
+
+    // This is deleted either by releasePromise() below, or on asynchronous
+    // completion of the request.
+    CryptoOperationResult* result = new CryptoOperationResult();

[abarth-chromium, 2013/08/16 04:53:07]

This is a naked call to |new|.  We shouldn't have any naked calls to new.  Why
can't this be:

OwnPtr<CryptoOperationResult> result = adoptPtr(new CryptoOperationResult());

Then the object will be deleted automatically when it goes out of scope.  If the
Chromium side needs to take ownership of the object (e.g., because it wants to
complete the operation later), then we can call release() and give the Chromium
side ownership.

[eroman, 2013/08/16 06:45:58]

This was to enable the implementations of encrypt(), decrypt(), sign(), etc to
finish EITHER synchronously or asynchronously.

In the current model,  the pointer is passed to encrypt(), decrypt(), etc.  If
those operations complete synchronously, then the result can be destroyed upon
return. Otherwise the caller has taken ownership of the result until it
completes.

I didn't find the OwnPtr<> pattern to express this better, but I'll think about
it more tomorrow.

 
     switch (operationType) {
     case Encrypt:
-        platformCrypto->encrypt(algorithm, key->key(), result);
+        platformCrypto->encrypt(algorithm, key->key(), data, dataSize, result);
         break;
     case Decrypt:
-        platformCrypto->decrypt(algorithm, key->key(), result);
+        platformCrypto->decrypt(algorithm, key->key(), data, dataSize, result);
         break;
     case Sign:
-        platformCrypto->sign(algorithm, key->key(), result);
+        platformCrypto->sign(algorithm, key->key(), data, dataSize, result);
         break;
     case Verify:
-        platformCrypto->verifySignature(algorithm, key->key(), reinterpret_cast<const unsigned char*>(signature->baseAddress()), signature->byteLength(), result);
+        platformCrypto->verifySignature(algorithm, key->key(), reinterpret_cast<const unsigned char*>(signature->baseAddress()), signature->byteLength(), data, dataSize, result);
         break;
     case Digest:
-        platformCrypto->digest(algorithm, result);
+        platformCrypto->digest(algorithm, data, dataSize, result);
         break;
     default:
         ASSERT_NOT_REACHED();
-        return 0;
+        return ScriptObject();
     }
 
-    if (opImpl->throwInitializationError(es))
-        return 0;
-    return CryptoOperation::create(algorithm, opImpl.get());
+    return result->releasePromise();
 }
 
 } // namespace
@@ -168,29 +251,29 @@ SubtleCrypto::SubtleCrypto()
     ScriptWrappable::init(this);
 }
 
-PassRefPtr<CryptoOperation> SubtleCrypto::encrypt(const Dictionary& rawAlgorithm, Key* key, ExceptionState& es)
+ScriptObject SubtleCrypto::encrypt(const Dictionary& rawAlgorithm, Key* key, ArrayBufferView* data, ExceptionState& es)
 {
-    return createCryptoOperation(rawAlgorithm, key, Encrypt, 0, es);
+    return startCryptoOperation(rawAlgorithm, key, Encrypt, 0, data, es);
 }
 
-PassRefPtr<CryptoOperation> SubtleCrypto::decrypt(const Dictionary& rawAlgorithm, Key* key, ExceptionState& es)
+ScriptObject SubtleCrypto::decrypt(const Dictionary& rawAlgorithm, Key* key, ArrayBufferView* data, ExceptionState& es)
 {
-    return createCryptoOperation(rawAlgorithm, key, Decrypt, 0, es);
+    return startCryptoOperation(rawAlgorithm, key, Decrypt, 0, data, es);
 }
 
-PassRefPtr<CryptoOperation> SubtleCrypto::sign(const Dictionary& rawAlgorithm, Key* key, ExceptionState& es)
+ScriptObject SubtleCrypto::sign(const Dictionary& rawAlgorithm, Key* key, ArrayBufferView* data, ExceptionState& es)
 {
-    return createCryptoOperation(rawAlgorithm, key, Sign, 0, es);
+    return startCryptoOperation(rawAlgorithm, key, Sign, 0, data, es);
 }
 
-PassRefPtr<CryptoOperation> SubtleCrypto::verifySignature(const Dictionary& rawAlgorithm, Key* key, ArrayBufferView* signature, ExceptionState& es)
+ScriptObject SubtleCrypto::verifySignature(const Dictionary& rawAlgorithm, Key* key, ArrayBufferView* signature, ArrayBufferView* data, ExceptionState& es)
 {
-    return createCryptoOperation(rawAlgorithm, key, Verify, signature, es);
+    return startCryptoOperation(rawAlgorithm, key, Verify, signature, data, es);
 }
 
-PassRefPtr<CryptoOperation> SubtleCrypto::digest(const Dictionary& rawAlgorithm, ExceptionState& es)
+ScriptObject SubtleCrypto::digest(const Dictionary& rawAlgorithm, ArrayBufferView* data, ExceptionState& es)
 {
-    return createCryptoOperation(rawAlgorithm, 0, Digest, 0, es);
+    return startCryptoOperation(rawAlgorithm, 0, Digest, 0, data, es);
 }
 
 ScriptObject SubtleCrypto::generateKey(const Dictionary& rawAlgorithm, bool extractable, const Vector<String>& rawKeyUsages, ExceptionState& es)
@@ -211,10 +294,13 @@ ScriptObject SubtleCrypto::generateKey(const Dictionary& rawAlgorithm, bool extr
     if (!normalizeAlgorithm(rawAlgorithm, GenerateKey, algorithm, es))
         return ScriptObject();
 
-    RefPtr<KeyOperation> keyOp = KeyOperation::create();
-    WebKit::WebCryptoKeyOperationResult result(keyOp.get());
+    // This is deleted either by releasePromise() below, or on asynchronous
+    // completion of the request.
+    CryptoOperationResult* result = new CryptoOperationResult();

[abarth-chromium, 2013/08/16 04:53:07]

Another naked |new|.  None of these should be necessary.

+
     platformCrypto->generateKey(algorithm, extractable, keyUsages, result);
-    return keyOp->returnValue(es);
+
+    return result->releasePromise();
 }
 
 ScriptObject SubtleCrypto::importKey(const String& rawFormat, ArrayBufferView* keyData, const Dictionary& rawAlgorithm, bool extractable, const Vector<String>& rawKeyUsages, ExceptionState& es)
@@ -248,10 +334,13 @@ ScriptObject SubtleCrypto::importKey(const String& rawFormat, ArrayBufferView* k
 
     const unsigned char* keyDataBytes = static_cast<unsigned char*>(keyData->baseAddress());
 
-    RefPtr<KeyOperation> keyOp = KeyOperation::create();
-    WebKit::WebCryptoKeyOperationResult result(keyOp.get());
+    // This is deleted either by releasePromise() below, or on asynchronous
+    // completion of the request.
+    CryptoOperationResult* result = new CryptoOperationResult();
+
     platformCrypto->importKey(format, keyDataBytes, keyData->byteLength(), algorithm, extractable, keyUsages, result);
-    return keyOp->returnValue(es);
+
+    return result->releasePromise();
 }
 
 } // namespace WebCore