Make CryptoResultImpl not to use WeakPtr.

Source/modules/crypto/CryptoResultImpl.h

 /*
  * 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 54 matching lines @@
     virtual void completeWithBoolean(bool) override;
     virtual void completeWithKey(const WebCryptoKey&) override;
     virtual void completeWithKeyPair(const WebCryptoKey& publicKey, const WebCryptoKey& privateKey) override;
     virtual bool cancelled() const override;
 
     // If called after completion (including cancellation) will return an empty
     // ScriptPromise.
     ScriptPromise promise();
 
 private:
-    class WeakResolver;
+    class Resolver;
     explicit CryptoResultImpl(ScriptState*);
 
+    void clearResolver();
     void cancel();
 
-    WeakPtr<ScriptPromiseResolver> m_resolver;
+    // Since ScriptPromiseResolver destroys itself, we should not use RefPtr

[haraken, 2014/12/12 08:49:31]

I still don't fully understand why we cannot make it a
RefPtr<ScriptPromiseResolver>. As far as I understand, ScriptPromiseResolver
doesn't destroy itself but just deref()'s itself when the ScriptPromiseResolver
is cleared. ScriptPromiseResolver is destroyed when the last reference is gone.

So I think we can just make this a RefPtr<ScriptPromiseResolver> and clear the
RefPtr in clearResolver(). It seems safer than having a raw pointer to the
ScriptPromiseResolver.

[eroman, 2014/12/12 23:32:54]

I had some concerns on this explained in earlier comments:

CryptoResultImpl may be deleted from a different thread. ScriptPromiseResolver
is not RefCountedThreadSafe. Therefore it is misleading to have a RefPtr<> since
it should not be releasing a reference during the destructor

[haraken, 2014/12/15 01:34:59]

Thanks, makes sense.

tasak@: Let's update a comment accordingly.

BTW, it still looks strange that CryptoResultImpl and CryptoResultImpl::Resolver
are referencing to each other using a raw pointer. Should we make
CryptoResultImpl::Resolver::m_result a RefPtr<CryptoResultImpl>? (In the
previous code, CryptoResultImpl::Resolver::m_result was a
RefPtr<CryptoResultImpl>. We can just keep it.)

+    // here. So to avoid using stale pointer, m_resolver should be cleared

[haraken, 2014/12/12 08:49:31]

a stale pointer

+    // after resolving.

[haraken, 2014/12/12 08:49:31]

after resolving => when getting resolved

[tasak, 2014/12/15 05:26:46]

Updated the comment.

+    // FIXME: ScriptPromiseResolver should not be exported.
+    // Instead, use ScriptPromise.
+    ScriptPromiseResolver* m_resolver;

[haraken, 2014/12/12 08:49:31]

Shall we change ScriptPromiseResolver* to CryptoResultImpl::Resolver* ?

(But I agree that a right way to fix this problem is to change the
ScriptPromiseResolver* to a ScriptPromise. We should try this in a follow-up
CL.)

     volatile int m_cancelled;
 };
 
 } // namespace blink
 
 #endif