Bug 109644 – MediaKeySession should not be GCd while firing event listeners or with custom properties.

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Jer Noble]

MediaKeySession should not be GCd while firing event listeners or with custom properties.

[Jer Noble]

I find that I'm having a very hard time writing a LayoutTest for this case.  How does one write a layout test to check whether an object has been GCd without holding a reference to that object (and thus keeping it from being GCd)?

[Jer Noble]

Created attachment 188011 [details]
Patch

[Jer Noble]

Created attachment 188012 [details]
Patch

This time, with project changes.

[Geoffrey Garen]

> I find that I'm having a very hard time writing a LayoutTest for this case.  How does one write a layout test to check whether an object has been GCd without holding a reference to that object (and thus keeping it from being GCd)?

mediaKeys.mediaKeySession.customProperty = 1;
gc();
if (mediaKeys.mediaKeySession.customProperty)
    pass();
else
    fail();

Here, your only explicit reference is to the MediaKeys object, and not the MediaKeySession object.

[Geoffrey Garen]

Comment on attachment 188012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188012&action=review

> Source/WebCore/bindings/js/JSMediaKeySessionCustom.cpp:54
> +    return visitor.containsOpaqueRoot(mediaKeySession->keys());

This line will only keep the MediaKeySession object alive if another object has called "visitor.addOpaqueRoot(keys)".

If you want a MediaKeys object to keep a MediaKeySession object alive, you also need MediaKeys to call "visitor.addOpaqueRoot(keys)".

[Geoffrey Garen]

Further review on IRC:

ggaren: jer: so, here's what you need:
[11:14am] ggaren: jer: (1) MediaKeySession needs to adopt the ActiveDOMObject API, so it won't GC in the middle of scheduled stuff
[11:15am] jer: yep.
[11:15am] ggaren: jer: (2) MediaKeySession needs to adopt the isReachable API, but only for the purpose of event listeners, and this should be automatic for any subclass of EventTarget -- so just double-check that it is auto-generated
[11:16am] jer: okay, i looked into that, and it does do it for event listeners, but not custom properties…. *double checks*
[11:17am] jer: oh never mind, it's both
[11:17am] jer: ok
[11:18am] ggaren: jer: (3) double-check that the automatic isReachable for event listener checks for isFiringEventListeners -- this might be an oversight in the auto generated code, which you should fix (one-line change in the auto generater)
[11:18am] jer: so, what about the "isFiringEventLiseteners" part? that should be handled by bumping ...
[11:18am] jer: okay, EventTarget doesn't check isFiringEventLiseteners.
[11:18am] ggaren: jer: i think that's a missing feature in the auto generator, which you should correct
[11:18am] jer: lol. ok.
[11:19am] ggaren: (4) MediaKeys needs to to check visitor.containsOpaqueRoot(root(HTMLMediaElement)) -- because of the HTMLMediaElement.mediaKeys API

[Jer Noble]

Created attachment 188652 [details]
Patch

[Jer Noble]

Depends on bug #109702 for the mediaElement accessor on MediaKeys.

[Jer Noble]

Whoops, an extra entry in the LayoutTests/ChangeLog.  I'll remove it before landing (or an upcoming version, depending).

[Brent Fulgham]

(In reply to comment #9)
> Whoops, an extra entry in the LayoutTests/ChangeLog.  I'll remove it before landing (or an upcoming version, depending).

Wow -- did this ever get landed? Seems important!

[Jer Noble]

This was fixed by bug #129841.

*** This bug has been marked as a duplicate of bug 129841 ***

[Csaba Osztrogonác]

Comment on attachment 188652 [details]
Patch

Cleared review? from attachment 188652 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).