Introduce KeepAliveWhilePending to ScriptPromiseResolverWithContext.

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 832f916541561c73da9c175f00c8229f9d9976be..6be374a1803ed494691b1c19181172592a10040f 100644
--- a/Source/modules/crypto/SubtleCrypto.cpp
+++ b/Source/modules/crypto/SubtleCrypto.cpp
@@ -32,17 +32,86 @@
 #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)
@@ -85,10 +154,11 @@ static bool canAccessWebCrypto(ScriptState* scriptState, CryptoResult* result)
     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;
@@ -168,8 +238,9 @@ ScriptPromise SubtleCrypto::digest(ScriptState* scriptState, const Dictionary& r
 
 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;
@@ -188,8 +259,9 @@ ScriptPromise SubtleCrypto::generateKey(ScriptState* scriptState, const Dictiona
 
 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;
@@ -215,8 +287,9 @@ ScriptPromise SubtleCrypto::importKey(ScriptState* scriptState, const String& ra
 
 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;
@@ -239,8 +312,9 @@ ScriptPromise SubtleCrypto::exportKey(ScriptState* scriptState, const String& ra
 
 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;
@@ -273,8 +347,9 @@ ScriptPromise SubtleCrypto::wrapKey(ScriptState* scriptState, const String& rawF
 
 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;