Can you share the test even if it's flaky?

See https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/wSIjYcoCqdY/discussion for more details.

The custom bindings should have code for attribute.is_keep_alive_for_gc.

"flaky" here means for example that the wrapper will be kept alive if you use ignition (--js-flags=--ignition-staging) as this will indeed put the wrapper into a temp variable, while full codegen won't 

The expectation changed at https://chromium.googlesource.com/chromium/src/+/e92d0a685af6a88b47d10a6abf5f9aaf29dcdeb7

The expectation change is because of the incorrect lifetime management, it's unrelated to the CL

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl?rcl=0&l=138
// Keep the wrapper object for the return value alive as long as |this|
// object is alive in order to save creation time of the wrapper object.

FWIW, seeing the comment above, keep_alive_for_gc was designed to improve the performance, not to guarantee the object's lifetime.  Is it correct to rely on keep_alive_for_gc to make an object alive?  I don't think so.

I don't think the comment is correct (although I vaguely remember that I added the comment a couple of years ago...).

is_keep_alive_gc is a mechanism for correctness, not for performance optimization. For example, we're excluding Node objects from the keep-alive list (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scripts/v8_attributes.py?q=is_keep_alive_for_gc&sq=package:chromium&l=356&dr=CSs). This is because the lifetime of Node objects are maintained by the object grouping.

In any case, the fact that the spec has XXX.foo means that there is a strong reference from XXX's wrapper to foo. We must have a way to keep foo alive while XXX's wrapper is alive.

I'll update the comment.

Hmm, I'm confused.

Why is is_keep_alive_gc limited to read-only DOM attributes? I think all DOM attributes that return a wrapper type must have the logic to keep the returned wrapper alive...

I thought it's a performance hack and that's why we apply the hack only to readonly attributes and the targets of the hack are very ad-hoc / heuristic.

Historically this code was written in the old WebKit days, when we were not worrying about wrapper lifetime. So it's possible it's doing a wrong thing about the wrapper lifetime.

What do you think should happen?

I guess a right fix would be to generate traceWrapper that traces all DOM attributes from the wrapper.

Yes, traceWrapper is the right fix, I think.  I think DOM objects must have a strong reference to its dependencies iff it really depends on them (using traceWrapper).  Since the dependencies are not wrriten in Web IDL (XXX.foo doesn't necessarily mean XXX should have a reference to foo), the DOM implementation should write its dependencies on their own using traceWrapper.

It would be nice if Oilpan automatically takes care of traceWrapper.  I think the dependencies are usually represented as follows.
    class DOMObject : public ScriptWrappable {
      Member<WrappableType1> dependency1_;
      Member<WrappableType2> dependency2_;
    };
If we could avoid to write mostly the same thing between trace() and traceWrapper(), that would be great.

> Since the dependencies are not wrriten in Web IDL (XXX.foo doesn't necessarily mean XXX should have a reference to foo)

Are you sure of this? I was thinking that:

- XXX.foo means that XXX should have a strong reference to foo.
- It meas that XXX's wrapper should have a strong reference to the foo's wrapper, because a wrapper and a C++ DOM object are the same thing in the spec.

> It would be nice if Oilpan automatically takes care of traceWrapper.

We considered this but found that it's not that easy because 1) there are many WrappableTypes that don't need to be traced and 2) there are lots of non-WrapperTypes that need to be traced.

> The expectation changed at https://chromium.googlesource.com/chromium/src/+/e92d0a685af6a88b47d10a6abf5f9aaf29dcdeb7

I did not remember this diff.
https://chromium.googlesource.com/chromium/src/+/e92d0a685af6a88b47d10a6abf5f9aaf29dcdeb7%5E%21/#F2

I guess I rebased the expectation wrongly.

The main part of the CL (renaming Event.scoped to Event.composed) is unlikely related to lifetime management.

re: #13

I was just writing purely technically.  There is no specification about reference about DOM attribute AFAIK.  I think it's (technically) okay that a single DOM attribute introduced two references, etc.

For example, there could be a case like CSSCalcLength:
https://drafts.css-houdini.org/css-typed-om/#csscalclength
In this case, there should be a single value behind the object, although the interface has 16 DOM attributes.  It doesn't mean the object has 16 strong references.

IMHO, Web IDL is not a tool to define references.  References should be defined separately.

I'm confused...

> For example, there could be a case like CSSCalcLength:
> https://drafts.css-houdini.org/css-typed-om/#csscalclength
> In this case, there should be a single value behind the object, although the interface has 16 DOM attributes.  It doesn't mean the object has 16 strong references.
> 
> IMHO, Web IDL is not a tool to define references.  References should be defined separately.

Then where are the references defined? In my understanding, XXX.foo defines a strong reference from XXX to foo (and thus XXX's wrapper to foo's wrapper).

In the case of CSSCalcLength, the strong reference doesn't become an issue because the DOM attributes are just double values (they are not wrappers).

> Then where are the references defined? In my understanding, XXX.foo defines a strong reference from XXX to foo (and thus XXX's wrapper to foo's wrapper).

Sometimes it's written in a description of the interface, sometimes it's written in a description of the attribute, sometimes I don't find no description.

> In the case of CSSCalcLength, the strong reference doesn't become an issue because the DOM attributes are just double values (they are not wrappers).

Consider the case of Node.firstChild and Node.lastChild.  There can be no object behind the Node, or a singe object behind the Node, or two or more objects behind the Node.  The number of references is independent from DOM attributes.  I'm just saying that we cannot determine the number of references or referenced objects just from the Web IDL.

As a counter question, which spec is saying that we can determine the references from Web IDL?

> As a counter question, which spec is saying that we can determine the references from Web IDL?

I'm referring to https://html.spec.whatwg.org/multipage/infrastructure.html#garbage-collection

(I think this is what we were discussing in the event-listener's lifetime discussion: https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/oYZryFuovo0/discussion)

> Consider the case of Node.firstChild and Node.lastChild.  There can be no object behind the Node, or a singe object behind the Node, or two or more objects behind the Node.  The number of references is independent from DOM attributes.  I'm just saying that we cannot determine the number of references or referenced objects just from the Web IDL.

Would you elaborate? There must be one object for Node and one object for .firstChild (if .firstChild is not null). Then there should be a strong reference from Node to .firstChild.

Discussed offline, and we agreed (at least mostly).

> I'm referring to https://html.spec.whatwg.org/multipage/infrastructure.html#garbage-collection

Good to know.  However, there is no way to determine if the attribute returns a "pre-existing object to that object" or not just from Web IDL.  That's my only point.

Should we wait for the trace wrapper to be shipped?

Otherwise, I would like to use DOMWrapperWorld::setWrapperReferencesInAllWorlds but it isn't working on workers (issue 625951).

Talked with haraken@. We will wait for trapceWrapper.

The test expectation was changed again:  https://crrev.com/39fc84f0cec650b199de0fa5d155454e2fa2d8a7

TraceWrapper fixed it.

Happy to see that such troubling issues are fixed now \o/