jochen@: If you don't like introducing v8::Persistent::CreationContext API (https://codereview.chromium.org/1627233002/), please suggest an alternative idea.

Note that traceWrapper cannot replace all hasPendingActivity()s because there're scenarios where DOM needs to keep alive V8 wrappers (e.g., XHR's event, ImageElement's onload event etc).

In any case, we need a way to fix the leaks before enabling traceWrapper.

I'm a little bit confused here, aren't active dom objects orthogonal to object grouping/wrapper tracing? If there is a leak with wrapper tracing, there is one with object grouping as well. Both wrapper tracing and object grouping will keep object alive as long as it returns "true" from hasPendingActivity.

Now if there is a leak, isn't it a problem of wrongly defined hasPendingActivity? If object depends on lifetime of its window object, that should be expressed in hasPendingActivity, right? Sorry if I miss some deeper insight.

Your understanding is correct.

hasPendingActivity and traceWrapper are orthogonal things. traceWrapper will be able to replace some of the hasPendingActivity's and thus fix their leaks, but not everything. This is because the two concepts are orthogonal things.

> Now if there is a leak, isn't it a problem of wrongly defined hasPendingActivity? If object depends on lifetime of its window object, that should be expressed in hasPendingActivity, right?

Correct. What I want to do here is to fix leaks of existing hasPendingActivity's. However, I don't want to insert a window->isDetached() check to all hasPendingActivity methods (it's not maintainable). Alternately, I want to insert the window->isDetached check to the place where V8GCController dispatches hasPendingActivity. Then I need the CreationContext API.

Is it really true that all active dom objects should return have hasPendingActivity return false when window->isDetached?

To me active dom object looks like a nice clean abstraction, and I wouldn't mind having window->isDetached check in hasPendingActivity. If it isn't maintainable, how about something simpler, like template method pattern?

hum hum, making hasPendingActivity return false once the window is detached, however, will break the required JS/DOM semantics.

> Is it really true that all active dom objects should return have hasPendingActivity return false when window->isDetached?

I think yes. Per the spec, almost all DOM objects should stop working when its browsing context is detached. (The only subtle exception I'm aware of is objects that may be used in unload handlers.) In any case, given the current situation where a lot of objects are leaking, it's much better to forcibly stop the objects when the window is detached.

> To me active dom object looks like a nice clean abstraction, and I wouldn't mind having window->isDetached check in hasPendingActivity. If it isn't maintainable, how about something simpler, like template method pattern?

What do you mean by the template pattern?

> making hasPendingActivity return false once the window is detached, however, will break the required JS/DOM semantics

Do you know an example that can break?

Another option is to make ActiveScriptWrappables inherit from ContextLifecycleObserver, so that ActiveScriptWrappable::hasPendingActivity can return false after the context has been detached. (We're already doing this in lots of ActiveScriptWrappables. We can do this for everything.) The downside of this approach is that it adds extra overhead to ContextLifecycleObserver. It contradicts with the effort of reducing # of ContextLifecycleObservers to decrease the overhead.

well, anything that puts JS properties on the wrapper. Imagine you put some JS properties on BatteryManager and then get it after the frame was detached. Since we can't disallow running scripts in detached frames, I don't think we can just break active dom objects in detached frames.

I was thinking about something like this:

class ActiveDomObject {
  ...
  virtual bool hasPendingActivity() const { 
    return !window()->isDetached && hasLocalPendingActivity();
  }
  virtual bool hasLocalPendingActivity() const = 0;
}

class BatteryManager ... {
  ...
  bool hasLocalPendingActivity() const {
    return hasEventListeners();
  }
}

What I look for is that the decision whether object has pending activity stays with the object, and doesn't depend on anything external, such as V8GCController...

BatteryManager shouldn't look at whether it has listeners or not, but at whether it will send further events or not. And once the frame gets detached, it should probably stop sending events.

I agree with Marcel that this should be a decision done inside BatteryManager, not inside gc (not inside the base class).

If we change ActiveDOMObject, we should make it something like this:

bool hasPendingActivity() const { return m_hasPendingActivity; }

protected:
  void setHasPendingActivity() { m_hasPendingActivity = true; }
  void pendingActivityStopped() { m_hasPendingActivyty = false; }

}

and fix all the hand-wavy lifetime management in the implementations :/

Re #9: However, as you mentioned, the lifetime of BatteryManager should be bound to the window object, right?

IIUC, the DOM spec requires to stop almost all DOM objects when the window object gets detached. Is there any DOM object whose lifetime is not bound to window's lifetime?

In any case, I think it's much better to make that behavior the default and introduce a way to customize it if some objects are expected to outlive window.

Re #11, #12: That will work but as I mentioned above, it requires to keep track of all ActiveScriptWrappables in ContextLifecycleObserver (to call pendingActivityStopped() when the window gets detached).

If you do insist you never want to add the CreationContext API anyway, I'll go that way, although I don't think that's a clever decision in terms of performance.

In terms of implementation, one thing we can do is to migrate V8PerIsolateData::m_activeScriptWrappables to V8PerContextData. Then we can stop pending activities of a given context when it's detached.

ScriptWrappableVisitor can trace all ActiveScriptWrappables via tracing all V8PerContextData.

Then we don't need to register ActiveScriptWrappables to ContextLifecycleObservers.

Jochen, so you want to switch from the current "People will query me and in the query I decide whether I'm done" to "I know the moment when I'm done and then I switch the variable, and people querying me just always get that variable"? That sounds like a nontrivial work :)

In terms of implementation, I think the approach in #14 would solve the problem.

However, what I'm wondering is:

- We have reached the conclusion that the concept of CreationContext is mandatory (which is a relevant realm of a wrapper object).

- Currently only V8 has knowledge of the CreationContext. The code base heavily depends on the CreationContext API. Remember that we have decided to use the relevant realm by default in V8 bindings.

- However, V8 doesn't expose the CreationContext for persistent handles. So we need to come up with a work-around every time we hit the inconvenience.

This sounds strange to me. Given that it's not trivial work (or not high-priority work) to migrate the CreationContext from V8 to Blink, it would make more sense to support the API until we actually do that IMHO.

I guess there are two things: trying to contain the memory leaks right now, and then get the right lifetime implemented.

I'm fine with whatever way you propose to contain the leaks now. I think once we have traceWrapper, we should work towards fixing this the right way[tm] 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/62da61e28d18afa418de5bf2f070e8d56d766b4b

commit 62da61e28d18afa418de5bf2f070e8d56d766b4b
Author: haraken <haraken@chromium.org>
Date: Mon Oct 03 06:40:12 2016

Make hasPendingActivity return false after the associated browsing context is detached

Currently some hasPendingActivitity's are mis-implemented and keep returning true forever.
Then the entire window object can leak. As a work-around to avoid the leak, this CL adds logic
to ignore the result of hasPendingActivity when the associated window object is detached.

See  bug 650146  for more detailed context.

BUG= 650146 

Review-Url: https://codereview.chromium.org/2377953002
Cr-Commit-Position: refs/heads/master@{#422378}

[modify] https://crrev.com/62da61e28d18afa418de5bf2f070e8d56d766b4b/third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp

All done now..right?