Introduce KeepAliveWhilePending to ScriptPromiseResolverWithContext.

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 14 matching lines @@
  * 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 "bindings/v8/Dictionary.h"
+#include "bindings/v8/ScriptPromiseResolverWithContext.h"
+#include "core/dom/DOMException.h"
 #include "core/dom/ExecutionContext.h"
+#include "modules/crypto/CrossThreadCryptoResult.h"
 #include "modules/crypto/CryptoResultImpl.h"
 #include "modules/crypto/Key.h"
+#include "modules/crypto/KeyPair.h"
 #include "modules/crypto/NormalizeAlgorithm.h"
 #include "public/platform/Platform.h"
+#include "public/platform/WebArrayBuffer.h"
 #include "public/platform/WebCrypto.h"
 #include "public/platform/WebCryptoAlgorithm.h"
 #include "wtf/ArrayBufferView.h"
 
 namespace WebCore {
 
+class SubtleCrypto::PendingResult : public CryptoResult {

[eroman, 2014/06/13 01:52:52]

This doesn't need to inherit from CryptoResult, it could inherit RefCounted
directly (which would be more efficient since CryptoResult is thread-safe
refcounted, and also doesn't need any virtual methods).

That said, this doesn't need to be refcounted at all.
  * unregisterThis() could directly do the "delete this" which I think would be
clearer than indirecting through the hashmap for ownership.

  *~SubtleCrypto() would have to also now delete any existing PendingResults

In fact, I will probably have to switch to that model anyway to support
cancellation. Since in a future change I would like to expose some method like:
   CrossThreadCryptoResult::wasCancelled()
As part of http://crbug.com/375430  (which can be called from any thread)

In this new design I imagine I would do that by having PendingResult also hold a
pointer to the CrossThreadCryptoResult, so when ~SubtleCrypto() runs the
outstanding CrossThreadCryptoResult can be marked as cancelled.

[yhirano, 2014/06/13 04:19:30]

I added decouplsed CompleteWith* functions from CryptoResult to
CryptoResultBase.
Now PendingResult is an Oilpan class.
What do you think of this?

+public:
+    virtual ~PendingResult() { }
+
+    static PassRefPtr<PendingResult> create(ScriptState* scriptState, RawPtr<SubtleCrypto> owner)
+    {
+        return adoptRef(new PendingResult(scriptState, owner));
+    }
+
+    virtual void completeWithError(blink::WebCryptoErrorType errorType, const blink::WebString& errorDetails) OVERRIDE
+    {
+        m_resolver->reject(DOMException::create(webCryptoErrorToExceptionCode(errorType), errorDetails));
+        unregisterThis();
+    }
+
+    virtual void completeWithBuffer(const blink::WebArrayBuffer& buffer) OVERRIDE
+    {
+        m_resolver->resolve(PassRefPtr<ArrayBuffer>(buffer));
+        unregisterThis();
+    }
+
+    virtual void completeWithBoolean(bool b) OVERRIDE
+    {
+        m_resolver->resolve(b);
+        unregisterThis();
+    }
+
+    virtual void completeWithKey(const blink::WebCryptoKey& key) OVERRIDE
+    {
+        m_resolver->resolve(Key::create(key));
+        unregisterThis();
+    }
+
+    virtual void completeWithKeyPair(const blink::WebCryptoKey& publicKey, const blink::WebCryptoKey& privateKey) OVERRIDE
+    {
+        m_resolver->resolve(KeyPair::create(publicKey, privateKey));
+        unregisterThis();
+    }
+
+    ScriptPromise promise()  { return m_resolver->promise(); }

[eroman, 2014/06/13 01:52:52]

extra space

[yhirano, 2014/06/13 04:19:30]

Done.

+
+    WeakPtr<PendingResult> createWeakPtr() { return m_factory.createWeakPtr(); }
+
+private:
+    PendingResult(ScriptState* scriptState, RawPtr<SubtleCrypto> owner)
+        : m_resolver(ScriptPromiseResolverWithContext::create(scriptState))
+        , m_owner(owner)
+        , m_factory(this)
+    {
+        m_resolver->keepObjectWhilePending(owner);

[eroman, 2014/06/13 01:52:52]

Could there be any issues with keeping SubtleCrypto alive for longer? Or is it
already the case that once created, SubtleCrypto lives until the
ExecutionContext is destroyed anyway?

[yhirano, 2014/06/13 04:19:30]

SubtleCrypto lives while one of the following meets:
 1. Its wrapper is referred by JavaScript
 2. One of *pending* promises which it returned is alive. 

When all Promises are resolved or rejected and the wrapper is not reachable from
the root set, the SubtleCrypto  will be garbage collected.

When the ExecutionContext is stopped, all Promises will become not Pending.

+        m_owner->m_pendingResults.add(this);
+    }
+
+    void unregisterThis()

[eroman, 2014/06/13 01:52:52]

Please document that this will likely also "delete this"

[yhirano, 2014/06/13 04:19:30]

Now PendingResult is an oilpan class and |this| will be garbage collected later.

+    {
+        if (m_owner->m_pendingResults.contains(this))

[eroman, 2014/06/13 01:52:52]

No need for the "contains()" -- remove() is a no-op if it it doesn't contain it.

The contains() should probably be an assertion instead.

[yhirano, 2014/06/13 04:19:30]

Done.
I don't want to put the assertion in order to allow multiple  calls.

+            m_owner->m_pendingResults.remove(this);
+    }
+
+    RefPtr<ScriptPromiseResolverWithContext> m_resolver;

[eroman, 2014/06/13 01:52:52]

The ownership model here has become quite complicated!

[yhirano, 2014/06/13 04:19:30]

It becomes simpler now, I hope.

+    Persistent<SubtleCrypto> m_owner;
+    WeakPtrFactory<PendingResult> m_factory;
+};
+
 // Seems like the generated bindings should take care of these however it
 // currently doesn't. See also http://crbug.com/264520
 static bool ensureNotNull(const ArrayPiece& x, const char* paramName, CryptoResult* result)
 {
     if (x.isNull()) {
         String message = String("Invalid ") + paramName + String(" argument");
         result->completeWithError(blink::WebCryptoErrorTypeType, blink::WebString(message));
         return false;
     }
     return true;
@@ skipping 22 matching lines @@
 {
     const SecurityOrigin* origin = scriptState->executionContext()->securityOrigin();
     if (!origin->canAccessFeatureRequiringSecureOrigin()) {
         result->completeWithError(blink::WebCryptoErrorTypeNotSupported, "WebCrypto is only supported over secure origins. See http://crbug.com/373032");
         return false;
     }
 
     return true;
 }
 
-static ScriptPromise startCryptoOperation(ScriptState* scriptState, const Dictionary& rawAlgorithm, Key* key, blink::WebCryptoOperation operationType, const ArrayPiece& signature, const ArrayPiece& dataBuffer)
+ScriptPromise SubtleCrypto::startCryptoOperation(ScriptState* scriptState, const Dictionary& rawAlgorithm, Key* key, blink::WebCryptoOperation operationType, const ArrayPiece& signature, const ArrayPiece& dataBuffer)
 {
-    RefPtr<CryptoResultImpl> result = CryptoResultImpl::create(scriptState);
-    ScriptPromise promise = result->promise();
+    RefPtr<PendingResult> pending = PendingResult::create(scriptState, this);

[eroman, 2014/06/13 01:52:52]

Can you extract the creation of CrossThreadCryptoResult to a member function
since this is done in several places?

  RefPtr<CrossThreadCryptoResult> SubtleCrypto::createResult();

[yhirano, 2014/06/13 04:19:30]

Done.

+    RefPtr<CrossThreadCryptoResult<PendingResult> > result = CrossThreadCryptoResult<PendingResult>::create(pending->createWeakPtr());
+    ScriptPromise promise = pending->promise();
 
     if (!canAccessWebCrypto(scriptState, result.get()))
         return promise;
 
     bool requiresKey = operationType != blink::WebCryptoOperationDigest;
 
     if (requiresKey && !ensureNotNull(key, "key", result.get()))
         return promise;
     if (operationType == blink::WebCryptoOperationVerify && !ensureNotNull(signature, "signature", result.get()))
         return promise;
@@ skipping 59 matching lines @@
     return startCryptoOperation(scriptState, rawAlgorithm, key, blink::WebCryptoOperationVerify, signature, data);
 }
 
 ScriptPromise SubtleCrypto::digest(ScriptState* scriptState, const Dictionary& rawAlgorithm, const ArrayPiece& data)
 {
     return startCryptoOperation(scriptState, rawAlgorithm, 0, blink::WebCryptoOperationDigest, ArrayPiece(), data);
 }
 
 ScriptPromise SubtleCrypto::generateKey(ScriptState* scriptState, const Dictionary& rawAlgorithm, bool extractable, const Vector<String>& rawKeyUsages)
 {
-    RefPtr<CryptoResultImpl> result = CryptoResultImpl::create(scriptState);
-    ScriptPromise promise = result->promise();
+    RefPtr<PendingResult> pending = PendingResult::create(scriptState, this);
+    RefPtr<CrossThreadCryptoResult<PendingResult> > result = CrossThreadCryptoResult<PendingResult>::create(pending->createWeakPtr());
+    ScriptPromise promise = pending->promise();
 
     if (!canAccessWebCrypto(scriptState, result.get()))
         return promise;
 
     blink::WebCryptoKeyUsageMask keyUsages;
     if (!Key::parseUsageMask(rawKeyUsages, keyUsages, result.get()))
         return promise;
 
     blink::WebCryptoAlgorithm algorithm;
     if (!parseAlgorithm(rawAlgorithm, blink::WebCryptoOperationGenerateKey, algorithm, result.get()))
         return promise;
 
     blink::Platform::current()->crypto()->generateKey(algorithm, extractable, keyUsages, result->result());
     return promise;
 }
 
 ScriptPromise SubtleCrypto::importKey(ScriptState* scriptState, const String& rawFormat, const ArrayPiece& keyData, const Dictionary& rawAlgorithm, bool extractable, const Vector<String>& rawKeyUsages)
 {
-    RefPtr<CryptoResultImpl> result = CryptoResultImpl::create(scriptState);
-    ScriptPromise promise = result->promise();
+    RefPtr<PendingResult> pending = PendingResult::create(scriptState, this);
+    RefPtr<CrossThreadCryptoResult<PendingResult> > result = CrossThreadCryptoResult<PendingResult>::create(pending->createWeakPtr());
+    ScriptPromise promise = pending->promise();
 
     if (!canAccessWebCrypto(scriptState, result.get()))
         return promise;
 
     if (!ensureNotNull(keyData, "keyData", result.get()))
         return promise;
 
     blink::WebCryptoKeyFormat format;
     if (!Key::parseFormat(rawFormat, format, result.get()))
         return promise;
 
     blink::WebCryptoKeyUsageMask keyUsages;
     if (!Key::parseUsageMask(rawKeyUsages, keyUsages, result.get()))
         return promise;
 
     blink::WebCryptoAlgorithm algorithm;
     if (!parseAlgorithm(rawAlgorithm, blink::WebCryptoOperationImportKey, algorithm, result.get()))
         return promise;
 
     blink::Platform::current()->crypto()->importKey(format, keyData.bytes(), keyData.byteLength(), algorithm, extractable, keyUsages, result->result());
     return promise;
 }
 
 ScriptPromise SubtleCrypto::exportKey(ScriptState* scriptState, const String& rawFormat, Key* key)
 {
-    RefPtr<CryptoResultImpl> result = CryptoResultImpl::create(scriptState);
-    ScriptPromise promise = result->promise();
+    RefPtr<PendingResult> pending = PendingResult::create(scriptState, this);
+    RefPtr<CrossThreadCryptoResult<PendingResult> > result = CrossThreadCryptoResult<PendingResult>::create(pending->createWeakPtr());
+    ScriptPromise promise = pending->promise();
 
     if (!canAccessWebCrypto(scriptState, result.get()))
         return promise;
 
     if (!ensureNotNull(key, "key", result.get()))
         return promise;
 
     blink::WebCryptoKeyFormat format;
     if (!Key::parseFormat(rawFormat, format, result.get()))
         return promise;
 
     if (!key->extractable()) {
         result->completeWithError(blink::WebCryptoErrorTypeInvalidAccess, "key is not extractable");
         return promise;
     }
 
     blink::Platform::current()->crypto()->exportKey(format, key->key(), result->result());
     return promise;
 }
 
 ScriptPromise SubtleCrypto::wrapKey(ScriptState* scriptState, const String& rawFormat, Key* key, Key* wrappingKey, const Dictionary& rawWrapAlgorithm)
 {
-    RefPtr<CryptoResultImpl> result = CryptoResultImpl::create(scriptState);
-    ScriptPromise promise = result->promise();
+    RefPtr<PendingResult> pending = PendingResult::create(scriptState, this);
+    RefPtr<CrossThreadCryptoResult<PendingResult> > result = CrossThreadCryptoResult<PendingResult>::create(pending->createWeakPtr());
+    ScriptPromise promise = pending->promise();
 
     if (!canAccessWebCrypto(scriptState, result.get()))
         return promise;
 
     if (!ensureNotNull(key, "key", result.get()))
         return promise;
 
     if (!ensureNotNull(wrappingKey, "wrappingKey", result.get()))
         return promise;
 
@@ skipping 12 matching lines @@
 
     if (!wrappingKey->canBeUsedForAlgorithm(wrapAlgorithm, blink::WebCryptoOperationWrapKey, result.get()))
         return promise;
 
     blink::Platform::current()->crypto()->wrapKey(format, key->key(), wrappingKey->key(), wrapAlgorithm, result->result());
     return promise;
 }
 
 ScriptPromise SubtleCrypto::unwrapKey(ScriptState* scriptState, const String& rawFormat, const ArrayPiece& wrappedKey, Key* unwrappingKey, const Dictionary& rawUnwrapAlgorithm, const Dictionary& rawUnwrappedKeyAlgorithm, bool extractable, const Vector<String>& rawKeyUsages)
 {
-    RefPtr<CryptoResultImpl> result = CryptoResultImpl::create(scriptState);
-    ScriptPromise promise = result->promise();
+    RefPtr<PendingResult> pending = PendingResult::create(scriptState, this);
+    RefPtr<CrossThreadCryptoResult<PendingResult> > result = CrossThreadCryptoResult<PendingResult>::create(pending->createWeakPtr());
+    ScriptPromise promise = pending->promise();
 
     if (!canAccessWebCrypto(scriptState, result.get()))
         return promise;
 
     if (!ensureNotNull(wrappedKey, "wrappedKey", result.get()))
         return promise;
     if (!ensureNotNull(unwrappingKey, "unwrappingKey", result.get()))
         return promise;
 
     blink::WebCryptoKeyFormat format;
@@ skipping 13 matching lines @@
         return promise;
 
     if (!unwrappingKey->canBeUsedForAlgorithm(unwrapAlgorithm, blink::WebCryptoOperationUnwrapKey, result.get()))
         return promise;
 
     blink::Platform::current()->crypto()->unwrapKey(format, wrappedKey.bytes(), wrappedKey.byteLength(), unwrappingKey->key(), unwrapAlgorithm, unwrappedKeyAlgorithm, extractable, keyUsages, result->result());
     return promise;
 }
 
 } // namespace WebCore