Introduce Promise mapping to the IDL generator

Source/bindings/v8/ScriptPromiseResolver.h

Index: Source/bindings/v8/ScriptPromiseResolver.h
diff --git a/Source/bindings/v8/ScriptPromiseResolver.h b/Source/bindings/v8/ScriptPromiseResolver.h
index 18d30523fad4eef50394bc20bae123dba4888b14..64ce6e469a06104adeeae54313e25fa6ad1dfcdd 100644
--- a/Source/bindings/v8/ScriptPromiseResolver.h
+++ b/Source/bindings/v8/ScriptPromiseResolver.h
@@ -32,7 +32,7 @@
 #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"
@@ -59,11 +59,14 @@ class ScriptExecutionContext;
 // 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.
@@ -83,10 +86,10 @@ public:
     //  - 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.
@@ -103,15 +106,35 @@ public:
     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;
 };