[webcrypto] Fix TODO regarding asynchronous completion of crypto operations.

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 16 matching lines @@
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #ifndef CryptoResultImpl_h
 #define CryptoResultImpl_h
 
 #include "bindings/v8/ScriptPromise.h"
 #include "core/platform/CryptoResult.h"
 #include "public/platform/WebCrypto.h"
+#include "wtf/Assertions.h"
 #include "wtf/Forward.h"
+#include "wtf/Threading.h"
 
 namespace WebCore {
 
 class ScriptPromiseResolver;
+class ScriptState;
 
 // Wrapper around a Promise to notify completion of the crypto operation.
 // Platform cannot know about Promises which are declared in bindings.
 class CryptoResultImpl : public CryptoResult {
 public:
     ~CryptoResultImpl();
 
     static PassRefPtr<CryptoResultImpl> create(ScriptPromise);
 
     virtual void completeWithError() OVERRIDE;
     virtual void completeWithBuffer(const blink::WebArrayBuffer&) OVERRIDE;
     virtual void completeWithBoolean(bool) OVERRIDE;
     virtual void completeWithKey(const blink::WebCryptoKey&) OVERRIDE;
     virtual void completeWithKeyPair(const blink::WebCryptoKey& publicKey, const blink::WebCryptoKey& privateKey) OVERRIDE;
 
 private:
     explicit CryptoResultImpl(ScriptPromise);
     void finish();
+    void CheckValidThread() const;
 
     RefPtr<ScriptPromiseResolver> m_promiseResolver;
+
+    // This ScriptState should outlive m_promiseResolver, as it is kept alive by

[eroman, 2014/01/10 05:21:13]

Please confirm whether this interpretation is correct (FontFaceSet.cpp is doing
the same thing, but I am not completely sure what controls the lifetime of
ScriptState).

[abarth-chromium, 2014/01/10 15:25:03]

This looks wrong.  Why should ScriptValue keep the ScriptState object alive?  If
you want to retain the ScriptState, you can store it as a member. It's a value
type.

[eroman, 2014/01/10 19:10:26]

Thanks abarth!

I modeled this after FontFaceSet.cpp [*] which similarly stores the naked
pointer to a ScriptState.

I chatted with aklein over IRC, and we tried to trace the code in
FontFaceSet.cpp to understand why that usage of naked ScriptState* is safe. My
understanding for the pattern follows:

(1) ScriptState::current() lives only as long as the underlying v8::Context (it
sets a weak callback to delete itself).

(2) ScriptPromiseResolver holds a ScriptPromise which holds a ScriptValue for
the V8Promise.

(3) V8Promise keeps the v8::Context alive. (Perhaps this assumption is
incorrect?)

(4) Therefore if the v8::Context is being kept alive by the
ScriptPromiseResolver, then the pointer returned by ScriptState::current() is
also still alive.


Regarding saving ScriptState as a value type - note that ScriptState is not
copiable so the result from ScriptState::current() cannot be duplicated. Morever
copying it would not be safe since it can be deleted from the weak callback.

Do you have other recommendations on how to ensure that the v8::Context is alive
before re-entering the ScriptPromiseResolver? Browsing through the code I came
across ContextLifecycleObserver (used by ActiveDOMObject) which keeps track of
whether the ExecutionContext is still alive. I presume that aliveness of
ExecutionContext implies that the v8::Context is alive and hence ScriptState is
alive.


[*]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[abarth-chromium, 2014/01/14 04:01:06]

You shouldn't need to keep a ScriptState.  Take a look at how other callbacks
work.  Basically, you keep an ExecutionContext and a DOMWrapperWorld.  You can
use those two together to get a v8::Context aka ScriptState:

https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...

[eroman, 2014/01/14 20:40:09]

Would it be appropriate to make CryptoResultImpl inherit from ActiveDOMCallback
to get the ExecutionContext lifecycle handling, or should I reimplement that
logic directly off of ContextLifecycleObserver?

+    // a ScriptValue held by the resolver.
+    ScriptState* m_scriptState;
+
+#if !ASSERT_DISABLED
+    ThreadIdentifier m_owningThread;
     bool m_finished;
+#endif
 };
 
 } // namespace WebCore
 
 #endif