Introduce Promise mapping to the IDL generator

Source/bindings/v8/ScriptPromiseResolver.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 14 matching lines @@
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (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 ScriptPromiseResolver_h
 #define ScriptPromiseResolver_h
 
 #include "bindings/v8/ScopedPersistent.h"
-#include "bindings/v8/ScriptObject.h"
+#include "bindings/v8/ScriptPromise.h"
 #include "bindings/v8/ScriptState.h"
 #include "bindings/v8/ScriptValue.h"
 #include "wtf/RefPtr.h"
 
 #include <v8.h>
 
 namespace WebCore {
 
 class ScriptExecutionContext;
 
 // ScriptPromiseResolver is a class for accessing PromiseResolver methods
 // (fulfill / resolve / reject) from C++ world.
 // ScriptPromiseResolver holds a Promise and a PromiseResolver.
 // All methods of this class must be called from the main thread.
 // Here is a typical usage:
 //  1. Create a ScriptPromiseResolver.
 //  2. Pass the promise object of the holder to a JavaScript program
 //     (such as XMLHttpRequest return value).
 //  3. Detach the promise object if you no long need it.
 //  4. Call fulfill or reject when the operation completes or
 //     the operation fails respectively.
 //
 // Most methods including constructors must be called within a v8 context.
 // To use ScriptPromiseResolver out of a v8 context the caller must
 // enter a v8 context, for example by using ScriptScope and ScriptState.
 //
-class ScriptPromiseResolver : public RefCounted<ScriptPromiseResolver> {
-    WTF_MAKE_NONCOPYABLE(ScriptPromiseResolver);
+class ScriptPromiseResolver {

[yhirano, 2013/09/02 11:58:17]

Can ScriptPromiseResolver be a subclass of ScriptValue?

 public:
-    static PassRefPtr<ScriptPromiseResolver> create(ScriptExecutionContext*);
-    static PassRefPtr<ScriptPromiseResolver> create();
+    ScriptPromiseResolver();
+    explicit ScriptPromiseResolver(ScriptValue resolver);
+    ScriptPromiseResolver(v8::Handle<v8::Value> resolver, v8::Isolate*);
+
+    static ScriptPromiseResolver create(ScriptExecutionContext*);
+    static ScriptPromiseResolver create();
 
     // A ScriptPromiseResolver should be fulfilled / resolved / rejected before
     // its destruction.
     // A ScriptPromiseResolver can be destructed safely without
     // entering a v8 context.
     ~ScriptPromiseResolver();
 
     // Detach the promise object and reject the resolver object with undefined.
     void detach();
 
     // Detach the promise object.
     void detachPromise() { m_promise.clear(); }
 
     // Return true if the following conditions are met:
     //  - The resolver object is not detached.
     //  - The resolver's promise object is in pending state.
     //  - The resolver's resolved flag is not set.
     bool isPending() const;
 
-    ScriptObject promise()
+    ScriptPromise promise()
     {
         ASSERT(v8::Context::InContext());
-        return ScriptObject(ScriptState::current(), m_promise.newLocal(m_isolate));
+        return m_promise;
     }
 
     // Fulfill with a C++ object which can be converted to a v8 object by toV8.
     template<typename T>
     inline void fulfill(PassRefPtr<T>);
     // Resolve with a C++ object which can be converted to a v8 object by toV8.
     template<typename T>
     inline void resolve(PassRefPtr<T>);
     // Reject with a C++ object which can be converted to a v8 object by toV8.
     template<typename T>
     inline void reject(PassRefPtr<T>);
 
     void fulfill(ScriptValue);
     void resolve(ScriptValue);
     void reject(ScriptValue);
 
+    bool isObject() const
+    {
+        return m_resolver.isObject();
+    }
+
+    bool isNull() const
+    {
+        return m_resolver.isNull();
+    }
+
+    bool isUndefinedOrNull() const
+    {
+        return m_resolver.isUndefined() || m_resolver.isNull();
+    }
+
+    v8::Handle<v8::Value> v8Value() const
+    {
+        return m_resolver.v8Value();
+    }
+
 private:
-    ScriptPromiseResolver(v8::Handle<v8::Object> creationContext, v8::Isolate*);
+    ScriptPromiseResolver(v8::Handle<v8::Value> resolver, v8::Handle<v8::Value> promise, v8::Isolate*);
     void fulfill(v8::Handle<v8::Value>);
     void resolve(v8::Handle<v8::Value>);
     void reject(v8::Handle<v8::Value>);
 
     v8::Isolate* m_isolate;
-    ScopedPersistent<v8::Object> m_promise;
-    ScopedPersistent<v8::Object> m_resolver;
+    ScriptPromise m_promise;
+    ScriptValue m_resolver;

[haraken, 2013/09/02 21:44:21]

Won't these cause memory leaks? Specifically, how is it guaranteed that
m_promise or m_resolver doesn't hold a (direct or indirect) reference to a
wrapper of ScriptPromiseResolver?

(Basically, we're trying to kill ScriptValues from the code base, because they
can cause reference cycles.)

I'm curious why you can't use ScopedPersistent instead of ScriptValue. (Note:
ScriptValue creates a reference cycles because ScriptValue creates a strong
reference to a V8 object. On the other hand, ScopedPersistent doesn't create a
reference cycle because ScopedPersistent creates a weak reference to a V8
object.)

[yusukesuzuki, 2013/09/03 01:52:40]

ScriptPromiseResolver is not a native class of DOMObject. It has same semantics
as ScriptValue. So there's no wrapper object of ScriptPromiseResolver as there's
no wrapper object of ScriptValue.
Without making ScopedPersistent<v8::Value> weak explicitly (calling makeWeak), I
think ScopedPersistent<v8::Value> also holds a strong reference to a V8 object.
Is it correct?

[haraken, 2013/09/03 02:12:19]

Sorry, actually I'm not a best reviewer here since I don't fully understand the
reference relationship of Promise. I just wanted to raise general concerns about
ScriptValue:

1) Memory leak. (e.g., wouldn't it be possible that the ScriptValue holds a
reference to some wrapper whose corresponding C++ object holds a strong
reference to the ScriptPromiseResolver?)

2) Isolated world leak. (e.g., are you sure that the ScriptPromiseResolver won't
return the ScriptValue to an isolated world other than the world that created
the ScriptValue?)

I'd be happy if you could add some comments about why using ScriptValue is safe
here.
Correct. So we normally call makeWeak() to break a reference cycle.

Your point is that because makeWeak() hasn't yet been called for the
ScopedPersistent, you can replace the ScopedPersistent with a ScriptValue.
Right?

[yusukesuzuki, 2013/09/03 03:57:45]

Right. This case causes memory leaks.
For example, CryptoOperation object of WebCrypto API[1] is a typical case of
this. CryptoOperation object holds a strong reference to a PromiseResolver. A
PromiseResolver holds a strong reference to callbacks indirectly and callbacks
can hold a strong reference to CryptoOperation object. If users don't fulfill
the CryptoOperation, there's circular reference.
But I remember that this is the reason why CryptoOperation doesn't inherits
Promise in Blink[2].
ScriptPromiseResolver breaks the reference to the promise and resolver when it
is fulfilled or rejected. So there's a convention that the owner of
ScriptPromiseResolver should fulfill or reject it without any user input. This
prevents memory leaks.

But I've found my mistakes in this patch.
In the following code, the resolver is fulfilled or rejected, but
ScriptPromiseResolver holds the reference to the promise and resolver.

    ScriptPromiseResolver a = ScriptPromiseResolver::create();
    ScriptPromiseResolver b = a;
    a.reject(...);
    // At this line, b's resolver is already fulfilled / rejected,
    // but b holds the references.

And recently it is discussed that PromiseResolver is not necessary[3].

Since there's no PromiseResolver user in the current IDL files, so I think
introducing only Promise to ScriptPromise mapping into IDL seems better.

[1]: http://www.w3.org/TR/WebCryptoAPI/
[2]: https://codereview.chromium.org/19082002
[3]: https://mail.mozilla.org/pipermail/es-discuss/2013-August/033007.html
Right.

     bool isPendingInternal() const;
 };
 
 template<typename T>
 void ScriptPromiseResolver::fulfill(PassRefPtr<T> value)
 {
     ASSERT(v8::Context::InContext());
     fulfill(toV8(value.get(), v8::Object::New(), m_isolate));
 }
 
 template<typename T>
 void ScriptPromiseResolver::resolve(PassRefPtr<T> value)
 {
     ASSERT(v8::Context::InContext());
     resolve(toV8(value.get(), v8::Object::New(), m_isolate));
 }
 
 template<typename T>
 void ScriptPromiseResolver::reject(PassRefPtr<T> value)
 {
     ASSERT(v8::Context::InContext());
     reject(toV8(value.get(), v8::Object::New(), m_isolate));
 }
 
 } // namespace WebCore
 
 
 #endif // ScriptPromiseResolver_h