MediaKeySession should not be GCd while firing event listeners or with custom properties.
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)?
Created attachment 188011 [details] Patch
Created attachment 188012 [details] Patch This time, with project changes.
> 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.
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)".
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
Created attachment 188652 [details] Patch
Depends on bug #109702 for the mediaElement accessor on MediaKeys.
Whoops, an extra entry in the LayoutTests/ChangeLog. I'll remove it before landing (or an upcoming version, depending).
(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!
This was fixed by bug #129841. *** This bug has been marked as a duplicate of bug 129841 ***
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).