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

Source/modules/crypto/CryptoResultImpl.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 45 matching lines @@
     if (!resolver->executionContext() || resolver->executionContext()->activeDOMObjectsAreStopped())
         return;
 
     ScriptState::Scope scope(resolver->scriptState());
     v8::Isolate* isolate = resolver->scriptState()->isolate();
     resolver->reject(v8::Exception::TypeError(v8String(isolate, errorDetails)));
 }
 
 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();
         resolver->keepAliveWhilePending();
         return resolver.release();
     }
 
     void stop() override
     {
         m_result->cancel();
         m_result->clearResolver();

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

optional: It might be clearer to remove this line, and instead make it part of
CryptoResultImpl::cancel(). (Not really part of your change, but noticed it
since modifying cancel).

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

Moved clearResolver() (it was moved over in some of the earlier patchsets here
also.)

         m_result = nullptr;
         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) {
     case WebCryptoErrorTypeNotSupported:
         return NotSupportedError;
     case WebCryptoErrorTypeSyntax:
         return SyntaxError;
     case WebCryptoErrorTypeInvalidAccess:
         return InvalidAccessError;
     case WebCryptoErrorTypeData:
         return DataError;
     case WebCryptoErrorTypeOperation:
         return OperationError;
     case WebCryptoErrorTypeType:
         return V8TypeError;
     }
 
     ASSERT_NOT_REACHED();
     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;
 }
 
 PassRefPtrWillBeRawPtr<CryptoResultImpl> CryptoResultImpl::create(ScriptState* scriptState)
 {
     return adoptRefWillBeNoop(new CryptoResultImpl(scriptState));
 }
 
 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());
-    }
+    keyPair.add("publicKey", ScriptValue::from(scriptState, CryptoKey::create(publicKey)));
+    keyPair.add("privateKey", ScriptValue::from(scriptState, CryptoKey::create(privateKey)));
+
+    m_resolver->resolve(keyPair.v8Value());
     clearResolver();
 }
 
-bool CryptoResultImpl::cancelled() const
-{
-    return acquireLoad(&m_cancelled);
-}
-
 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()
 {
     return m_resolver ? m_resolver->promise() : ScriptPromise();
 }
 
 } // namespace blink