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

Source/modules/crypto/CryptoResultImpl.h

Index: Source/modules/crypto/CryptoResultImpl.h
diff --git a/Source/modules/crypto/CryptoResultImpl.h b/Source/modules/crypto/CryptoResultImpl.h
index a0f39ed8f35e04455a66ace1e4feeebec2d78b7f..39ae66fcd84b615ab91517480347230d583ae837 100644
--- a/Source/modules/crypto/CryptoResultImpl.h
+++ b/Source/modules/crypto/CryptoResultImpl.h
@@ -34,11 +34,14 @@
 #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.
@@ -57,9 +60,18 @@ public:
 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