WebCrypto: Remove support for multi-part operations.

Source/modules/crypto/SubtleCrypto.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 13 matching lines @@
  * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #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) {
     case Encrypt:
         return WebKit::WebCryptoKeyUsageEncrypt;
     case Decrypt:
         return WebKit::WebCryptoKeyUsageDecrypt;
     case Sign:
         return WebKit::WebCryptoKeyUsageSign;
     case Verify:
@@ skipping 30 matching lines @@
     // algorithm.
 
     if (key.algorithm().paramsType() == WebKit::WebCryptoAlgorithmParamsTypeHmacParams) {
         return key.algorithm().hmacParams()->hash().id() == algorithm.hmacParams()->hash().id();
     }
 
     ASSERT_NOT_REACHED();
     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
 
 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)
 {
     WebKit::WebCrypto* platformCrypto = WebKit::Platform::current()->crypto();
     if (!platformCrypto) {
         es.throwDOMException(NotSupportedError);
         return ScriptObject();
     }
 
     WebKit::WebCryptoKeyUsageMask keyUsages;
     if (!Key::parseUsageMask(rawKeyUsages, keyUsages)) {
         es.throwTypeError();
         return ScriptObject();
     }
 
     WebKit::WebCryptoAlgorithm algorithm;
     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)
 {
     WebKit::WebCrypto* platformCrypto = WebKit::Platform::current()->crypto();
     if (!platformCrypto) {
         es.throwDOMException(NotSupportedError);
         return ScriptObject();
     }
 
@@ skipping 13 matching lines @@
         es.throwTypeError();
         return ScriptObject();
     }
 
     WebKit::WebCryptoAlgorithm algorithm;
     if (!normalizeAlgorithm(rawAlgorithm, ImportKey, algorithm, es))
         return ScriptObject();
 
     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