Lifetime of event listeners

52 views
Skip to first unread message

Kentaro Hara

unread,
Sep 21, 2016, 9:09:45 AM9/21/16
to platform-architecture-dev
Hi

I noticed that the current lifetime management of event listeners is very complicated, broken and leaking. I have some ideas to make it simpler, more correct and not leaky, but before running into the details, let me ask one question:

Per the spec, how long should an event listener be kept alive?

The current implementation of the lifetime management of event listeners looks pretty arbitrary. Here are a couple of examples:

- XHR's event listener is kept alive until XHR finishes dispatching all events.

- Node's event listener (e.g., onclick listener) is kept alive until all wrappers in the DOM tree which the Node belongs to are collected by V8's GC.

- BatteryManager's event listener is kept alive forever (unless developers explicitly remove the listener). This is because battery status events may happen in the DOM side anytime (=there is no clear timing when we can collect the event listener). This implementation is clearly wrong because it leaks the event listener, which keeps a strong reference to the entire window object. There are a couple of other DOM objects that have the same problem.

I couldn't find any explicit description in the spec about how the lifetime of event listeners should be maintained, but my best guess is as follows:

- An event listener should be kept alive until its associated window object is detached (=until the browsing context becomes inactive).

- However, a user agent is allowed to collect the event listener earlier if it's not observable to the user-facing script.

For example, XHR and Node collect event listeners before their associated window object is detached, but that is fine because the premature collection is not observable to the user-facing script. On the other hand, BatteryManager should release the event listener when its associated window object is detached. Otherwise, the event listener leaks (actually it is leaking).

Am I understanding things correctly?


--
Kentaro Hara, Tokyo, Japan

Yuki Shiino

unread,
Sep 21, 2016, 9:37:26 AM9/21/16
to Kentaro Hara, platform-architecture-dev
I think event listeners are not different from any other objects in terms of lifetime.  If nothing has a reference to a certain function object, then that function object is discardable by GC.  Event listeners are usually associated with DOM objects (e.g. addEventListener), then those event listenres should be kept alive as long as the associated DOM objects are reachable, I think.

Most DOM objects are in a DOM tree of a document, so DOM objects are alive as long as the document is alive.  I don't think this situation is a leak.

Discarding a document and browsing context is spec'ed at HTML 7.3.4.
https://html.spec.whatwg.org/multipage/browsers.html#garbage-collection-and-browsing-contexts
It's not when a window object gets detached, but when nothing has a strong reference to a document or window, then we can discard it.  When discarding a document or browsing context, we eventually discard DOM objects and associated event listeners, too.

Cheers,
Yuki Shiino


--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jzQEDUfcCsREBuMQiG9QT6zbB8yy8VWFrzvF98kh-deZg%40mail.gmail.com.

Kentaro Hara

unread,
Sep 21, 2016, 11:07:53 AM9/21/16
to Yuki Shiino, Jochen Eisinger, Marcel Hlopko, hpayer, platform-architecture-dev
I think event listeners are not different from any other objects in terms of lifetime.  If nothing has a reference to a certain function object, then that function object is discardable by GC.  Event listeners are usually associated with DOM objects (e.g. addEventListener), then those event listenres should be kept alive as long as the associated DOM objects are reachable, I think.

Do you have a link to the spec that defines the behavior?

You're probably correct, but then the next question is how to implement the lifetime relationship in our current architecture.

For example, if we want to keep the event listener alive as long as the C++ BatteryManager is alive, we need to create the following object graph:


​
​​
The BatteryManager => v8::Function reference can cause the following cycle:

  BatteryManager ==> v8::Function ==> window ==(any arbitrary references)==> BatteryManager

Remember that traceWrapper is not helpful to break the cycle. traceWrapper is (just) a mechanism to represent reachability from one wrapper to another wrapper. However, what we need here is to represent reachability from a DOM object (=BatteryManager) to a V8 object (=v8::Function) in a way that doesn't create a cycle.

One practical solution to this problem would be to explicitly remove the BatteryManager => v8::Function reference when the window object gets detached. This is why I was wondering if the event listener's lifetime is bound to the lifetime of its associated window object.


On Wed, Sep 21, 2016 at 10:36 PM, Yuki Shiino <yukis...@chromium.org> wrote:
I think event listeners are not different from any other objects in terms of lifetime.  If nothing has a reference to a certain function object, then that function object is discardable by GC.  Event listeners are usually associated with DOM objects (e.g. addEventListener), then those event listenres should be kept alive as long as the associated DOM objects are reachable, I think.

Most DOM objects are in a DOM tree of a document, so DOM objects are alive as long as the document is alive.  I don't think this situation is a leak.

The event listener of BatteryManager is actually leaking for the reason I described above.

You may wonder why LayoutTests/battery-status/promise-with-eventlisteners.html is not leaking. That's just because the test is explicitly calling removeEventListener before the leak detector runs... Sigh.

As far as I grep the code base, Animation, MeidaQueryList, Sensor, BroadcastChannel, ImageCapture, RemotePlayback, RemotePlaybackAvailability, NetworkInformation etc have the same leak problem.


 
Discarding a document and browsing context is spec'ed at HTML 7.3.4.
https://html.spec.whatwg.org/multipage/browsers.html#garbage-collection-and-browsing-contexts
It's not when a window object gets detached, but when nothing has a strong reference to a document or window, then we can discard it.  When discarding a document or browsing context, we eventually discard DOM objects and associated event listeners, too.

Cheers,
Yuki Shiino

2016-09-21 22:09 GMT+09:00 Kentaro Hara <har...@chromium.org>:
Hi

I noticed that the current lifetime management of event listeners is very complicated, broken and leaking. I have some ideas to make it simpler, more correct and not leaky, but before running into the details, let me ask one question:

Per the spec, how long should an event listener be kept alive?

The current implementation of the lifetime management of event listeners looks pretty arbitrary. Here are a couple of examples:

- XHR's event listener is kept alive until XHR finishes dispatching all events.

- Node's event listener (e.g., onclick listener) is kept alive until all wrappers in the DOM tree which the Node belongs to are collected by V8's GC.

- BatteryManager's event listener is kept alive forever (unless developers explicitly remove the listener). This is because battery status events may happen in the DOM side anytime (=there is no clear timing when we can collect the event listener). This implementation is clearly wrong because it leaks the event listener, which keeps a strong reference to the entire window object. There are a couple of other DOM objects that have the same problem.

I couldn't find any explicit description in the spec about how the lifetime of event listeners should be maintained, but my best guess is as follows:

- An event listener should be kept alive until its associated window object is detached (=until the browsing context becomes inactive).

- However, a user agent is allowed to collect the event listener earlier if it's not observable to the user-facing script.

For example, XHR and Node collect event listeners before their associated window object is detached, but that is fine because the premature collection is not observable to the user-facing script. On the other hand, BatteryManager should release the event listener when its associated window object is detached. Otherwise, the event listener leaks (actually it is leaking).

Am I understanding things correctly?


--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.

Jochen Eisinger

unread,
Sep 21, 2016, 11:44:15 AM9/21/16
to Kentaro Hara, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev

Why would wrapper tracing not solve this problem?


To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jx78hVVbR_MwNUWTmUSg80NqOuL3PS_wUNaJEHvw-u61g%40mail.gmail.com.

Kentaro Hara

unread,
Sep 22, 2016, 2:11:19 AM9/22/16
to Jochen Eisinger, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
Why would wrapper tracing not solve this problem?

This is an interesting point.

traceWrapper is a mechanism to represent reachability from one V8 wrapper to another V8 wrapper, like this:

  V8 wrapper => DOM object => DOM object => ... => V8 wrapper

On the other hand, what we need now is a mechanism to represent reachability from a DOM object to a V8 wrapper:

  DOM object (BatteryManager) => V8 wrapper (v8::Function)

If we use a strong v8::Persistent to represent the reference, it will cause leaks, as I described in the last email.

One way to solve the problem is to use the strong v8::Persistent but explicitly break the reference when the window object gets detached. (Note: We can realize the same thing more simply by making BatteryManager::hasPendingActivity return false when the associated window object is detached.) That's why I'm wondering if the event listener's lifetime is bound to the lifetime of the window object.


To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

Jochen Eisinger

unread,
Sep 22, 2016, 4:06:22 AM9/22/16
to Kentaro Hara, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
Once we have traceWrappers enabled, the traced persistents aren't roots anymore. So the actual cycle is

context -> dom window -> battery manager -> v8 function -> context

which is fully tracable and shouldn't keep anything alive.

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jzDEe-H%2BUOLTsGSXb-J5bQUGQPjfNVOrwjsoMvNS7e2VQ%40mail.gmail.com.

Domenic Denicola

unread,
Sep 22, 2016, 7:39:47 AM9/22/16
to Yuki Shiino, Kentaro Hara, platform-architecture-dev


On Wed, Sep 21, 2016, 14:37 Yuki Shiino <yukis...@chromium.org> wrote:
I think event listeners are not different from any other objects in terms of lifetime.  If nothing has a reference to a certain function object, then that function object is discardable by GC.  Event listeners are usually associated with DOM objects (e.g. addEventListener), then those event listenres should be kept alive as long as the associated DOM objects are reachable, I think.

Most DOM objects are in a DOM tree of a document, so DOM objects are alive as long as the document is alive.  I don't think this situation is a leak.

Discarding a document and browsing context is spec'ed at HTML 7.3.4.
https://html.spec.whatwg.org/multipage/browsers.html#garbage-collection-and-browsing-contexts
It's not when a window object gets detached, but when nothing has a strong reference to a document or window, then we can discard it.  When discarding a document or browsing context, we eventually discard DOM objects and associated event listeners, too.

This sounds correct to me. There is no manual cleanup when you navigate or disconnect an iframe from the tree or similar. Normal JS semantics are followed. As long as JS has a reference to the event target, the listeners need to be retained.

Note that event listeners are stored on the Window, not WindowProxy, and JS can never get a direct reference to the window. So in the case of Window event listeners can be thrown away during navigation, I think. But not for other objects like battery status.

Cheers,
Yuki Shiino


2016-09-21 22:09 GMT+09:00 Kentaro Hara <har...@chromium.org>:
Hi

I noticed that the current lifetime management of event listeners is very complicated, broken and leaking. I have some ideas to make it simpler, more correct and not leaky, but before running into the details, let me ask one question:

Per the spec, how long should an event listener be kept alive?

The current implementation of the lifetime management of event listeners looks pretty arbitrary. Here are a couple of examples:

- XHR's event listener is kept alive until XHR finishes dispatching all events.

- Node's event listener (e.g., onclick listener) is kept alive until all wrappers in the DOM tree which the Node belongs to are collected by V8's GC.

- BatteryManager's event listener is kept alive forever (unless developers explicitly remove the listener). This is because battery status events may happen in the DOM side anytime (=there is no clear timing when we can collect the event listener). This implementation is clearly wrong because it leaks the event listener, which keeps a strong reference to the entire window object. There are a couple of other DOM objects that have the same problem.

I couldn't find any explicit description in the spec about how the lifetime of event listeners should be maintained, but my best guess is as follows:

- An event listener should be kept alive until its associated window object is detached (=until the browsing context becomes inactive).

- However, a user agent is allowed to collect the event listener earlier if it's not observable to the user-facing script.

For example, XHR and Node collect event listeners before their associated window object is detached, but that is fine because the premature collection is not observable to the user-facing script. On the other hand, BatteryManager should release the event listener when its associated window object is detached. Otherwise, the event listener leaks (actually it is leaking).

Am I understanding things correctly?


--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jzQEDUfcCsREBuMQiG9QT6zbB8yy8VWFrzvF98kh-deZg%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAN0uC_SigF7Gz9sPGY6Fxuz6hT93mSm49WbGDCjnzqyG7Y3Y4A%40mail.gmail.com.

Kentaro Hara

unread,
Sep 22, 2016, 9:40:43 AM9/22/16
to Jochen Eisinger, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
Once we have traceWrappers enabled, the traced persistents aren't roots anymore. So the actual cycle is 
 
context -> dom window -> battery manager -> v8 function -> context 
 
which is fully tracable and shouldn't keep anything alive.

That's exactly what I'm asking here -- whether it's okay to bound event listener's lifetime to the window's lifetime. If it's okay, we can implement the lifetime management using traceWrapper (as you described) or hasPendingActivity (as I described). The problem is that per the spec it's not clear if it's okay (as Domenic described).


To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

Jochen Eisinger

unread,
Sep 22, 2016, 9:42:41 AM9/22/16
to Kentaro Hara, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
I'm not suggesting to bind it to the lifetime of window, but (in this example) to the lifetime of the battery manager.

If the battery manager dies before the window does, the listener should go away.

I think that's in line with what Domenic said about the regular lifetime of JS objects.

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jxLQXGCgtC12DBZVUg%3DD8eOHzsL0vSJ2wa5PW1thxRk3g%40mail.gmail.com.

Kentaro Hara

unread,
Sep 22, 2016, 9:51:45 AM9/22/16
to Jochen Eisinger, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
I'm not suggesting to bind it to the lifetime of window, but (in this example) to the lifetime of the battery manager 
 
If the battery manager dies before the window does, the listener should go away. 
 
I think that's in line with what Domenic said about the regular lifetime of JS objects.

Ah, thanks. I got your point :)

That being said, that behavior would be problematic as well... Imagine the following scenario:

function func() {
  window.batteryManager.addEventListener('levelchange', function () { ... });
  gc();
}
func();

gc() collects BatteryManager's wrapper. If we collect the event listener at that point, the battery status event won't be fired after func() has been called. However, the current Blink's implementation is expecting that the battery status event keeps getting fired while the C++ BatteryManager object is alive. (Sensor, BroadcastChannel, ImageCapture, RemotePlayback etc have the same problem.)

Another example is XHR. XHR's event is expected to be fired even after XHR's wrapper is collected in the JS side. XHR's event listener is expected to be alive until all XHR events have been dispatched.



To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

Jochen Eisinger

unread,
Sep 22, 2016, 9:54:37 AM9/22/16
to Kentaro Hara, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
yeah, but that's an artifact of binding the lifetime to the wrapper, instead of using the underlying C++ object which survives the gc.

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

Kentaro Hara

unread,
Sep 22, 2016, 10:02:18 AM9/22/16
to Jochen Eisinger, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
yeah, but that's an artifact of binding the lifetime to the wrapper, instead of using the underlying C++ object which survives the gc.

Then what's your suggestion to realize the expected lifetime?

Assuming that there's no way to represent a strong reachability from the C++ BatteryManager object to the event listener in a way that doesn't leak things, I guess we'll need to bound the event listener's lifetime to the window object or something (using traceWrapper or hasPendingActivity).


To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CALjhuidJ8Smxm6x-_sRzysE3Fp2EaC%2By3hPw3pZkSUmU2traLA%40mail.gmail.com.

Jochen Eisinger

unread,
Sep 22, 2016, 10:05:53 AM9/22/16
to Kentaro Hara, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
we "just" need to make sure that tracewrapper sees the following cycle:

context (js) -> window (C++) -> battery manager (C++) -> v8 function (js) -> context (js)

if the connection from window (C++) -> battery manager (C++) goes away, v8 function should get collected.

During a normal navigation, the context -> window link is severed which should make oilpan collect the battery manager and v8 collect the function (and context)

Kentaro Hara

unread,
Sep 22, 2016, 10:31:40 AM9/22/16
to Jochen Eisinger, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
On Thu, Sep 22, 2016 at 11:05 PM, Jochen Eisinger <joc...@chromium.org> wrote:
we "just" need to make sure that tracewrapper sees the following cycle:

context (js) -> window (C++) -> battery manager (C++) -> v8 function (js) -> context (js)

if the connection from window (C++) -> battery manager (C++) goes away, v8 function should get collected.

During a normal navigation, the context -> window link is severed which should make oilpan collect the battery manager and v8 collect the function (and context)

Thanks, this makes a lot of more sense.

I begin to think that the real problem is that there's no reference from window (JS) to BatteryManager (JS).

- If we don't have window (JS) => BatteryManager (JS), the wrapper returned by window.batteryManager may change over time. The expando on the wrapper may be lost. This is problematic (before worrying about the event listener). Hence, we need to have window (JS) => BatteryManager (JS).

- If we have window (JS) => BatteryManager (JS), all the problem we're discussing on this thread will be gone, because the event listener will be kept alive by window (JS) => BatteryManager (JS) => event listener (JS).

Of course, we can easily represent window (JS) => BatteryManager (JS) and BatteryManager (JS) => event listener (JS) using traceWrapper.

(You're probably essentially saying the same thing -- thanks, I think I now understand how to make it work correctly in the traceWrapper world :-)


 

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.

To post to this group, send email to platform-architecture-dev@chromium.org.

Jochen Eisinger

unread,
Sep 22, 2016, 10:44:11 AM9/22/16
to Kentaro Hara, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
On Thu, Sep 22, 2016 at 3:31 PM Kentaro Hara <har...@chromium.org> wrote:
On Thu, Sep 22, 2016 at 11:05 PM, Jochen Eisinger <joc...@chromium.org> wrote:
we "just" need to make sure that tracewrapper sees the following cycle:

context (js) -> window (C++) -> battery manager (C++) -> v8 function (js) -> context (js)

if the connection from window (C++) -> battery manager (C++) goes away, v8 function should get collected.

During a normal navigation, the context -> window link is severed which should make oilpan collect the battery manager and v8 collect the function (and context)

Thanks, this makes a lot of more sense.

I begin to think that the real problem is that there's no reference from window (JS) to BatteryManager (JS).

I don't think that's the problem - we should not create such a reference unless the spec explicitly asks for such a reference.
 

- If we don't have window (JS) => BatteryManager (JS), the wrapper returned by window.batteryManager may change over time. The expando on the wrapper may be lost. This is problematic (before worrying about the event listener). Hence, we need to have window (JS) => BatteryManager (JS).

- If we have window (JS) => BatteryManager (JS), all the problem we're discussing on this thread will be gone, because the event listener will be kept alive by window (JS) => BatteryManager (JS) => event listener (JS).

this is, however, the wrong path to keep the event listener alive. It should be kept alive via the C++ BatteryManager, and this is what traceWrapper does.
 
battery_manager_proposed.png
​
​​
The BatteryManager => v8::Function reference can cause the following cycle:

  BatteryManager ==> v8::Function ==> window ==(any arbitrary references)==> BatteryManager

Remember that traceWrapper is not helpful to break the cycle. traceWrapper is (just) a mechanism to represent reachability from one wrapper to another wrapper. However, what we need here is to represent reachability from a DOM object (=BatteryManager) to a V8 object (=v8::Function) in a way that doesn't create a cycle.

One practical solution to this problem would be to explicitly remove the BatteryManager => v8::Function reference when the window object gets detached. This is why I was wondering if the event listener's lifetime is bound to the lifetime of its associated window object.


Kentaro Hara

unread,
Sep 22, 2016, 10:53:17 AM9/22/16
to Jochen Eisinger, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
On Thu, Sep 22, 2016 at 11:43 PM, Jochen Eisinger <joc...@chromium.org> wrote:


On Thu, Sep 22, 2016 at 3:31 PM Kentaro Hara <har...@chromium.org> wrote:
On Thu, Sep 22, 2016 at 11:05 PM, Jochen Eisinger <joc...@chromium.org> wrote:
we "just" need to make sure that tracewrapper sees the following cycle:

context (js) -> window (C++) -> battery manager (C++) -> v8 function (js) -> context (js)

if the connection from window (C++) -> battery manager (C++) goes away, v8 function should get collected.

During a normal navigation, the context -> window link is severed which should make oilpan collect the battery manager and v8 collect the function (and context)

Thanks, this makes a lot of more sense.

I begin to think that the real problem is that there's no reference from window (JS) to BatteryManager (JS).

I don't think that's the problem - we should not create such a reference unless the spec explicitly asks for such a reference.

In the spec, a wrapper and a C++ object are the same thing (there's no distinction between a wrapper and a C++ object). Also window.batteryManager is not speced to create a new object. It means that the spec requires to add the reference, right?

 

- If we don't have window (JS) => BatteryManager (JS), the wrapper returned by window.batteryManager may change over time. The expando on the wrapper may be lost. This is problematic (before worrying about the event listener). Hence, we need to have window (JS) => BatteryManager (JS).

- If we have window (JS) => BatteryManager (JS), all the problem we're discussing on this thread will be gone, because the event listener will be kept alive by window (JS) => BatteryManager (JS) => event listener (JS).

this is, however, the wrong path to keep the event listener alive. It should be kept alive via the C++ BatteryManager, and this is what traceWrapper does.

Then I begin to wonder why you treat the window (C++) => BatteryManager (C++) reference specially. In general, window (C++) is not the only object that has a strong reference to BatteryManager (C++). (Note: In the particular case of BatteryManager, window (C++) may be the only object though.)

My understanding is that you treat the window (C++) => BatteryManager (C++) reference specially because we have window.batteryManager.

Jochen Eisinger

unread,
Sep 22, 2016, 11:19:22 AM9/22/16
to Kentaro Hara, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
On Thu, Sep 22, 2016 at 3:53 PM Kentaro Hara <har...@chromium.org> wrote:
On Thu, Sep 22, 2016 at 11:43 PM, Jochen Eisinger <joc...@chromium.org> wrote:


On Thu, Sep 22, 2016 at 3:31 PM Kentaro Hara <har...@chromium.org> wrote:
On Thu, Sep 22, 2016 at 11:05 PM, Jochen Eisinger <joc...@chromium.org> wrote:
we "just" need to make sure that tracewrapper sees the following cycle:

context (js) -> window (C++) -> battery manager (C++) -> v8 function (js) -> context (js)

if the connection from window (C++) -> battery manager (C++) goes away, v8 function should get collected.

During a normal navigation, the context -> window link is severed which should make oilpan collect the battery manager and v8 collect the function (and context)

Thanks, this makes a lot of more sense.

I begin to think that the real problem is that there's no reference from window (JS) to BatteryManager (JS).

I don't think that's the problem - we should not create such a reference unless the spec explicitly asks for such a reference.

In the spec, a wrapper and a C++ object are the same thing (there's no distinction between a wrapper and a C++ object). Also window.batteryManager is not speced to create a new object. It means that the spec requires to add the reference, right?

https://heycam.github.io/webidl/#SameObject implies that only attributes annotated with [SameObject] are supposed to return the same object on repeated invocations.

Kentaro Hara

unread,
Sep 22, 2016, 11:55:18 AM9/22/16
to Jochen Eisinger, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
I guess we are essentially saying the same thing.

The key point is (just) that we should have the following two strong references:

- Window => BatteryManager (because we have window.batteryManager)
- BatteryManager => event listener

It doesn't matter whether the Window/BatteryManager is a C++ thing or a JS thing, because there's no distinction between them in the spec world.

========================

Details:

For example, if you make traceWrapper see the following graph:

  window (JS) => window (C++) => BatteryManager (C++) => event listener (JS)

then you'll also keep alive BatteryManager (JS) (because traceWrapper(BatteryManager (C++)) traces BatteryManager (JS)):

Conversely, if I make traceWrapper see the following graph:

  window (JS) => BatteryManager (JS) => event listener (JS)

then I'll also keep alive window (C++) (because window (JS) has a strong reference to window (C++)) and BatteryManager (C++) (because BatteryManager (JS) has a strong reference to BatteryManager (C++))

In other words, the objects kept alive in your approach is the same as the ones kept alive in my approach. The difference is just how to implement the speced two strong references using traceWrapper.

========================

Am I understanding correctly, or still missing your point?



--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

Jochen Eisinger

unread,
Sep 22, 2016, 12:11:56 PM9/22/16
to Kentaro Hara, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
yeah, from point of view what C++ objects will survive, it's equivalent.

I think the important difference is whether we force the wrapper creation just to be able to present the graph, and I think we no longer have to do that with traceWrapper

Yutaka Hirano

unread,
Sep 22, 2016, 10:17:19 PM9/22/16
to Kentaro Hara, Jochen Eisinger, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
I'm a bit confused: According https://www.w3.org/TR/battery-status/ there is no window.batteryManager and a Navigator has getBattery() function instead. https://www.w3.org/TR/battery-status/#the-navigator-interface says the promise returned by getBattery() should be kept in the Navigator object. And the implementation looks compliant with the spec. Am I seeing a wrong spec / implementation?

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.



--
Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jwzCUX9aza5Xj79d8sD5-3Z_Nd1LEBK-e1qYwoWeGfFtA%40mail.gmail.com.

Kentaro Hara

unread,
Sep 23, 2016, 12:50:17 AM9/23/16
to Yutaka Hirano, Jochen Eisinger, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
You're right. window.batterManager was a wrong example.

We should make traceWrapper see the following cycle:

  Navigator => BatteryManager's promise => BatteryManager => Event Listener





To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jwzCUX9aza5Xj79d8sD5-3Z_Nd1LEBK-e1qYwoWeGfFtA%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABihn6EpQTqLd4s_2agySVrfGCSqZ5VHkZjUThr8WSyoG9Ekow%40mail.gmail.com.

Elliott Sprehn

unread,
Sep 23, 2016, 9:25:31 PM9/23/16
to Jochen Eisinger, Domenic Denicola, Kentaro Hara, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
On Thu, Sep 22, 2016 at 4:19 PM, Jochen Eisinger <joc...@chromium.org> wrote:
On Thu, Sep 22, 2016 at 3:53 PM Kentaro Hara <har...@chromium.org> wrote:
On Thu, Sep 22, 2016 at 11:43 PM, Jochen Eisinger <joc...@chromium.org> wrote:
On Thu, Sep 22, 2016 at 3:31 PM Kentaro Hara <har...@chromium.org> wrote:
...
I begin to think that the real problem is that there's no reference from window (JS) to BatteryManager (JS).

I don't think that's the problem - we should not create such a reference unless the spec explicitly asks for such a reference.
In the spec, a wrapper and a C++ object are the same thing (there's no distinction between a wrapper and a C++ object). Also window.batteryManager is not speced to create a new object. It means that the spec requires to add the reference, right?

https://heycam.github.io/webidl/#SameObject implies that only attributes annotated with [SameObject] are supposed to return the same object on repeated invocations.

That this is pretty confusing but I don't think that's true, for example Node.idl's firstChild is not [SameObject], but should return the same object on repeated invocations unless an algorithm specs that we should swap the value.

I don't really understand the reason for [SameObject], it seems the specs maybe actually want [NewObject] to clarify the cases when you should always vend a new object? Nearly everything wants [SameObject], and getting a new object feels like the exceptional case.

See also usage like:

  [SameObject] readonly attribute Node target;
  [SameObject] readonly attribute NodeList addedNodes;
  [SameObject] readonly attribute NodeList removedNodes;

which I guess is because that algorithm never swaps the value? I'm not sure what SameObject vs not on a Node even means.

and usage like:

  readonly attribute Navigator navigator; 
  readonly attribute ApplicationCache applicationCache;

but we're definitely not returning a new navigator object every time. :P

The specs seem to be saying:
- You *might* need to return the same object if there's no annotation.
- You *must* return the same object if [SameObject] is there.

- E

Jochen Eisinger

unread,
Sep 24, 2016, 5:03:54 AM9/24/16
to Elliott Sprehn, Domenic Denicola, Kentaro Hara, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev

Maybe SameObject only applies to things that otherwise might reasonably die, like the NodeList could as well be an temporary object and just the nodes in the list staying the same?

But in either case, I don't think that this implies that the wrapper always has to exist - just that once it does, out should stay the same


--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAO9Q3iKUfzdnc0ALnFuTaddm2oEqHVqqmTiQMoki%2B0dDrDeWdg%40mail.gmail.com.

Domenic Denicola

unread,
Sep 24, 2016, 5:52:22 AM9/24/16
to Jochen Eisinger, Elliott Sprehn, Domenic Denicola, Kentaro Hara, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
[SameObject] is basically documentation, in that it is redundant with normative spec text elsewhere. I think Gecko's code generator uses it; I'm not sure if ours does.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CALjhuidR6Seh4efFVM0Q2KsNpXjiuKXEPqM3C-E98ieUwiGEKg%40mail.gmail.com.

Yuki Shiino

unread,
Sep 26, 2016, 2:55:07 AM9/26/16
to Domenic Denicola, Jochen Eisinger, Elliott Sprehn, Domenic Denicola, Kentaro Hara, Yuki Shiino, Marcel Hlopko, hpayer, platform-architecture-dev
Just FYI, in our codebase, [SameObject] adds a test in debug build to confirm the returned object is the same.  In release build, [SameObject] does nothing.

We have our own [SaveSameObject] in order for the binding layer to cache the returned object so that we always return the same object.  The reason why don't do this for all [SameObject] is a performance.  [SaveSameObject] is a bit slow.  If it's not [SaveSameObject], then it's a duty of a DOM object.

Cheers,
Yuki Shiino


To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAM0wra_V0kOX8x9P-7ZZ%2BrG0KSRCckGp2-na%2BgVs33xHxq0x_Q%40mail.gmail.com.

Reply all
Reply to author
Forward
0 new messages