[abandoned] IDL attribute "SetHiddenValueTo" to build strong refs to wrapped object's dependen…

This became http://crrev.com/26792002

implement custom IDL attribute "SetHiddenValueTo" to build strong refs to wrapped object's dependencies.

This patch introduces new custom Blink IDL attribute "SetHiddenValueTo". When the attribute is specified, the wrapper is setNamedHiddenReference() to the wrapped object's dependencies specified by its argument. This ensures that the dependencies of wrapped object are kept alive while the wrapper is alive.

This setNamedHiddenReference technique was previously used by NodeIterator/TreeWalker to fix cyclic reference cycles on Blink side, but they were implemented as a "customToV8", which introduced additional code complexity and potential source of bugs. This patch allows a developer to accomplish it with out using "customToV8".

BUG=None

[kouhei (in TOK), 2013-09-10 01:28:39]

haraken, nbarth: PTAL.

[kouhei (in TOK), 2013-09-10 01:30:34]

https://codereview.chromium.org/23532053/diff/1/Source/bindings/scripts/depre...
File Source/bindings/scripts/deprecated_code_generator_v8.pm (right):

https://codereview.chromium.org/23532053/diff/1/Source/bindings/scripts/depre...
Source/bindings/scripts/deprecated_code_generator_v8.pm:4009: 
I think we need to addIncludesForType(<$setHiddenValue return type>) here.
However, I don't know how I can do that...

[Nils Barth (inactive), 2013-09-10 01:50:55]

Perl seems fine, module how to handle additional includes.

Before landing, could you document SetHiddenValueTo on the wiki:
https://sites.google.com/a/chromium.org/dev/blink/webidl/blink-idl-extended-a...
?
Thanks!

https://codereview.chromium.org/23532053/diff/1/Source/bindings/scripts/depre...
File Source/bindings/scripts/deprecated_code_generator_v8.pm (right):

https://codereview.chromium.org/23532053/diff/1/Source/bindings/scripts/depre...
Source/bindings/scripts/deprecated_code_generator_v8.pm:4009: 
On 2013/09/10 01:30:34, kouhei wrote:
> I think we need to addIncludesForType(<$setHiddenValue return type>) here.
> However, I don't know how I can do that...

Discussed offline; issue is that required include cannot be determined from the
SetHiddenValueTo extended attribute itself or the IDL file: it might be some
not-obviously-related C++ class.

Most generic solution is probably to add an 'AdditionalIncludes' extended
attribute to handle these.

haraken: other ideas?

[haraken, 2013-09-10 03:45:09]

https://codereview.chromium.org/23532053/diff/1/Source/core/dom/NodeIterator.idl
File Source/core/dom/NodeIterator.idl (right):

https://codereview.chromium.org/23532053/diff/1/Source/core/dom/NodeIterator....
Source/core/dom/NodeIterator.idl:27: readonly attribute NodeFilter filter;

I'm sorry for my previous comment. Looking at your CL, I began to think that
[SetHiddenValueTo] might not be a good idea...

A better approach would be to add the [KeepAttributeAliveForGC] IDL attribute
(not on an interface but) on an attribute. Specifically, you can write:

  [KeepAttributeAliveForGC] readonly attribute NodeFilter filter;

Then you can know a return type of the attribute and thus determine a header
file to include.

Note that there are two types of [KeepAttributeAliveForGC]'s. The first type is
creating a hidden reference when an interface wrapper is created (e.g.,
NodeIterator::filter). The second type is creating a hidden reference when an
attribute wrapper is created (e.g., HTMLTemplateElement::content). We can
distinguish these two types by using [KeepAttributeAliveForGC] and
[KeepAttributeAliveForGC=Lazily]. In practice, you'll just need to rename
existing [KeepAttributeAliveForGC] with [KeepAttributeAliveForGC=Lazily].

WDYT?

[kouhei (in TOK), 2013-09-10 03:52:36]

On 2013/09/10 03:45:09, haraken wrote:
>
https://codereview.chromium.org/23532053/diff/1/Source/core/dom/NodeIterator.idl
> File Source/core/dom/NodeIterator.idl (right):
> 
>
https://codereview.chromium.org/23532053/diff/1/Source/core/dom/NodeIterator....
> Source/core/dom/NodeIterator.idl:27: readonly attribute NodeFilter filter;
> 
> I'm sorry for my previous comment. Looking at your CL, I began to think that
> [SetHiddenValueTo] might not be a good idea...
> 
> A better approach would be to add the [KeepAttributeAliveForGC] IDL attribute
> (not on an interface but) on an attribute. Specifically, you can write:
> 
>   [KeepAttributeAliveForGC] readonly attribute NodeFilter filter;
> 
> Then you can know a return type of the attribute and thus determine a header
> file to include.
> 
> Note that there are two types of [KeepAttributeAliveForGC]'s. The first type
is
> creating a hidden reference when an interface wrapper is created (e.g.,
> NodeIterator::filter). The second type is creating a hidden reference when an
> attribute wrapper is created (e.g., HTMLTemplateElement::content). We can
> distinguish these two types by using [KeepAttributeAliveForGC] and
> [KeepAttributeAliveForGC=Lazily]. In practice, you'll just need to rename
> existing [KeepAttributeAliveForGC] with [KeepAttributeAliveForGC=Lazily].
> 
> WDYT?

I strongly prefer SetHiddenValueTo for two reasons.

1) KeepAttributeAliveForGC is simply not enough for some cases.

The problem is that the target is not always exposed as an attribute. This is
WebMIDI's case. The midiAccess() ref is not exposed as an attribute accessible
from Javascript.

2) It is confusing.

I think it will be confusing for both users and codegen devs.

[KeepAttributeAliveForGC=Lazily] will be translated as a conditionally added
code in attribute getter.
[KeepAttributeAliveForGC] will introduce its binding at wrap().

I don't think this difference is subtle enough to be absorbed in the attribute
argument.

[Nils Barth (inactive), 2013-09-10 03:55:08]

https://codereview.chromium.org/23532053/diff/1/Source/core/dom/NodeIterator.idl
File Source/core/dom/NodeIterator.idl (right):

https://codereview.chromium.org/23532053/diff/1/Source/core/dom/NodeIterator....
Source/core/dom/NodeIterator.idl:27: readonly attribute NodeFilter filter;
On 2013/09/10 03:45:09, haraken wrote:
> A better approach would be to add the [KeepAttributeAliveForGC] IDL attribute
> (not on an interface but) on an attribute.

Adding [KeepAttributeAliveForGC] to an attribute sounds cleaner!

If there are two types of [KeepAttributeAliveForGC], would it be easier to just
have two extended attributes,
KeepAttributeAliveForGC
KeepAttributeAliveForGCLazily
?

Or, to avoid having to rename existing ones, we could use:
KeepAttributeAliveForGCEagerly
KeepAttributeAliveForGC  << existing
...and then we'd just be adding KeepAttributeAliveForGCEagerly
to attributes here.
?

[Nils Barth (inactive), 2013-09-10 03:58:06]

On 2013/09/10 03:52:36, kouhei wrote:
> I strongly prefer SetHiddenValueTo for two reasons.
> 
> 1) KeepAttributeAliveForGC is simply not enough for some cases.
> 
> The problem is that the target is not always exposed as an attribute. This is
> WebMIDI's case. The midiAccess() ref is not exposed as an attribute accessible
> from Javascript.

OIC -- right, the problem with determining includes was exactly that the target
is not always exposed as an attribute.
We could put an ExtAttr on the attribute if it's exposed and use an interface
ExtAttr if it's not, but that sounds really confusing.
In any case, looks like we need to use an interface ExtAttr (at least
sometimes).

[haraken, 2013-09-10 03:58:47]

> 1) KeepAttributeAliveForGC is simply not enough for some cases.
> 
> The problem is that the target is not always exposed as an attribute. This is
> WebMIDI's case. The midiAccess() ref is not exposed as an attribute accessible
> from Javascript.

I'm a bit confused. Why do we need to create a wrapper for a DOM object that is
not accessible from JavaScript? (I think it's theoretically possible, but it's
very likely that something is mis-designed.)

> 2) It is confusing.
> 
> I think it will be confusing for both users and codegen devs.
> 
> [KeepAttributeAliveForGC=Lazily] will be translated as a conditionally added
> code in attribute getter.
> [KeepAttributeAliveForGC] will introduce its binding at wrap().
> 
> I don't think this difference is subtle enough to be absorbed in the attribute
> argument.

Regarding this, nbarth's suggestion sounds good to me.

[kouhei (in TOK), 2013-09-10 04:38:21]

On 2013/09/10 03:58:47, haraken wrote:
> > 1) KeepAttributeAliveForGC is simply not enough for some cases.
> > 
> > The problem is that the target is not always exposed as an attribute. This
is
> > WebMIDI's case. The midiAccess() ref is not exposed as an attribute
accessible
> > from Javascript.
> 
> I'm a bit confused. Why do we need to create a wrapper for a DOM object that
is
> not accessible from JavaScript? (I think it's theoretically possible, but it's
> very likely that something is mis-designed.)

Let me try to explain this based on specific examples:

a) WebMIDI's case.

MIDIAccess creates MIDIInput. MIDIAccess is not exposed as an readable attribute
from MIDIInput, but MIDIAccess itself is exposed through API, so it has its own
V8 wrappers. It could have, but the API was not decided as that.

MIDIInput's implementation relies on MIDIAccess for doing its job. MIDIInput
receives MIDI data from midi devices, so it receives data via WebKit embedder
API interfaces. Currently the reference to the embedder API is kept in
MIDIAccess[1], and MIDIAccess act as a fan out router for its messages. The
MIDIAccess routes a MIDI packet to its associated port.

While I think it is still possible for each MIDIInput to talk directly to
embedder API, I think it will bloat the number of interfaces involved and
introduce even more additional complexity for life-time management. So I think
the current design is reasonable.

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

b) SVG's case

This is currently WIP but I'm trying to add reference to contextElement(). This
is needed to resolve cyclic reference occuring in Blink side. The TearOff
requires its context element to be alive as a tearoff contains a reference to
the value kept inside the element instance. The SVG patch is currently using
$hasTearOff instead of custom attributes, but using this attribute is more
explicit so I would like to convert it to use this.

> > 2) It is confusing.
> > 
> > I think it will be confusing for both users and codegen devs.
> > 
> > [KeepAttributeAliveForGC=Lazily] will be translated as a conditionally added
> > code in attribute getter.
> > [KeepAttributeAliveForGC] will introduce its binding at wrap().
> > 
> > I don't think this difference is subtle enough to be absorbed in the
attribute
> > argument.
> 
> Regarding this, nbarth's suggestion sounds good to me.

SGTM. I'm ok if they are two different extended attributes.

[haraken, 2013-09-10 05:32:09]

Thanks for the clarification. I read the code base and understand the situation.

My general concern is that it's fragile to set a hidden reference to an
arbitrary DOM object when an interface wrapper is created. Who guarantees that
the DOM object doesn't change while the interface wrapper is alive? (In case of
MIDIAccess and NodeIterator, it happens to be guaranteed though.) I'm afraid
that this kind of fragile generated code can easily produce memory bugs in the
future.

In my understanding, what we do need would be [ReachableFrom=xxx] and
[ReachableTo=xxx] ?

[ReachableFrom=foo, ReachableTo=bar] interface A { ... }

The above implies that:

- If a wrapper of A::foo() exists, then we consider that there is a reference
from the A::foo()'s wrapper to A's wrapper.
- If a wrapper of A::bar() exists, then we consider that there is a reference
from A's wrapper to the A::bar()'s wrapper.

Something like [ReachableFrom] is already implemented as [GenerateIsReachable]
(See how opaqueRootForGC is implemented in V8GCController.cpp). Similarly, we'll
need to implement [ReachableTo]. (The only difference between [ReachableFrom]
and [ReachableTo] is the direction of the reference.)

I think we can solve the problem of MIDIAccess and NodeIterator by using
[ReachableTo].

WDYT?

[kouhei (google), 2013-09-10 05:44:54]

[ReachableTo] SGTM, assuming there is no performance difference. Let's go
this way.

WDYT about AdditionalIncludes? We stlil need it in [ReachableTo].
FYI: GenerateIsReachable avoids this by assuming its return type as Node*,
but I don't want to go that way.



2013/9/10 <haraken@chromium.org>

> Thanks for the clarification. I read the code base and understand the
> situation.
>
> My general concern is that it's fragile to set a hidden reference to an
> arbitrary DOM object when an interface wrapper is created. Who guarantees
> that
> the DOM object doesn't change while the interface wrapper is alive? (In
> case of
> MIDIAccess and NodeIterator, it happens to be guaranteed though.) I'm
> afraid
> that this kind of fragile generated code can easily produce memory bugs in
> the
> future.
>
> In my understanding, what we do need would be [ReachableFrom=xxx] and
> [ReachableTo=xxx] ?
>
> [ReachableFrom=foo, ReachableTo=bar] interface A { ... }
>
> The above implies that:
>
> - If a wrapper of A::foo() exists, then we consider that there is a
> reference
> from the A::foo()'s wrapper to A's wrapper.
> - If a wrapper of A::bar() exists, then we consider that there is a
> reference
> from A's wrapper to the A::bar()'s wrapper.
>
> Something like [ReachableFrom] is already implemented as
> [GenerateIsReachable]
> (See how opaqueRootForGC is implemented in V8GCController.cpp). Similarly,
> we'll
> need to implement [ReachableTo]. (The only difference between
> [ReachableFrom]
> and [ReachableTo] is the direction of the reference.)
>
> I think we can solve the problem of MIDIAccess and NodeIterator by using
> [ReachableTo].
>
> WDYT?
>
>
https://codereview.chromium.**org/23532053/<https://codereview.chromium.org/2...
>



-- 
Kouhei Ueno

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[haraken, 2013-09-10 06:00:13]

On 2013/09/10 05:44:54, kouhei (google) wrote:
> [ReachableTo] SGTM, assuming there is no performance difference. Let's go
> this way.
> 
> WDYT about AdditionalIncludes? We stlil need it in [ReachableTo].
> FYI: GenerateIsReachable avoids this by assuming its return type as Node*,
> but I don't want to go that way.

How about just hard-coding the include files in the code generator? The code
generator is already full of hard-coded #includes, and also, we already know
that it's impossible to control #includes using consistent IDL syntax. (Though
we want to centralize the hard-coded #includes to one place in the new code
generator.)

[haraken, 2013-09-10 06:02:01]

> > [ReachableTo] SGTM, assuming there is no performance difference. Let's go
> > this way.

In theory, it will improve performance.

- [ReachableTo/From] just adds temporary references during a GC, so there would
be no performance concern.
- setHiddenValue() is heavy because V8 internally creates a hash map per
wrapper.

[kouhei (in TOK), 2013-09-10 06:04:51]

> > WDYT about AdditionalIncludes? We stlil need it in [ReachableTo].
> > FYI: GenerateIsReachable avoids this by assuming its return type as Node*,
> > but I don't want to go that way.
> 
> How about just hard-coding the include files in the code generator? The code
> generator is already full of hard-coded #includes, and also, we already know
> that it's impossible to control #includes using consistent IDL syntax. (Though
> we want to centralize the hard-coded #includes to one place in the new code
> generator.)

I'm not sure if I understand this correctly.

Do you mean adding #include in code generator every time we encounter a new
return type needed for ReachableTo? I don't think that is scalable.

[kouhei (in TOK), 2013-09-10 06:07:55]

On 2013/09/10 06:02:01, haraken wrote:
> > > [ReachableTo] SGTM, assuming there is no performance difference. Let's go
> > > this way.
> 
> In theory, it will improve performance.
> 
> - [ReachableTo/From] just adds temporary references during a GC, so there
would
> be no performance concern.
> - setHiddenValue() is heavy because V8 internally creates a hash map per
> wrapper.

Thanks for explanation!

What I was thinking was that toV8() will incur DOMWrapper hashmap lookup, but
allow eliminating a hashmap in v8 side seems like more win.

[haraken, 2013-09-10 07:28:53]

On 2013/09/10 06:04:51, kouhei wrote:
> > > WDYT about AdditionalIncludes? We stlil need it in [ReachableTo].
> > > FYI: GenerateIsReachable avoids this by assuming its return type as Node*,
> > > but I don't want to go that way.
> > 
> > How about just hard-coding the include files in the code generator? The code
> > generator is already full of hard-coded #includes, and also, we already know
> > that it's impossible to control #includes using consistent IDL syntax.
(Though
> > we want to centralize the hard-coded #includes to one place in the new code
> > generator.)
> 
> I'm not sure if I understand this correctly.
> 
> Do you mean adding #include in code generator every time we encounter a new
> return type needed for ReachableTo? I don't think that is scalable.

You're right. My approach won't work well.

However, [AddIncludes] won't be enough either, because you'll need to know the
return type of the method specified by [ReachableFrom/To]. Otherwise, you cannot
generate code that catches the value returned by the method.

What we need would be something like [ReachableFrom=ownerNode^Node], which means
that ownerNode() returns a Node. Needless to say, we should strictly follow the
IDL syntax, so I never want to adopt [ownerNode^Node].

hmm, is there any good idea to annotate the return type of the method specified
in the value of the IDL attribute?

[haraken, 2013-09-10 07:32:21]

> What we need would be something like [ReachableFrom=ownerNode^Node], which
means
> that ownerNode() returns a Node. Needless to say, we should strictly follow
the
> IDL syntax, so I never want to adopt [ownerNode^Node].
> 
> hmm, is there any good idea to annotate the return type of the method
specified
> in the value of the IDL attribute?

Probably [ReachableFrom=Node ownerNode] works fine and looks fine?

[kouhei (in TOK), 2013-09-10 07:35:47]

> However, [AddIncludes] won't be enough either, because you'll need to know the
> return type of the method specified by [ReachableFrom/To]. Otherwise, you
cannot
> generate code that catches the value returned by the method.

Ahh, you are right! Can I have your permission to use a C++ template magic to
extract the return type here??? Then we can procrastinate the problem :)

[haraken, 2013-09-10 07:44:40]

On 2013/09/10 07:35:47, kouhei wrote:
> > However, [AddIncludes] won't be enough either, because you'll need to know
the
> > return type of the method specified by [ReachableFrom/To]. Otherwise, you
> cannot
> > generate code that catches the value returned by the method.
> 
> Ahh, you are right! Can I have your permission to use a C++ template magic to
> extract the return type here??? Then we can procrastinate the problem :)

Depending on how dirty it is :)

I've observed the same problem before somewhere. We already have several IDL
attributes that specify methods. The code generator sometimes wants to know the
return types.

[Nils Barth (inactive), 2013-09-10 09:07:12]

On 2013/09/10 07:32:21, haraken wrote:
> Probably [ReachableFrom=Node ownerNode] works fine and looks fine?

That doesn't work with the grammar (or the current parsers, either Perl or
Python).

Easiest is:
[ReachableFrom=Node|ownerNode]
...which doesn't explicitly say which is which, but just lists the relevant
types;
hopefully that's enough.

If we actually want to specify a function signature, the
ExtendedAttributeNamedArgList
production is suitable.
We have these in IDL files for [NamedConstructor] (unsurprisingly), but I'm not
sure
we actually parse it.

E.g.:
[NamedConstructor=Image(DOMString src)]
BNF:
ExtendedAttributeNamedArgList → identifier = identifier ( ArgumentList )

http://www.w3.org/TR/WebIDL/#idl-extended-attributes

[Nils Barth (inactive), 2013-09-10 09:14:03]

On 2013/09/10 07:32:21, haraken wrote:
> Probably [ReachableFrom=Node ownerNode] works fine and looks fine?

Reading more carefully, since we want to treat these differently:
easiest is:
[
 ReachableFrom=ownerNode,
 ReachableFromReturns=Node,
]

We could add an addition grammar production for these, as suggested above,
but I'm leery of putting arbitrary function signatures in extended attributes
unless it's really helpful.
(It might be -- it sounds like CG often needs this -- but it would be a pretty
big change)