Make CryptoResultImpl not to use WeakPtr.

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 32 matching lines @@
 #include "core/dom/ExecutionContext.h"
 #include "modules/crypto/CryptoKey.h"
 #include "modules/crypto/NormalizeAlgorithm.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebCryptoAlgorithm.h"
 
 namespace blink {
 
 class CryptoResultImpl::WeakResolver : public ScriptPromiseResolver {
 public:
-    static WeakPtr<ScriptPromiseResolver> create(ScriptState* scriptState, CryptoResultImpl* result)
+    static PassRefPtr<ScriptPromiseResolver> create(ScriptState* scriptState, CryptoResultImpl* result)
     {
         RefPtr<WeakResolver> p = adoptRef(new WeakResolver(scriptState, result));
         p->suspendIfNeeded();
         p->keepAliveWhilePending();
-        return p->m_weakPtrFactory.createWeakPtr();
+        return p.release();
     }
 
-    virtual ~WeakResolver()
+    virtual void stop() override
     {
         m_result->cancel();
+        m_result->clearResolver();
+        ScriptPromiseResolver::stop();
     }
 
 private:
     WeakResolver(ScriptState* scriptState, CryptoResultImpl* result)
         : ScriptPromiseResolver(scriptState)
-        , m_weakPtrFactory(this)
         , m_result(result) { }
-    WeakPtrFactory<ScriptPromiseResolver> m_weakPtrFactory;
-    RefPtr<CryptoResultImpl> m_result;
+    CryptoResultImpl* m_result;

[yhirano, 2014/12/11 02:27:59]

I'm not sure this change is correct, because I think |m_result| can be
destructed before this object is destroyed without RefPtr.
Perhaps we should clear m_result explicitly when calling resolve / reject,
right?

[tasak, 2014/12/11 09:44:39]

I see.
Now CryptoResultImpl::clearResolver() invokes clearCryptoResult() to clear
m_result.

 };
 
 ExceptionCode webCryptoErrorToExceptionCode(WebCryptoErrorType errorType)
 {
     switch (errorType) {
     case WebCryptoErrorTypeNotSupported:
         return NotSupportedError;
     case WebCryptoErrorTypeSyntax:
         return SyntaxError;
     case WebCryptoErrorTypeInvalidState:
@@ skipping 14 matching lines @@
     }
 
     ASSERT_NOT_REACHED();
     return 0;
 }
 
 CryptoResultImpl::~CryptoResultImpl()
 {
 }
 
+void CryptoResultImpl::clearResolver()
+{
+    m_resolver = nullptr;
+}
+
 PassRefPtr<CryptoResultImpl> CryptoResultImpl::create(ScriptState* scriptState)
 {
     return adoptRef(new CryptoResultImpl(scriptState));
 }
 
 void CryptoResultImpl::completeWithError(WebCryptoErrorType errorType, const WebString& errorDetails)
 {
     if (m_resolver)
         m_resolver->reject(DOMException::create(webCryptoErrorToExceptionCode(errorType), errorDetails));
+    m_resolver = nullptr;
 }
 
 void CryptoResultImpl::completeWithBuffer(const void* bytes, unsigned bytesSize)
 {
     if (m_resolver)
         m_resolver->resolve(DOMArrayBuffer::create(bytes, bytesSize));
+    m_resolver = nullptr;
 }
 
 void CryptoResultImpl::completeWithJson(const char* utf8Data, unsigned length)
 {
     if (m_resolver) {
         ScriptPromiseResolver* resolver = m_resolver.get();
         ScriptState* scriptState = resolver->scriptState();
         ScriptState::Scope scope(scriptState);
 
         v8::Handle<v8::String> jsonString = v8::String::NewFromUtf8(scriptState->isolate(), utf8Data, v8::String::kInternalizedString, length);
 
         v8::TryCatch exceptionCatcher;
         v8::Handle<v8::Value> jsonDictionary = v8::JSON::Parse(jsonString);
         if (exceptionCatcher.HasCaught() || jsonDictionary.IsEmpty()) {
             ASSERT_NOT_REACHED();
             resolver->reject(DOMException::create(OperationError, "Failed inflating JWK JSON to object"));
         } else {
             resolver->resolve(jsonDictionary);
         }
     }
+    m_resolver = nullptr;
 }
 
 void CryptoResultImpl::completeWithBoolean(bool b)
 {
     if (m_resolver)
         m_resolver->resolve(b);
+    m_resolver = nullptr;
 }
 
 void CryptoResultImpl::completeWithKey(const WebCryptoKey& key)
 {
     if (m_resolver)
         m_resolver->resolve(CryptoKey::create(key));
+    m_resolver = nullptr;
 }
 
 void CryptoResultImpl::completeWithKeyPair(const WebCryptoKey& publicKey, const WebCryptoKey& privateKey)
 {
     if (m_resolver) {
         ScriptState* scriptState = m_resolver->scriptState();
         ScriptState::Scope scope(scriptState);
 
         Dictionary keyPair = Dictionary::createEmpty(scriptState->isolate());
 
         v8::Handle<v8::Value> publicKeyValue = toV8(CryptoKey::create(publicKey), scriptState->context()->Global(), scriptState->isolate());
         v8::Handle<v8::Value> privateKeyValue = toV8(CryptoKey::create(privateKey), scriptState->context()->Global(), scriptState->isolate());
 
         keyPair.set("publicKey", publicKeyValue);
         keyPair.set("privateKey", privateKeyValue);
 
         m_resolver->resolve(keyPair.v8Value());
     }
+    m_resolver = nullptr;
 }
 
 bool CryptoResultImpl::cancelled() const
 {
     return acquireLoad(&m_cancelled);
 }
 
 void CryptoResultImpl::cancel()
 {
     releaseStore(&m_cancelled, 1);
 }
 
 CryptoResultImpl::CryptoResultImpl(ScriptState* scriptState)
     : m_cancelled(0)
 {
     // Creating the WeakResolver may return nullptr if active dom objects have

[eroman, 2014/12/09 19:15:29]

This comment is no longer correct, and I believe the line below can now result
in a use-after-free.

The constructor of ScriptPromiseResolver may set
  m_state = ResolvedOrRejected
When active DOM obects are already stopped.

If that happens, then the call to keepAliveWhilePending() will not add a
reference. So m_resolver will be set to the value of the WeakResolver, however
as soon as the temporary PassRefPtr is destructed, the underlying object's
refcount will reach zero and be freed.

[tasak, 2014/12/11 09:44:39]

I see.
I checked whether ActiveDOMObjectStopped here. If stopped, m_result = nullptr.

     // been stopped. And in the process set m_cancelled to 1.
-    m_resolver = WeakResolver::create(scriptState, this);
+    m_resolver = WeakResolver::create(scriptState, this).get();
 }
 
 ScriptPromise CryptoResultImpl::promise()
 {
     return m_resolver ? m_resolver->promise() : ScriptPromise();
 }
 
 } // namespace blink