I think this is a right way to go.

I thought we'd trace wrapper objects in the following way.

  // V8Window.h
  static void V8Window::traceWrappers(WrapperVisitor* visitor, ScriptWrappable* scriptWrappable) {
    DOMWindow* window = scriptWrappable->toImpl<DOMWindow>()
    visitor->trace(window);
  }

  // DOMWindow.cpp
  DEFINE_TRACE(DOMWindow) {
    visitor->trace(m_location);
    ParentClass::trace(visitor);
    MarkV8WrapperObjectAliveIfAny(this);  // NEW
  }

  // Location.cpp
  DEFINE_TRACE(Location) {
    MarkV8WrapperObjectAliveIfAny(this);  // NEW
  }

Given that Oilpan correctly traces all the DOM objects which are alive, MarkV8WrapperObjectAliveIfAny() makes window.location alive.  MarkV8WrapperObjectAliveIfAny marks the V8 wrapper objects (in all worlds?) if and only if the wrapper object already exists.

We can update DEFINE_TRACE macro so that it automatically performs MarkV8WrapperObjectAliveIfAny if |this| is a ScriptWrappable.

#0: Not sure I understood correctly. If you are trying to keep those fields alive, why not just add them to the corresponding DEFIEN_TRACE_WRAPPERS(ClassName) visitor? If that's not possible, can we think about creating the proper object abstraction?

(I'd rather get away without any IDL magic if we can, that's why I am asking.)

Yeah, I think mlippautz's idea will work.

We can generate something like this:

DEFINE_TRACE(V8Window) {
    if (locationWrapper exists)
     visitor->trace(locationWrapper);
}

Probably, I'm lost.  Is it different from #2?
Or, are you going to define DEFINE_TRACE for V8Window, not for DOMWindow?

#2 So the issue today is that wrapper tracing is inconsistent for fields. So we don't need to mark our own v8 wrapper--that happens by virtue of calling traceWrappers(). The problem is:

DEFINE_TRACE(DOMWindow) {
  visitor->trace(m_location);  <-- this line is missing
  ParentClass::trace(visitor);
  MarkV8WrapperObjectAliveIfAny(this);
}

So the question is if we should try to autogenerate the code for this or not.

Originally, I thought that since the IDL compiler knows the C++ class and the methods backing attributes, it would be nice if the IDL compiler could automatically do it. However, my proposal has disadvantages too--some attributes are lazily created (for example, window.locationbar), so tracing would trigger creation of all these things =(

#3 We can do that, but see my previous point--originally, I was hoping we could establish this relationship automatically (like how we do it today with V8HiddenValue), rather than having to duplicate this knowledge out into traceWrappers(). My concern is it's really easy to forget to trace something, and then you'll end up with mysteriously disappearing expandos.

#4 I think we probably still want it on DOMWindow, right? V8Window is mostly just a collection of helper methods, without data fields.

So to summarize:

- Keep V8HiddenValue. It just works, but we want to deprecate/remove V8HiddenValue. It can also lead to redundant tracing of wrappers. v8 will trace from the holder object, and if the member is also added to traceWrappers() (because you didn't realize IDL is already autotracing), it will be traced from Blink as well.

- Automatically trace attribute getters in V8Window::traceWrappers. It just works, but prevents lazy creation from working. It also has the same problem of duplicated tracing work if you aren't careful and don't realize the auto-generated code is already tracing the wrapper.

- Manually trace in the C++ class's DEFINE_TRACE_WRAPPERS(). It requires manually writing all this code, and it's possible to forget to trace certain wrappers. Even if a compiler plugin enforces that we trace all TraceWrapperMember fields, there's currently no requirement for ScriptWrappable to be stored in a TraceWrapperMember. As a result, it could be stored in a Persistent or a Member, and then the plugin will still miss it.

Given this, I guess we should just manually trace. However, we should implement some plugin checks, and we should decide if we want to require ScriptWrappable GC objects to be stored in distinct types from the base Oilpan pointers. However, this also means that we get even more smart pointer types...

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/DOMWindow.cpp?rcl=5429b39dda87c47ace86635ab4cf998255ced5fb&l=449

DOMWindow.cpp:
DEFINE_TRACE(DOMWindow) {
  visitor->trace(m_frame);
  visitor->trace(m_location);
  EventTargetWithInlineData::trace(visitor);
}

Are we really missing that line?  At least, I see m_location in DOMWindow's case.

I think that it is a fundamental requirement from Oilpan to write
    visitor->trace(m_member);
if the class has
    Member<Type> m_member;
Otherwise, Oilpan may collect m_member.
Then, we should have such a line, shouldn't we?


My understanding is that V8 GC and Oilpan will be perfectly connected and traceable each other, including wrapper object <--> Blink DOM object (in all worlds).  Then, what else do we need to keep wrapper objects alive?

#7

There are two different tracing methods now. One is DEFINE_TRACE(DOMWindow), which is only used for Oilpan tracing.

However, for wrapper tracing, we have DEFINE_TRACE_WRAPPERS(). Since DOMWindow does not have one, it simply uses DEFINE_TRACE_WRAPPERS(EventTarget) instead (and thus never traces the location wrapper).

Thanks for helping me understand better.  Could you help me more if possible?

Then, my question is why we cannot connect DEFINE_TRACE and DEFINE_TRACE_WRAPPERS to each other.  V8 GC and Oilpan cannot collaborate for some reasons?  Do we need to have V8 GC marking phase and Oilpan marking phase separated for some reasons?

Even if we'd like V8 GC and Oilpan to be somewhat separated, why we cannot re-use the Oilpan's graph?
For example, why we cannot do like the following?
    visitor = new WrapperOnlyMarkingVisitor;
    // WrapperOnlyMarkingVisitor does not touch Oilpan things.
    kick_DEFINE_TRACE_with(visitor, rootDOMWindow);
    // DEFINE_TRACE is used with |visitor| which traces Oilpan's
    // graph and collects corresponding wrapper objects.

I'm thinking this way:
  a) We already have the graph of DOM objects (Oilpan's graph)
  b) We can map a DOM object to wrapper objects in all worlds
then, we should already have had enough info to mark all living wrapper objects, and there should be theoretically no need to code-generate another graph.

(Sorry, my comment #4 had typo and was really confusing. Let me clarify my thoughts on this reply :-)

First of all, what I really want to do is to trace (not only readonly DOM attributes but) all DOM attributes so that the pointed-to wrappers are kept alive as long as the holder object is alive. If the spec has window.xyz, window should trace xyz.

The spec is not crystal clear about what DOM attributes should be kept alive when, but my understanding is that if the spec has window.xyz, it indicates that the spec is expecting that xyz is kept alive as long as window is alive. Correct me if I'm wrong.

> - Manually trace in the C++ class's DEFINE_TRACE_WRAPPERS(). It requires manually writing all this code, and it's possible to forget to trace certain wrappers. Even if a compiler plugin enforces that we trace all TraceWrapperMember fields, there's currently no requirement for ScriptWrappable to be stored in a TraceWrapperMember. As a result, it could be stored in a Persistent or a Member, and then the plugin will still miss it.

I want to avoid manual tracing as much as possible :) In theory, the IDL compiler knows all DOM attributes, so it should try to auto-generate the code to trace the DOM attributes.

> - Keep V8HiddenValue. It just works, but we want to deprecate/remove V8HiddenValue. It can also lead to redundant tracing of wrappers. v8 will trace from the holder object, and if the member is also added to traceWrappers() (because you didn't realize IDL is already autotracing), it will be traced from Blink as well.

The redundant tracing is just fine. ScriptWrappableVisitor::markWrapper returns without doing anything if the target object is already marked.

Do you think it would be an option to replace the V8HiddenValue with V8PrivateProperty (instead of auto-generating TRACE_WRAPPERS)?

> - Automatically trace attribute getters in V8Window::traceWrappers. It just works, but prevents lazy creation from working. It also has the same problem of duplicated tracing work if you aren't careful and don't realize the auto-generated code is already tracing the wrapper.

I like this approach the most, if possible.

Regarding the redundant tracing, it's fine.

Regarding the lazy wrapper creation, would it be possible to auto-generate TRACE_WRAPPERS that checks if a wrapper exists and trace the wrapper only when it exists? You can check it by calling DOMDataStore::contains().

Another concern would be performance. Remember that V8HiddenValue is also used to improve performance of DOM attribute getters. If we replace it with the wrapper tracing, we will lose the performance gain. (In long term, I want to address the performance issue by attribute caching but the infrastructure is not yet ready.)

#9: A V8 GC and an Oilpan GC are totally separated things. They are not (yet?) a unified GC. They work independently.

The reason we have DEFINE_TRACE and DEFINE_TRACE_WRAPPERS separately is that an object graph traced by a V8 GC is a (very) subset of an object graph traced by an Oilpan GC. It's too wasteful for a V8 GC to trace the entire object graph of Oilpan. A V8 GC is interested in tracing an object graph defined by "wrapper reachability" whereas an Oilpan GC is interested in tracing an object graph defined by "object reachability". The former graph is much smaller than the latter graph.

For more details, see the "Wrapper reachability != DOM object reachability" section of this document:

https://docs.google.com/document/d/1gu7NwxuQ9fLWRRBjbH1MGKr7RZEx9ESRCZ3P4KwvP68/edit#heading=h.kifmz18emfbu

Re #11: That explanation makes sense.  If there is a big difference of reachability between them, then making a small dedicated graph makes sense.

By the way, what the spec is saying is NOT to make attributes alive.
The spec clearly says that, at least for platform objects, there must be the unique ES object corresponding to an IDL value (= platform object).
https://heycam.github.io/webidl/#es-interface

So, whether or not window.xyz needs to be alive is determined as follows.
a) ES object |window| is alive
b) IDL value of |window| is alive
c) IDL value of |window| has a reference to an IDL value of |xyz|
   (there is a reference from a DOM object to a DOM object internally.)
d) IDL value of |xyz| is alive
e) ES object |xyz| is alive
Note that there is no word of "attribute" in the above reason, and the same theory applies to anything, including DOM operations, etc.  The key reason is that IDL value |window| has a reference to an IDL value |xyz|.  Otherwise, there is no need to make |xyz| alive.

This is why I first thought of using Oilpan's graph (= graph of IDL values).

Anyway, thanks for the explanation.

Ack to #10/#11. 

I just wanted to emphasize again that we shouldn't require any conditionals to avoid duplicated tracing. The infra uses markbits to filter out anything that has already been traced.

mlippautz@: This is another bug we want to fix with the wrapper tracing.

#12: As far as I can tell, all the conditions of liveness you mentioned can be expressed by wrapper tracing. 

This is mainly a summary to (hopefully) get us on the same page wrt current state.

I just had a brief look at the uses of V8HiddenValue::setHiddenValue when used for keeping alive *readonly* properties. There might be cases that are more complicated but I tried to check the majority of uses first.

As an example, I was looking at V8CSSFontFaceRule and its styleAttributeGetter [1]. Just looking at this use case, it should be trivial to support this with tracing. As far as I see, doing this manually, we would just need to trace CSSFontFaceRule::m_propertiesCSSOMWrapper and be done.

The problem I see with inferring and generating that directly from IDL is that 
a) we do not generate the C++ member fields for the DOM, so while we could add tracing support in the IDL compiler, we'd still need to manually change the Member<> to a TraceWrapperMember<>
b) we only generate the outer layer of tracing automatically, i.e., we generate the trace method for V8CSSFontFaceRule [2]. I dislike the approach of tracing inner fields in the outer traceWrappers method as this essentially breaks our abstractions.

dcheng,haraken: Does the explanation and summary make sense? Not sure where we would go from here if we make automatic generation of tracing from IDL a requirement.

Also, what is the problem of lazy creation? We would just create the attributes lazily as we do now and trace wrappers. If attributes are created then they will be traced, otherwise tracing will bail out.

[1] https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8CSSFontFaceRule.cpp?sq=package:chromium&l=57
[2] https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8CSSFontFaceRule.h?sq=package:chromium&l=44

I thought about this a bit more.

- I'm not sure if we should unconditionally trace all DOM attribtues. Some DOM attribute getters might have a side effect.

- We should probably add a [TraceWrapper] IDL extended attribute to DOM attributes that should be traced. Many DOM attributes will end up with having [TraceWrapper].

- If we have [TraceWrapper] on Foo.bar, we auto-generate the following code:

V8Foo::traceWrappers() {
  Foo* foo = toImpl(...);
  Bar* bar = foo->bar();
  if (DOMDataStore::current()->contains(bar)) {
    traceWrappers(ToV8(foo), ToV8(bar));  // foo => bar
  }
}

- However, I have a mixed feeling now :) Rather than introducing [TraceWrapper] and auto-generate mysterious code, do you think it's simpler to just manually changing the C++ implementation to use TRACE_WRAPPERS()? i.e., we edit Foo.cpp and define TRACE_WRAPPERS() to trace from Foo to Bar.

It seems like doing it in IDL is going to be pretty complex. My worry about fixing TRACE_WRAPPERS() implementations to do it is that we'll forget it somewhere and have random parts of web-exposed interfaces behave inconsistently WRT keeping wrappers live. If this is somehow enforced by something, then I would be OK with it.

Agreed.

I'm now leaning toward to manually implementing TRACE_WRAPPERS on the C++ side. It's straightforward and aligns with what Node.firstChild/lastChild etc are doing.

The remaining question is how to verify that we are not missing necessary TRACE_WRAPPERS. (I'm thinking about it.)

Can clang detect a Member<T> member in the class where T inherits from ScriptWrappable?  Then, we'd need TRACE_WRAPPERS there.

Based on the discussion in this bug and CLs like https://codereview.chromium.org/2826393004/, it seems like the right approach is to just keep converting things over to use wrapper tracing, rather than trying to do anything automatically.

Do we want to keep this bug open to track that work? Is there an easy way to quantify how much work is left in this area?

Yes, let's use this bug. I chatted with mlippautz@ last week and I'm planning to find an eng in Tokyo who can work on this.

> Is there an easy way to quantify how much work is left in this area?

Can we count # of DOM attributes that support the wrapper tracing?