Introduce Promise mapping to the IDL generator

Introduce Promise mapping to the IDL generator

Since Promise will be implemented in V8-side in the
future, we add the following special conversion rules to the IDL code
generator like DOMString -> const String&, Promise -> ScriptPromise.

BUG=266700
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157435

[yusukesuzuki, 2013-09-02 10:55:36]

On 2013/09/02 10:45:16, yusukesuzuki wrote:

The ScriptPromise is based on this CL[1].

[1]: https://codereview.chromium.org/23254004/

[yhirano, 2013-09-02 11:58:16]

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
File Source/bindings/v8/ScriptPromise.h (right):

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
Source/bindings/v8/ScriptPromise.h:41: class ScriptPromise {
Can ScriptPromise be a subclass of ScriptValue?

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
File Source/bindings/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
Source/bindings/v8/ScriptPromiseResolver.h:62: class ScriptPromiseResolver {
Can ScriptPromiseResolver be a subclass of ScriptValue?

[haraken, 2013-09-02 21:44:20]

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/scripts/de...
File Source/bindings/scripts/deprecated_idl_parser.pm (right):

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/scripts/de...
Source/bindings/scripts/deprecated_idl_parser.pm:337: my $nextArgumentList_1 =
'^(\(|::|ByteString|DOMString|Promise|PromiseResolver|Date|\[|any|boolean|byte|double|float|in|int|long|object|octet|optional|sequence|short|unrestricted|unsigned)$';

Do you really need to make these changes to the IDL parser? I guess the IDL
parser can treat "Promise" and "PromiseResolver" without treating them as
primitive types. (The IDL parser is strictly following the Web IDL syntax, and
thus we don't want to add custom syntax to the parser.)

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
File Source/bindings/v8/ScriptPromise.h (right):

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
Source/bindings/v8/ScriptPromise.h:86: ScriptValue m_promise;

Why can't you use ScopedPersistent?

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
File Source/bindings/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
Source/bindings/v8/ScriptPromiseResolver.h:137: ScriptValue m_resolver;

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]

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/scripts/de...
File Source/bindings/scripts/deprecated_idl_parser.pm (right):

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/scripts/de...
Source/bindings/scripts/deprecated_idl_parser.pm:337: my $nextArgumentList_1 =
'^(\(|::|ByteString|DOMString|Promise|PromiseResolver|Date|\[|any|boolean|byte|double|float|in|int|long|object|octet|optional|sequence|short|unrestricted|unsigned)$';
On 2013/09/02 21:44:21, haraken wrote:
> 
> Do you really need to make these changes to the IDL parser? I guess the IDL
> parser can treat "Promise" and "PromiseResolver" without treating them as
> primitive types. (The IDL parser is strictly following the Web IDL syntax, and
> thus we don't want to add custom syntax to the parser.)

Promise and PromiseResolver won't be DOMObject since Promise and PromiseResolver
will be implemented inside of V8 as JavaScript builtin objects such as "Date".
So I think treating them as primitive types is consistent.

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
File Source/bindings/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
Source/bindings/v8/ScriptPromiseResolver.h:137: ScriptValue m_resolver;
On 2013/09/02 21:44:21, haraken wrote:
> 
> 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?

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.


> (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.)

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]

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/scripts/de...
File Source/bindings/scripts/deprecated_idl_parser.pm (right):

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/scripts/de...
Source/bindings/scripts/deprecated_idl_parser.pm:337: my $nextArgumentList_1 =
'^(\(|::|ByteString|DOMString|Promise|PromiseResolver|Date|\[|any|boolean|byte|double|float|in|int|long|object|octet|optional|sequence|short|unrestricted|unsigned)$';

Thanks for the clarification. Makes sense. Would you add a comment like "Promise
and PromiseResolver are not yet in the Web IDL spec but are going to be speced
as primitive types in the future."

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
File Source/bindings/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
Source/bindings/v8/ScriptPromiseResolver.h:137: ScriptValue m_resolver;
> 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.

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.


> 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?

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:44]

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
File Source/bindings/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
Source/bindings/v8/ScriptPromiseResolver.h:137: ScriptValue m_resolver;
On 2013/09/03 02:12:19, haraken wrote:
> 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.

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

> 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?

Right.

[yusukesuzuki, 2013-09-03 08:33:11]

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
File Source/bindings/v8/ScriptPromise.h (right):

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
Source/bindings/v8/ScriptPromise.h:41: class ScriptPromise {
On 2013/09/02 11:58:17, yhirano wrote:
> Can ScriptPromise be a subclass of ScriptValue?

Done.

[yusukesuzuki, 2013-09-03 09:35:54]

https://chromiumcodereview.appspot.com/23479016/diff/6001/Source/bindings/scr...
File Source/bindings/scripts/deprecated_idl_parser.pm (right):

https://chromiumcodereview.appspot.com/23479016/diff/6001/Source/bindings/scr...
Source/bindings/scripts/deprecated_idl_parser.pm:337: my $nextArgumentList_1 =
'^(\(|::|ByteString|DOMString|Promise|PromiseResolver|Date|\[|any|boolean|byte|double|float|in|int|long|object|octet|optional|sequence|short|unrestricted|unsigned)$';
On 2013/09/03 02:12:19, haraken wrote:
> 
> Thanks for the clarification. Makes sense. Would you add a comment like
"Promise
> and PromiseResolver are not yet in the Web IDL spec but are going to be speced
> as primitive types in the future."

Done.

[haraken, 2013-09-03 17:03:07]

The IDL compiler change LGTM.

I'd like to defer the review of the Promise implementation to abarth.

https://chromiumcodereview.appspot.com/23479016/diff/39001/Source/bindings/v8...
File Source/bindings/v8/ScriptPromiseResolver.h (right):

https://chromiumcodereview.appspot.com/23479016/diff/39001/Source/bindings/v8...
Source/bindings/v8/ScriptPromiseResolver.h:114: ScriptPromise m_promise;

I still don't fully understand why this doesn't cause memory leaks nor leaks
between isolated worlds. Would you add a comment about it?

[yhirano, 2013-09-04 02:14:14]

https://codereview.chromium.org/23479016/diff/39001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/23479016/diff/39001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptPromiseResolver.h:114: ScriptPromise m_promise;
On 2013/09/03 17:03:07, haraken wrote:
> 
> I still don't fully understand why this doesn't cause memory leaks nor leaks
> between isolated worlds. Would you add a comment about it?

Users of ScriptPromiseResolver should call detachPromise if they do  not need
the Promise object, as the class comment says. Otherwise a memory leak may be
introduced.
In a similar way, users of ScriptPromiseResolver should call fulfill / resolve /
reject if the operation completes or fails. Otherwise a memory leak may be
introduced.
Destroying the ScriptPromiseResolver object will detach the Promise and the
PromiseResolver objects.

In short, you have to call such methods manually if you hold
ScriptPromiseResolver as a member variable.

[haraken, 2013-09-04 02:20:07]

On 2013/09/04 02:14:14, yhirano wrote:
>
https://codereview.chromium.org/23479016/diff/39001/Source/bindings/v8/Script...
> File Source/bindings/v8/ScriptPromiseResolver.h (right):
> 
>
https://codereview.chromium.org/23479016/diff/39001/Source/bindings/v8/Script...
> Source/bindings/v8/ScriptPromiseResolver.h:114: ScriptPromise m_promise;
> On 2013/09/03 17:03:07, haraken wrote:
> > 
> > I still don't fully understand why this doesn't cause memory leaks nor leaks
> > between isolated worlds. Would you add a comment about it?
> 
> Users of ScriptPromiseResolver should call detachPromise if they do  not need
> the Promise object, as the class comment says. Otherwise a memory leak may be
> introduced.
> In a similar way, users of ScriptPromiseResolver should call fulfill / resolve
/
> reject if the operation completes or fails. Otherwise a memory leak may be
> introduced.
> Destroying the ScriptPromiseResolver object will detach the Promise and the
> PromiseResolver objects.
> 
> In short, you have to call such methods manually if you hold
> ScriptPromiseResolver as a member variable.

Makes sense. Thanks for the clarification!

[yusukesuzuki, 2013-09-04 06:36:59]

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
File Source/bindings/v8/ScriptPromise.h (right):

https://codereview.chromium.org/23479016/diff/6001/Source/bindings/v8/ScriptP...
Source/bindings/v8/ScriptPromise.h:41: class ScriptPromise {
On 2013/09/03 08:33:11, yusukesuzuki wrote:
> On 2013/09/02 11:58:17, yhirano wrote:
> > Can ScriptPromise be a subclass of ScriptValue?
> 
> Done.

ScriptPromise may have different semantics from ScriptValue in the future.
So I've reverted the change of making ScriptPromise a subclass of ScriptValue.

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:98: # PRIMITIVE_TYPES doesn't
contain Promise.
Since V8 doesn't provide Promise primitive object currently,
adding Promise type to PRIMITIVE_TYPES causes compile errors.

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/scripts/d...
File Source/bindings/scripts/deprecated_code_generator_v8.pm (right):

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/scripts/d...
Source/bindings/scripts/deprecated_code_generator_v8.pm:154: # primitiveTypeHash
doesn't contain Promise.
Since V8 doesn't provide Promise primitive object currently,
adding Promise type to primitiveTypeHash causes compile errors.

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptPromise.h (right):

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptPromise.h:45: //
Added the comment about possible memory leaks.

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptPromiseResolver.h:69: //
Added the comment about possible memory leaks.

[yhirano, 2013-09-04 07:13:17]

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptPromise.h (right):

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptPromise.h:45: //
On 2013/09/04 06:37:00, yusukesuzuki wrote:
> Added the comment about possible memory leaks.

> So holding ScriptPromise as a member variable causes...

"So holding a ScriptPromise as a member variable in a DOM object causes..." is
better, I think.

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptPromiseResolver.h:55: //  3. Detach the promise object
if you no long need it.
Can you fix an existing typo? no long => no longer
Sorry, it was my typo.

[yusukesuzuki, 2013-09-04 08:02:07]

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptPromise.h (right):

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptPromise.h:45: //
On 2013/09/04 07:13:17, yhirano wrote:
> On 2013/09/04 06:37:00, yusukesuzuki wrote:
> > Added the comment about possible memory leaks.
> 
> > So holding ScriptPromise as a member variable causes...
> 
> "So holding a ScriptPromise as a member variable in a DOM object causes..." is
> better, I think.

Done. Thanks!

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/23479016/diff/51001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptPromiseResolver.h:55: //  3. Detach the promise object
if you no long need it.
On 2013/09/04 07:13:17, yhirano wrote:
> Can you fix an existing typo? no long => no longer
> Sorry, it was my typo.

Done.

[yhirano, 2013-09-04 09:17:46]

lgtm

[yusukesuzuki, 2013-09-06 03:23:01]

Hi abarth, could you take a look at this CL?

[abarth-chromium, 2013-09-06 05:17:47]

How does a ScriptPromise differ from a ScriptValue?

[abarth-chromium, 2013-09-06 05:18:48]

I can see the value in teaching the code generator about the Promise type, but
why not map it to a ScriptValue?

[yusukesuzuki, 2013-09-06 06:57:49]

On 2013/09/06 05:17:47, abarth wrote:
> How does a ScriptPromise differ from a ScriptValue?

ScriptPromise isn't much different from ScriptValue now.
But by creating the ScriptPromise class, we can extend it to perform operations
on Promises from C++ world, such as adding `then` method to it. When extending
it, it becomes different from ScriptValue.

Memory leaks cause when a ScriptPromise is held as a member variable.
So semantics may change in the future.
This is why currently ScriptPromise isn't the subclass of ScriptValue.
But if it is preferable that ScriptPromise is the subclass of ScriptValue, I'll
do so.

[yusukesuzuki, 2013-09-06 06:59:21]

On 2013/09/06 05:18:48, abarth wrote:
> I can see the value in teaching the code generator about the Promise type, but
> why not map it to a ScriptValue?

We can type check the APIs returning Promises.

[abarth-chromium, 2013-09-06 16:00:23]

I would probably start out with ScriptPromise just being a typedef for
ScriptValue until we add those extra features.  Otherwise, LGTM

[commit-bot: I haz the power, 2013-09-06 16:00:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukesuzuki@chromium.org/23479016/21001

[commit-bot: I haz the power, 2013-09-06 16:01:06]

Failed to apply patch for Source/bindings/scripts/code_generator_v8.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/scripts/code_generator_v8.py
  Hunk #1 FAILED at 84.
  Hunk #2 FAILED at 91.
  Hunk #3 FAILED at 211.
  3 out of 3 hunks FAILED -- saving rejects to file
Source/bindings/scripts/code_generator_v8.py.rej

Patch:       Source/bindings/scripts/code_generator_v8.py
Index: Source/bindings/scripts/code_generator_v8.py
diff --git a/Source/bindings/scripts/code_generator_v8.py
b/Source/bindings/scripts/code_generator_v8.py
index
0e647b49f749dc6951e431c48ed5340f24398a0a..b7c8ee4cafb4ed780014e8b48129b7252df39fa2
100644
--- a/Source/bindings/scripts/code_generator_v8.py
+++ b/Source/bindings/scripts/code_generator_v8.py
@@ -84,6 +84,7 @@ CPP_TYPE_SPECIAL_CONVERSION_RULES = {
     'byte': 'int',
     'boolean': 'bool',
     'DOMString': 'const String&',
+    'Promise': 'ScriptPromise',
 }
 CPP_UNSIGNED_TYPES = set([
     'octet',
@@ -91,6 +92,10 @@ CPP_UNSIGNED_TYPES = set([
     'unsigned long',
     'unsigned short',
 ])
+# Promise is not yet in the Web IDL spec but is going to be speced
+# as primitive types in the future.
+# Since V8 dosn't provide Promise primitive object currently,
+# PRIMITIVE_TYPES doesn't contain Promise.
 PRIMITIVE_TYPES = set([
     'boolean',
     'void',
@@ -211,6 +216,8 @@ def uncapitalize(name):
 def includes_for_type(data_type):
     if primitive_type(data_type) or data_type == 'DOMString':
         return set()
+    if data_type == 'Promise':
+        return set(['ScriptPromise.h'])
     if array_or_sequence_type(data_type):
         return includes_for_type(array_or_sequence_type(data_type))
     return set(['V8%s.h' % data_type])

[yusukesuzuki, 2013-09-06 18:03:19]

Thanks. I'll rebaseline it.

[commit-bot: I haz the power, 2013-09-06 23:48:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukesuzuki@chromium.org/23479016/73001

[commit-bot: I haz the power, 2013-09-07 00:39:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-09 03:59:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukesuzuki@chromium.org/23479016/85001

[commit-bot: I haz the power, 2013-09-09 04:49:47]

Change committed as 157435