Refactoring ActiveDOMObject

[Kentaro Hara]

Hi

TL;DR: I'm planning to make the following refactoring to ActiveDOMObject.

Currently we mix multiple functionalities on ActiveDOMObjects, which has been a cause of confusions.

- hasPendingAcitivity(): A mechanism to keep alive a V8 wrapper while DOM is doing something.

- stop(): A method called when the associated ExecutionContext is stopped.

- suspend()/resume(): A method called when the associated ExecutionContext is suspended/resumed (e.g., Devtools enters/leaves a break point).

Conceptually, hasPendingAcitivity() should be a functionality of ScriptWrappable. stop() should be a functionality of ContextLifecycleObserver (which already has contextDestroyed(), which is called almost at the same time as ActiveDOMObject::stop()).

So I'm planning to make the following changes:

1) Move hasPendingActivity() to ScriptWrappable. Remove the [ActiveDOMObject] IDL extended attribute.

2) Remove ActiveDOMObject::stop(). Classes that had been using ActiveDOMObject::stop() should instead inherit from ContextLifecycleObserver and use ContextLifecycleObserver::contextDestroyed().

3) At this point, ActiveDOMObject will have only suspend() and resume(). Rename ActiveDOMObject to SuspendableObject.

4) Document how to use them.

A tracking bug is here. If you have any question/concern, please let me know :)

--

Kentaro Hara, Tokyo, Japan

[Elliott Sprehn]

On Wed, Feb 24, 2016 at 2:02 PM, Kentaro Hara <har...@chromium.org> wrote:

Hi

TL;DR: I'm planning to make the following refactoring to ActiveDOMObject.

Currently we mix multiple functionalities on ActiveDOMObjects, which has been a cause of confusions.

- hasPendingAcitivity(): A mechanism to keep alive a V8 wrapper while DOM is doing something.

- stop(): A method called when the associated ExecutionContext is stopped.

- suspend()/resume(): A method called when the associated ExecutionContext is suspended/resumed (e.g., Devtools enters/leaves a break point).

Conceptually, hasPendingAcitivity() should be a functionality of ScriptWrappable. stop() should be a functionality of ContextLifecycleObserver (which already has contextDestroyed(), which is called almost at the same time as ActiveDOMObject::stop()).

So I'm planning to make the following changes:

1) Move hasPendingActivity() to ScriptWrappable. Remove the [ActiveDOMObject] IDL extended attribute.

Can we give this a better name then? Maybe keepWrapperAlive() ?

 

2) Remove ActiveDOMObject::stop(). Classes that had been using ActiveDOMObject::stop() should instead inherit from ContextLifecycleObserver and use ContextLifecycleObserver::contextDestroyed().

3) At this point, ActiveDOMObject will have only suspend() and resume(). Rename ActiveDOMObject to SuspendableObject.

 Almost everything I see that implements suspend/resume implements stop, does that mean all SuspendableObject's will need to be ContextLifecycleObserver's?

- E

[Kentaro Hara]

On Wed, Feb 24, 2016 at 10:27 PM, Elliott Sprehn <esp...@chromium.org> wrote:

On Wed, Feb 24, 2016 at 2:02 PM, Kentaro Hara <har...@chromium.org> wrote:

Hi

TL;DR: I'm planning to make the following refactoring to ActiveDOMObject.

Currently we mix multiple functionalities on ActiveDOMObjects, which has been a cause of confusions.

- hasPendingAcitivity(): A mechanism to keep alive a V8 wrapper while DOM is doing something.

- stop(): A method called when the associated ExecutionContext is stopped.

- suspend()/resume(): A method called when the associated ExecutionContext is suspended/resumed (e.g., Devtools enters/leaves a break point).

Conceptually, hasPendingAcitivity() should be a functionality of ScriptWrappable. stop() should be a functionality of ContextLifecycleObserver (which already has contextDestroyed(), which is called almost at the same time as ActiveDOMObject::stop()).

So I'm planning to make the following changes:

1) Move hasPendingActivity() to ScriptWrappable. Remove the [ActiveDOMObject] IDL extended attribute.

Can we give this a better name then? Maybe keepWrapperAlive() ? 

Sounds nicer :)

 

2) Remove ActiveDOMObject::stop(). Classes that had been using ActiveDOMObject::stop() should instead inherit from ContextLifecycleObserver and use ContextLifecycleObserver::contextDestroyed().

3) At this point, ActiveDOMObject will have only suspend() and resume(). Rename ActiveDOMObject to SuspendableObject.

 Almost everything I see that implements suspend/resume implements stop, does that mean all SuspendableObject's will need to be ContextLifecycleObserver's? 

I'm planning to make SuspendableObject inherit from ContextLifecycleObserver. So SuspendableObject can just use ContextLifecycleObserver::contextDestroyed().

[Dominic Cooney]

(Sorry for top posting.)

This in general sounds like a good idea but I would like to know more specifics.

+1 to giving hasPendingActivity a better name.

What assertions can you build into this? I guess we want things like: No creating these things when the context is stopped, or if you allow that, then hasPendingActivity must be false; after stop (or contextDestroyed) hasPendingActivity must be false; etc.

Was there any difference between the timing of stop and contextDestroyed?

How does ActiveDOMObject::didMoveToNewExecutionContext work in this world?

[Kentaro Hara]

On Wed, Feb 24, 2016 at 11:36 PM, Dominic Cooney <domi...@chromium.org> wrote:

(Sorry for top posting.)

This in general sounds like a good idea but I would like to know more specifics.

+1 to giving hasPendingActivity a better name. 

 

What assertions can you build into this? I guess we want things like: No creating these things when the context is stopped, or if you allow that, then hasPendingActivity must be false; after stop (or contextDestroyed) hasPendingActivity must be false; etc.

Yeah, this is an interesting question. Currently we're asking each customer of hasPendingActivity to make sure that hasPendingActivity returns false in a finite time period (otherwise, it will end up with leaking the entire window which the DOM object belongs to). Ideally I want to change V8GCController so that it simply ignores hasPendingActivitiy of a DOM object that belongs to an already stopped ExecutionContext (i.e., forcibly treat that the hasPendingActivity returns false), but I need to study if it doesn't break any existing lifetime contracts.

 

Was there any difference between the timing of stop and contextDestroyed?

No. They are already called in a row.

 

How does ActiveDOMObject::didMoveToNewExecutionContext work in this world?

It's used only by HTMLMediaElement. So I think we can just inline the logic into HTMLMediaElement::didMoveToNewDocument.

 

On Wed, Feb 24, 2016 at 2:38 PM, Kentaro Hara <har...@chromium.org> wrote:

On Wed, Feb 24, 2016 at 10:27 PM, Elliott Sprehn <esp...@chromium.org> wrote:

On Wed, Feb 24, 2016 at 2:02 PM, Kentaro Hara <har...@chromium.org> wrote:

Hi

TL;DR: I'm planning to make the following refactoring to ActiveDOMObject.

Currently we mix multiple functionalities on ActiveDOMObjects, which has been a cause of confusions.

- hasPendingAcitivity(): A mechanism to keep alive a V8 wrapper while DOM is doing something.

- stop(): A method called when the associated ExecutionContext is stopped.

- suspend()/resume(): A method called when the associated ExecutionContext is suspended/resumed (e.g., Devtools enters/leaves a break point).

Conceptually, hasPendingAcitivity() should be a functionality of ScriptWrappable. stop() should be a functionality of ContextLifecycleObserver (which already has contextDestroyed(), which is called almost at the same time as ActiveDOMObject::stop()).

So I'm planning to make the following changes:

1) Move hasPendingActivity() to ScriptWrappable. Remove the [ActiveDOMObject] IDL extended attribute.

Can we give this a better name then? Maybe keepWrapperAlive() ? 

Sounds nicer :)

 

2) Remove ActiveDOMObject::stop(). Classes that had been using ActiveDOMObject::stop() should instead inherit from ContextLifecycleObserver and use ContextLifecycleObserver::contextDestroyed().

3) At this point, ActiveDOMObject will have only suspend() and resume(). Rename ActiveDOMObject to SuspendableObject.

 Almost everything I see that implements suspend/resume implements stop, does that mean all SuspendableObject's will need to be ContextLifecycleObserver's? 

I'm planning to make SuspendableObject inherit from ContextLifecycleObserver. So SuspendableObject can just use ContextLifecycleObserver::contextDestroyed().

--

Kentaro Hara, Tokyo, Japan

[Jochen Eisinger]

On Thu, Feb 25, 2016 at 10:48 AM Kentaro Hara <har...@chromium.org> wrote:

On Wed, Feb 24, 2016 at 11:36 PM, Dominic Cooney <domi...@chromium.org> wrote:

(Sorry for top posting.)

This in general sounds like a good idea but I would like to know more specifics.

+1 to giving hasPendingActivity a better name. 

 

What assertions can you build into this? I guess we want things like: No creating these things when the context is stopped, or if you allow that, then hasPendingActivity must be false; after stop (or contextDestroyed) hasPendingActivity must be false; etc.

Yeah, this is an interesting question. Currently we're asking each customer of hasPendingActivity to make sure that hasPendingActivity returns false in a finite time period (otherwise, it will end up with leaking the entire window which the DOM object belongs to). Ideally I want to change V8GCController so that it simply ignores hasPendingActivitiy of a DOM object that belongs to an already stopped ExecutionContext (i.e., forcibly treat that the hasPendingActivity returns false), but I need to study if it doesn't break any existing lifetime contracts.

that would be easier if we also stopped running scripts in those contexts.. 

[Kentaro Hara]

On Thu, Feb 25, 2016 at 10:26 AM, Jochen Eisinger <joc...@chromium.org> wrote:

On Thu, Feb 25, 2016 at 10:48 AM Kentaro Hara <har...@chromium.org> wrote:

On Wed, Feb 24, 2016 at 11:36 PM, Dominic Cooney <domi...@chromium.org> wrote:

(Sorry for top posting.)

This in general sounds like a good idea but I would like to know more specifics.

+1 to giving hasPendingActivity a better name. 

 

What assertions can you build into this? I guess we want things like: No creating these things when the context is stopped, or if you allow that, then hasPendingActivity must be false; after stop (or contextDestroyed) hasPendingActivity must be false; etc.

Yeah, this is an interesting question. Currently we're asking each customer of hasPendingActivity to make sure that hasPendingActivity returns false in a finite time period (otherwise, it will end up with leaking the entire window which the DOM object belongs to). Ideally I want to change V8GCController so that it simply ignores hasPendingActivitiy of a DOM object that belongs to an already stopped ExecutionContext (i.e., forcibly treat that the hasPendingActivity returns false), but I need to study if it doesn't break any existing lifetime contracts.

that would be easier if we also stopped running scripts in those contexts.. 

There are a couple of subtleties around it. For example, there is a way to get an ActiveDOMObject that belongs to an already detached ExecutionContext.

// iframe.html

<video></video>

// main.html

<iframe src=iframe.html></iframe>

<script>

iframe = document.querySelector("iframe");

iframeDocument = iframe.contentDocument;

document.body.removeChild(iframe);  // At this point, the ExecutionContext of the iframe stops.

iframeDocument.querySelector("video");  // You can still get an ActiveDOMObject that belongs to the ExecutionContext.

</script>

So I need a bit more study before making that change.

[Philip Jägenstedt]

On Thu, Feb 25, 2016 at 5:50 PM, Kentaro Hara <har...@chromium.org> wrote:

On Thu, Feb 25, 2016 at 10:26 AM, Jochen Eisinger <joc...@chromium.org> wrote:

On Thu, Feb 25, 2016 at 10:48 AM Kentaro Hara <har...@chromium.org> wrote:

On Wed, Feb 24, 2016 at 11:36 PM, Dominic Cooney <domi...@chromium.org> wrote:

(Sorry for top posting.)

This in general sounds like a good idea but I would like to know more specifics.

+1 to giving hasPendingActivity a better name. 

 

What assertions can you build into this? I guess we want things like: No creating these things when the context is stopped, or if you allow that, then hasPendingActivity must be false; after stop (or contextDestroyed) hasPendingActivity must be false; etc.

Yeah, this is an interesting question. Currently we're asking each customer of hasPendingActivity to make sure that hasPendingActivity returns false in a finite time period (otherwise, it will end up with leaking the entire window which the DOM object belongs to). Ideally I want to change V8GCController so that it simply ignores hasPendingActivitiy of a DOM object that belongs to an already stopped ExecutionContext (i.e., forcibly treat that the hasPendingActivity returns false), but I need to study if it doesn't break any existing lifetime contracts.

that would be easier if we also stopped running scripts in those contexts.. 

There are a couple of subtleties around it. For example, there is a way to get an ActiveDOMObject that belongs to an already detached ExecutionContext.

// iframe.html

<video></video>

// main.html

<iframe src=iframe.html></iframe>

<script>

iframe = document.querySelector("iframe");

iframeDocument = iframe.contentDocument;

document.body.removeChild(iframe);  // At this point, the ExecutionContext of the iframe stops.

iframeDocument.querySelector("video");  // You can still get an ActiveDOMObject that belongs to the ExecutionContext.

</script>

So I need a bit more study before making that change.

It would be great to get this situation in order. If HTMLMediaElement specifically is giving you trouble, don't assume that it's working correctly and needs stay unchanged, because moving media elements between documents is actually entirely broken. This can work to your advantage, because a media element recreates the backend when moving to a new document, it's hard to make it much worse.

Continuing the example above, one could move that video into an active document. Per spec there's nothing to explain why the state of the media element has changed at all, or indeed why it stopped playing. It could be that we have to change the spec to allow for this, since just continuing to play is not an option.

Philip 

[Kentaro Hara]

Thanks Phillip!

Unfortunately this is a problem of all DOM objects that inherit from hasPendingActivity, not limited to HTMLMediaElement. That said, as you described for HTMLMediaElement, in common cases things are already broken (you cannot do any meaningful thing) after the ExecutionContext stops. So it wouldn't matter in practice whether we stop keeping alive the wrapper or not, I think.