Reliably cancel in-progress CryptoResult(Impl) upon shutdown.

Source/modules/crypto/CryptoResultImpl.cpp

Index: Source/modules/crypto/CryptoResultImpl.cpp
diff --git a/Source/modules/crypto/CryptoResultImpl.cpp b/Source/modules/crypto/CryptoResultImpl.cpp
index 2360727a13a91f34ae1e4352523c2068b1c2c2e2..c8e2cca994f2cabee953a36e4e556fbe0c4460ed 100644
--- a/Source/modules/crypto/CryptoResultImpl.cpp
+++ b/Source/modules/crypto/CryptoResultImpl.cpp
@@ -63,7 +63,7 @@ static void rejectWithTypeError(const String& errorDetails, ScriptPromiseResolve
 
 class CryptoResultImpl::Resolver final : public ScriptPromiseResolver {
 public:
-    static PassRefPtrWillBeRawPtr<ScriptPromiseResolver> create(ScriptState* scriptState, CryptoResultImpl* result)
+    static PassRefPtrWillBeRawPtr<Resolver> create(ScriptState* scriptState, CryptoResultImpl* result)
     {
         RefPtrWillBeRawPtr<Resolver> resolver = adoptRefWillBeNoop(new Resolver(scriptState, result));
         resolver->suspendIfNeeded();
@@ -79,13 +79,35 @@ public:
         ScriptPromiseResolver::stop();
     }
 
+    DEFINE_INLINE_VIRTUAL_TRACE()
+    {
+        visitor->trace(m_result);
+        ScriptPromiseResolver::trace(visitor);
+    }
+
 private:
     Resolver(ScriptState* scriptState, CryptoResultImpl* result)
         : ScriptPromiseResolver(scriptState)
         , m_result(result) { }
-    RefPtr<CryptoResultImpl> m_result;
+
+    RefPtrWillBeMember<CryptoResultImpl> m_result;
 };
 
+CryptoResultImpl::ResultCancel::ResultCancel()
+    : m_cancelled(0)
+{
+}
+
+bool CryptoResultImpl::ResultCancel::cancelled() const
+{
+    return acquireLoad(&m_cancelled);
+}
+
+void CryptoResultImpl::ResultCancel::cancel()
+{
+    releaseStore(&m_cancelled, 1);
+}
+
 ExceptionCode webCryptoErrorToExceptionCode(WebCryptoErrorType errorType)
 {
     switch (errorType) {
@@ -107,11 +129,30 @@ ExceptionCode webCryptoErrorToExceptionCode(WebCryptoErrorType errorType)
     return 0;
 }
 
+CryptoResultImpl::CryptoResultImpl(ScriptState* scriptState)
+{
+    ASSERT(scriptState->contextIsValid());
+    if (scriptState->executionContext()->activeDOMObjectsAreStopped()) {
+        // If active dom objects have been stopped, avoid creating
+        // CryptoResultImpl::Resolver.
+        m_resolver = nullptr;
+        return;

[eroman, 2015/08/01 00:13:14]

IMPORTANT: This return doesn't seem right. In the edge case where this codepath
is reached, m_cancel will be left as a nullptr, which will cause a crash in when
calling the WebCryptoResult::completeWith*() methods.

Instead you should initialize it at a minimum to a valid pointer here (and
preferably to a value indicating it is cancelled).

[sof, 2015/08/01 06:44:30]

The WebCryptoResult ctor will complain if that happens; initialized m_cancel as
cancelled.

It seems wasteful to be able to initiate a webcrypto operation if the object is
in a stopped state, when it is considered cancelled already.

[eroman, 2015/08/03 19:00:29]

The WebCryptoResult ctor will merely ASSERT().

It is important to handle this, otherwise we are going to crash later.

If we can reach this codepath, then we will be able to reach the completeWith*()
codepath as well, which is then going to crash....

The only case this doesn't matter is if you can prove that this branch is never
entered.

[eroman, 2015/08/03 19:45:34]

Ah, I didn't notice that you addressed this in the following patchset, please
disregard then :)

Still LGTM

+    }
+    m_resolver = Resolver::create(scriptState, this).get();
+    m_cancel = ResultCancel::create();
+}
+
 CryptoResultImpl::~CryptoResultImpl()
 {
     ASSERT(!m_resolver);
 }
 
+DEFINE_TRACE(CryptoResultImpl)
+{
+    visitor->trace(m_resolver);
+    CryptoResult::trace(visitor);
+}
+
 void CryptoResultImpl::clearResolver()
 {
     m_resolver = nullptr;
@@ -124,96 +165,88 @@ PassRefPtrWillBeRawPtr<CryptoResultImpl> CryptoResultImpl::create(ScriptState* s
 
 void CryptoResultImpl::completeWithError(WebCryptoErrorType errorType, const WebString& errorDetails)
 {
-    if (m_resolver) {
-        ExceptionCode ec = webCryptoErrorToExceptionCode(errorType);
+    if (!m_resolver)
+        return;
 
-        // Handle TypeError separately, as it cannot be created using
-        // DOMException.
-        if (ec == V8TypeError)
-            rejectWithTypeError(errorDetails, m_resolver);
-        else
-            m_resolver->reject(DOMException::create(ec, errorDetails));
-    }
+    ExceptionCode ec = webCryptoErrorToExceptionCode(errorType);
+
+    // Handle TypeError separately, as it cannot be created using
+    // DOMException.
+    if (ec == V8TypeError)
+        rejectWithTypeError(errorDetails, m_resolver);
+    else
+        m_resolver->reject(DOMException::create(ec, errorDetails));
     clearResolver();
 }
 
 void CryptoResultImpl::completeWithBuffer(const void* bytes, unsigned bytesSize)
 {
-    if (m_resolver)
-        m_resolver->resolve(DOMArrayBuffer::create(bytes, bytesSize));
+    if (!m_resolver)
+        return;
+
+    m_resolver->resolve(DOMArrayBuffer::create(bytes, bytesSize));
     clearResolver();
 }
 
 void CryptoResultImpl::completeWithJson(const char* utf8Data, unsigned length)
 {
-    if (m_resolver) {
-        ScriptPromiseResolver* resolver = m_resolver;
-        ScriptState* scriptState = resolver->scriptState();
-        ScriptState::Scope scope(scriptState);
+    if (!m_resolver)
+        return;
 
-        v8::Local<v8::String> jsonString = v8AtomicString(scriptState->isolate(), utf8Data, length);
+    ScriptState* scriptState = m_resolver->scriptState();
+    ScriptState::Scope scope(scriptState);
 
-        v8::TryCatch exceptionCatcher;
-        v8::Local<v8::Value> jsonDictionary;
-        if (v8Call(v8::JSON::Parse(scriptState->isolate(), jsonString), jsonDictionary, exceptionCatcher))
-            resolver->resolve(jsonDictionary);
-        else
-            resolver->reject(exceptionCatcher.Exception());
-    }
+    v8::Local<v8::String> jsonString = v8AtomicString(scriptState->isolate(), utf8Data, length);
+
+    v8::TryCatch exceptionCatcher;
+    v8::Local<v8::Value> jsonDictionary;
+    if (v8Call(v8::JSON::Parse(scriptState->isolate(), jsonString), jsonDictionary, exceptionCatcher))
+        m_resolver->resolve(jsonDictionary);
+    else
+        m_resolver->reject(exceptionCatcher.Exception());
     clearResolver();
 }
 
 void CryptoResultImpl::completeWithBoolean(bool b)
 {
-    if (m_resolver)
-        m_resolver->resolve(b);
+    if (!m_resolver)
+        return;
+
+    m_resolver->resolve(b);
     clearResolver();
 }
 
 void CryptoResultImpl::completeWithKey(const WebCryptoKey& key)
 {
-    if (m_resolver)
-        m_resolver->resolve(CryptoKey::create(key));
+    if (!m_resolver)
+        return;
+
+    m_resolver->resolve(CryptoKey::create(key));
     clearResolver();
 }
 
 void CryptoResultImpl::completeWithKeyPair(const WebCryptoKey& publicKey, const WebCryptoKey& privateKey)
 {
-    if (m_resolver) {
-        ScriptState* scriptState = m_resolver->scriptState();
-        ScriptState::Scope scope(scriptState);
+    if (!m_resolver)
+        return;
 
-        V8ObjectBuilder keyPair(scriptState);
+    ScriptState* scriptState = m_resolver->scriptState();
+    ScriptState::Scope scope(scriptState);
 
-        keyPair.add("publicKey", ScriptValue::from(scriptState, CryptoKey::create(publicKey)));
-        keyPair.add("privateKey", ScriptValue::from(scriptState, CryptoKey::create(privateKey)));
+    V8ObjectBuilder keyPair(scriptState);
 
-        m_resolver->resolve(keyPair.v8Value());
-    }
-    clearResolver();
-}
+    keyPair.add("publicKey", ScriptValue::from(scriptState, CryptoKey::create(publicKey)));
+    keyPair.add("privateKey", ScriptValue::from(scriptState, CryptoKey::create(privateKey)));
 
-bool CryptoResultImpl::cancelled() const
-{
-    return acquireLoad(&m_cancelled);
+    m_resolver->resolve(keyPair.v8Value());
+    clearResolver();
 }
 
 void CryptoResultImpl::cancel()
 {
-    releaseStore(&m_cancelled, 1);
-}
-
-CryptoResultImpl::CryptoResultImpl(ScriptState* scriptState)
-    : m_cancelled(0)
-{
-    ASSERT(scriptState->contextIsValid());
-    if (scriptState->executionContext()->activeDOMObjectsAreStopped()) {
-        // If active dom objects have been stopped, avoid creating
-        // CryptoResultResolver.
-        m_resolver = nullptr;
-    } else {
-        m_resolver = Resolver::create(scriptState, this).get();
-    }
+    ASSERT(m_cancel);
+    m_cancel->cancel();
+    m_cancel.release();

[eroman, 2015/07/31 23:40:30]

optional: It might be more idiomatic to write this as:
   m_cancel.clear();

[sof, 2015/08/01 06:44:30]

Done.

 }
 
 ScriptPromise CryptoResultImpl::promise()