Bug 97037 – Bring WebKit up to speed with latest Encrypted Media spec.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Bring WebKit up to speed with latest Encrypted Media spec.

[Jer Noble]

Created attachment 164609 [details]
Patch

Initial WIP.

[Jer Noble]

Created attachment 164635 [details]
Patch

WIP v2; Added a ENABLE_ENCRYPTED_MEDIA_V2 to allow the older spec implementation to be deprecated.

[Jer Noble]

Created attachment 164768 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 164768 [details]
Patch

Attachment 164768 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13898628

[WebKit Review Bot]

Comment on attachment 164768 [details]
Patch

Attachment 164768 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13900647

[Peter Beverloo (cr-android ews)]

Comment on attachment 164768 [details]
Patch

Attachment 164768 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13894646

[Adam Barth]

Comment on attachment 164768 [details]
Patch

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

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:66
> +bool CDM::supportsKeySystem(const String& keySystem)

CDM -> generally we like to avoid acronyms whenever possible.  Can we us english words to describe the name of this class?

[Build Bot]

Comment on attachment 164768 [details]
Patch

Attachment 164768 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13895612

[Adam Barth]

Comment on attachment 164768 [details]
Patch

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

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:99
> +    ScriptExecutionContext* m_scriptExecutionContext;

I should look more carefully, but having a RefCounted class with a raw pointer to a ScriptExecutionContext is generally a red flag.  How do we know this pointer will stay valid?

[Adam Barth]

Comment on attachment 164768 [details]
Patch

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

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.idl:32
> +    ] MediaKeySession {

It looks like this is a JavaScript object with no mechanism to make sure that the ScriptExecutionContext stays valid.

[David Dorwin]

Comment on attachment 164768 [details]
Patch

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

> Source/WebCore/ChangeLog:42
> +        (WebCore::MediaKeyMessageEventInit::MediaKeyMessageEventInit): Initializer now only takes message and defaultURL

defaultURL is now destinationURL.

> Source/WebCore/ChangeLog:68
> +        (WebCore::MediaKeySession::close): Tell the CDM to clear any saved session keys.

s/clear any saved/release any/
I think that is clearer and less likely to lead to confusion about what "saved" means.

> Source/WebCore/ChangeLog:112
> +        Add an ENABLE(ENCRYPTED_MEDIA_V2) check to the existing ENABLE(ENCRYPTED_MEDIA) one:

Is this accurate? It's a different set of members for "V2".
Presumably you are also adding mediaKeys attribute.

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:29
> +#if ENABLE(ENCRYPTED_MEDIA_V2)

I think the new API should be ENCRYPTED_MEDIA. We can fix that later or change the existing version to ENCRYPTED_MEDIA_DEPRECATED. I can help change this in Chrome if you want.

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:68
> +    Vector<CDMFactory*>& cdms = installedCDMs();

"cdms" is a confusing name when reading the code below. Suggest cdmFactories.
One reason the distinction is important is that the CDMs don't need to be initialized (i.e. loaded) for these functions to run.

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:86
> +static CDMFactory* bestCDMForKeySystem(const String& keySystem)

Key system generally exactly identifies a CDM, so the name is a little strange.
I'm guessing the use case might be different implementations of a key system for different media engines. If that is the case, though, then maybe more information is needed or the CDM selection will affect the media engine (or vice versa).

> Source/WebCore/Modules/encryptedmedia/CDM.h:47
> +    static bool supportsKeySystemMIMETypeAndCodec(const String& keySystem, const String& mimeType, const String& codec);

I don't see any callers. I assume HTMLMediaElement will start deferring to this as part of canPlayType().

> Source/WebCore/Modules/encryptedmedia/CDM.h:51
> +    String generateSessionId();

I think we should preserve sessions as objects internally. That would mean adding a CDMSession class for these methods.

Using an object allows implementations that want to use objects to do so while also allowing other implementations to convert from shell objects to functions that take a session ID. It's much more awkward to go the other way (OO to functions to OO).

If we do that, it might be worth noting that this class represents a MediaKeys object from the spec and CDMSession represents a MediaKeySession.

> Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:96
> +    // NOTE: Currently, no CDMs provide the ability to query for MIME type support.

Shouldn't we push this detail into the CDMs and have this class still poll? You can always have implementations return "yes" until they support real querying.

[Jer Noble]

Comment on attachment 164768 [details]
Patch

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

>> Source/WebCore/Modules/encryptedmedia/CDM.cpp:66
>> +bool CDM::supportsKeySystem(const String& keySystem)
> 
> CDM -> generally we like to avoid acronyms whenever possible.  Can we us english words to describe the name of this class?

Sure.  ContentDecryptionModule is a little verbose though.  Suggestions?

>> Source/WebCore/Modules/encryptedmedia/CDM.cpp:68
>> +    Vector<CDMFactory*>& cdms = installedCDMs();
> 
> "cdms" is a confusing name when reading the code below. Suggest cdmFactories.
> One reason the distinction is important is that the CDMs don't need to be initialized (i.e. loaded) for these functions to run.

Sure thing.

>> Source/WebCore/Modules/encryptedmedia/CDM.cpp:86
>> +static CDMFactory* bestCDMForKeySystem(const String& keySystem)
> 
> Key system generally exactly identifies a CDM, so the name is a little strange.
> I'm guessing the use case might be different implementations of a key system for different media engines. If that is the case, though, then maybe more information is needed or the CDM selection will affect the media engine (or vice versa).

I can change this so it's less strange.

>> Source/WebCore/Modules/encryptedmedia/CDM.h:47
>> +    static bool supportsKeySystemMIMETypeAndCodec(const String& keySystem, const String& mimeType, const String& codec);
> 
> I don't see any callers. I assume HTMLMediaElement will start deferring to this as part of canPlayType().

Yes, I've deleted this function in a later (local) revision.

>> Source/WebCore/Modules/encryptedmedia/CDM.h:51
>> +    String generateSessionId();
> 
> I think we should preserve sessions as objects internally. That would mean adding a CDMSession class for these methods.
> 
> Using an object allows implementations that want to use objects to do so while also allowing other implementations to convert from shell objects to functions that take a session ID. It's much more awkward to go the other way (OO to functions to OO).
> 
> If we do that, it might be worth noting that this class represents a MediaKeys object from the spec and CDMSession represents a MediaKeySession.

That just seems to complicate things for very little benefit.  Right now, there is a single abstract interface to the CDM with very simple lifetime mechanisms.  Adding a CDMSession interface parallel to MediaKeySession opens the door to hairy lifetime management bugs.

>> Source/WebCore/Modules/encryptedmedia/MediaKeySession.idl:32
>> +    ] MediaKeySession {
> 
> It looks like this is a JavaScript object with no mechanism to make sure that the ScriptExecutionContext stays valid.

Would you suggest this be an ActiveDOMObject instead?

>> Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:96
>> +    // NOTE: Currently, no CDMs provide the ability to query for MIME type support.
> 
> Shouldn't we push this detail into the CDMs and have this class still poll? You can always have implementations return "yes" until they support real querying.

Sorry, this has been implemented in my current (local) revision.

[Jer Noble]

(In reply to comment #11)
> (From update of attachment 164768 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164768&action=review
> 
> > Source/WebCore/ChangeLog:42
> > +        (WebCore::MediaKeyMessageEventInit::MediaKeyMessageEventInit): Initializer now only takes message and defaultURL
> 
> defaultURL is now destinationURL.

Got it.  Will change.

> > Source/WebCore/ChangeLog:68
> > +        (WebCore::MediaKeySession::close): Tell the CDM to clear any saved session keys.
> 
> s/clear any saved/release any/
> I think that is clearer and less likely to lead to confusion about what "saved" means.

Will change.

> > Source/WebCore/ChangeLog:112
> > +        Add an ENABLE(ENCRYPTED_MEDIA_V2) check to the existing ENABLE(ENCRYPTED_MEDIA) one:
> 
> Is this accurate? It's a different set of members for "V2".

It's a subset, yes.

> Presumably you are also adding mediaKeys attribute.

::boggle::  I completely left out the mediaKeys attribute.  Will change.

> > Source/WebCore/Modules/encryptedmedia/CDM.cpp:29
> > +#if ENABLE(ENCRYPTED_MEDIA_V2)
> 
> I think the new API should be ENCRYPTED_MEDIA. We can fix that later or change the existing version to ENCRYPTED_MEDIA_DEPRECATED. I can help change this in Chrome if you want.
> 
> > Source/WebCore/Modules/encryptedmedia/CDM.cpp:68
> > +    Vector<CDMFactory*>& cdms = installedCDMs();
> 
> "cdms" is a confusing name when reading the code below. Suggest cdmFactories.
> One reason the distinction is important is that the CDMs don't need to be initialized (i.e. loaded) for these functions to run.

Will change.

[Jer Noble]

(In reply to comment #11)
> (From update of attachment 164768 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164768&action=review
> 
> > Source/WebCore/Modules/encryptedmedia/CDM.cpp:29
> > +#if ENABLE(ENCRYPTED_MEDIA_V2)
> 
> I think the new API should be ENCRYPTED_MEDIA. We can fix that later or change the existing version to ENCRYPTED_MEDIA_DEPRECATED. I can help change this in Chrome if you want.

Lets do it in another patch.  This one is big enough as it is. :)

Once Chrome has switched over to the _DEPRECATED flag, we can remove the _V2 from this new one.

[Jer Noble]

Created attachment 164872 [details]
Patch

This revision still has the ScriptExecutionContext which Adam mentioned, but should solve the compilation ploblems on platforms which do not enable ENCRYPTED_MEDIA_V2.

[Jer Noble]

This patch also has all the necessary changes to Platform.h to enable this new flag on the Mac port, as well as new tests for the new spec API.

[Jer Noble]

(In reply to comment #15)
> Created an attachment (id=164872) [details]
> Patch
> 
> This revision still has the ScriptExecutionContext [problem] which Adam mentioned...

This comment missing word.

[Jer Noble]

Created attachment 164874 [details]
Patch

Made MediaKeySession inherit from ContextDestructionObserver, which should allay the problems Adam has with the bare ScriptExecutionContext pointer.

[WebKit Review Bot]

Comment on attachment 164874 [details]
Patch

Attachment 164874 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13950157

[Peter Beverloo (cr-android ews)]

Comment on attachment 164874 [details]
Patch

Attachment 164874 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13953115

[Jer Noble]

Created attachment 164927 [details]
Patch

Wrapped the HTMLMediaElement mediaKey attribute in more careful conditionals. It should no longer be defined for the deprecated flag.

[Jer Noble]

Created attachment 164929 [details]
Patch

Apparently the V8 code generator won't wrap #includes with the same conditionals it uses when wrapping the attributes which require them.

[Adam Barth]

> Would you suggest this be an ActiveDOMObject instead?

That will certainly work.

[Jer Noble]

(In reply to comment #23)
> > Would you suggest this be an ActiveDOMObject instead?
> 
> That will certainly work.

Well, I'm not sure that MediaKeySession will have any long running tasks which would necessitate being an ActiveDOMObject.  I.e., I don't know if the "fire a task" portion of the spec would require calling setPendingActivity.  If it is, I can certainly do that, but for the mean time I've made MediaKeySession a ContextDestructionObserver.

[Jer Noble]

And David, I've changed my mind on making a separate CDMSession class (or interface).  I'll get that change into an upcoming patch.

[Adam Barth]

> Well, I'm not sure that MediaKeySession will have any long running tasks which would necessitate being an ActiveDOMObject.  I.e., I don't know if the "fire a task" portion of the spec would require calling setPendingActivity.  If it is, I can certainly do that, but for the mean time I've made MediaKeySession a ContextDestructionObserver.

That works too.  :)

Thanks.

[David Dorwin]

Comment on attachment 164929 [details]
Patch

Some comments related to session lifetime. I'll wait for a new patch with CDMSession before reviewing further.

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

> Source/WebCore/ChangeLog:15
> +        Tests: media/encrypted-media/encrypted-media-v2-events.html

I think we should have the test names correct from the beginning. If overrides get added later, it will be harder to fix.
Chromium does not have overrides, so you can rename the current tests to -v01 or -deprecated completely within WK then use the new name for your tests.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:58
> +    close();

I don't think this is correct. A CDM can still use a session's keys even if no representation of the session exists in the DOM.
close() is intended to give the application explicit control.

close() and "noMoreDomReferences()" (i.e. dereferencing the new CDMSession) are different messages. If the latter is received after the former, the session can be erased. Otherwise, the session must remain in the CDM implementation.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:69
> +        m_keys->cdm()->clearKeysFromSessionId(m_sessionId);

It seems that closeSession() would be better. CDMs will need to know that this means to clear the keys, but there are other things that closing a session ID might mean.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:70
> +    m_asyncEventQueue->cancelAllEvents();

I'm not sure this is correct. The act of closing may trigger events, such as key release messages. close() does not destroy the key session.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:47
> +class MediaKeySession : public RefCounted<MediaKeySession>, public EventTarget, public ContextDestructionObserver {

We need to figure out some lifetime issues in the DOM. What should happen if there are no references to a MediaKeys object in the DOM but there is still a reference to one of its MediaKeySession objects? Does the former disappear from the DOM but is kept alive internally? I think that might require a change to the implementation to hold a reference to MediaKeys from the MediaKeySession (the opposite of the current implementation).

[WebKit Review Bot]

Comment on attachment 164929 [details]
Patch

Attachment 164929 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13950290

New failing tests:
media/encrypted-media/encrypted-media-v2-syntax.html
media/encrypted-media/encrypted-media-v2-events.html
media/encrypted-media/encrypted-media-can-play-type.html

[Jer Noble]

Comment on attachment 164929 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        Tests: media/encrypted-media/encrypted-media-v2-events.html
> 
> I think we should have the test names correct from the beginning. If overrides get added later, it will be harder to fix.
> Chromium does not have overrides, so you can rename the current tests to -v01 or -deprecated completely within WK then use the new name for your tests.

Okay, will do.

>> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:58
>> +    close();
> 
> I don't think this is correct. A CDM can still use a session's keys even if no representation of the session exists in the DOM.
> close() is intended to give the application explicit control.
> 
> close() and "noMoreDomReferences()" (i.e. dereferencing the new CDMSession) are different messages. If the latter is received after the former, the session can be erased. Otherwise, the session must remain in the CDM implementation.

Except, at this point, the CDMSession's lifetime is owned by the MediaKeySession.  If it's deleted, the CDMSession will be deleted soon after.  I thought it would make sense to release all the keys when the CDM was deleted, but we could also just make this the responsibility of the CDMSession itself.

So, if what you're saying is true (that the session must remain active until it's closed, it sounds like MediaKeySession /should/ be an ActiveDOMObject, and should bump it's activity count when a key is added and clear it when close() is called.  That way, the CDMSession stays alive, but doesn't have to persist past the end of the lifetime of the MediaKeySession object.

>> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:69
>> +        m_keys->cdm()->clearKeysFromSessionId(m_sessionId);
> 
> It seems that closeSession() would be better. CDMs will need to know that this means to clear the keys, but there are other things that closing a session ID might mean.

Okay, I had renamed this "releaseKeys()" on the new CDMSession object.  But I can further rename it to just "close()".

>> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:70
>> +    m_asyncEventQueue->cancelAllEvents();
> 
> I'm not sure this is correct. The act of closing may trigger events, such as key release messages. close() does not destroy the key session.

Okay.  I'll remove this.

>> Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:47
>> +class MediaKeySession : public RefCounted<MediaKeySession>, public EventTarget, public ContextDestructionObserver {
> 
> We need to figure out some lifetime issues in the DOM. What should happen if there are no references to a MediaKeys object in the DOM but there is still a reference to one of its MediaKeySession objects? Does the former disappear from the DOM but is kept alive internally? I think that might require a change to the implementation to hold a reference to MediaKeys from the MediaKeySession (the opposite of the current implementation).

More likely, this will need some isReachable hacks, but even so, this has the potential to cause some circular-reference leaks.

At what point should an MediaKeysSession be collectable?  Only if it has no keys (because none have been added or the session has been close()d) ?

[WebKit Review Bot]

Comment on attachment 164929 [details]
Patch

Attachment 164929 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13950302

New failing tests:
media/encrypted-media/encrypted-media-v2-syntax.html
media/encrypted-media/encrypted-media-v2-events.html
media/encrypted-media/encrypted-media-can-play-type.html

[Jer Noble]

Okay, how's this for an idea:

- a MediaKeys object is considered "active" when it has at least one "active" session
- a MediaKeySession object  is considered "active" when it has at least one outstanding keyRequest, or has had a key added, and has not been "closed".
- Once a MediaKeySession has been "closed", it can no longer generate key requests or add keys.  Adding a key will throw an INVALID_STATE_ERR exception.

This would guarantee the following:

- The MediaKeys object would not get GCd while there were active keys in any of its MediaKeySession objects
- Once all a MediaKeys' session object were closed, it would be GCd.
- If ever a MediaKeySession object outlived its MediaKeys, it could no longer add keys or create key requests. (Because that session must have been closed.)
- Ownership would still flow normally (MediaKeys would still own MediaKeySessions & CDMs, MediaKeySessions would still own CDMSessions), without needing upstream ownership.

[David Dorwin]

(In reply to comment #29)
> >> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:58
> >> +    close();
> > 
> > I don't think this is correct. A CDM can still use a session's keys even if no representation of the session exists in the DOM.
> > close() is intended to give the application explicit control.
> > 
> > close() and "noMoreDomReferences()" (i.e. dereferencing the new CDMSession) are different messages. If the latter is received after the former, the session can be erased. Otherwise, the session must remain in the CDM implementation.
> 
> Except, at this point, the CDMSession's lifetime is owned by the MediaKeySession.  If it's deleted, the CDMSession will be deleted soon after.  I thought it would make sense to release all the keys when the CDM was deleted, but we could also just make this the responsibility of the CDMSession itself.

The intent of the spec is that the MediaKeySession object in the DOM does not represent the lifetime of a session (and its keys). Technically, an app could call addKey() then release all references to the MKS. The MKS wouldn't exist in the DOM, but the session (or at least its keys) would still exist in the CDM.

Maybe this is weird and DOM references _should_ manage lifetime (I can think of a couple problems with this) or the MediaKeys (aka CDM) should hold a reference until close() is called, at which time it releases the reference and the MKS will only exist as long as there is a reference in the DOM. The latter seems harder to get right with respect to CDM implementations (for example, the CDM firing a key release message).
> 
> So, if what you're saying is true (that the session must remain active until it's closed, it sounds like MediaKeySession /should/ be an ActiveDOMObject, and should bump it's activity count when a key is added and clear it when close() is called.  That way, the CDMSession stays alive, but doesn't have to persist past the end of the lifetime of the MediaKeySession object.

I assume "have" means can if the CDM needs it to. Really, it puts the CDM in control once the MKS is destroyed. If so, sounds good.



> >> Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:47
> >> +class MediaKeySession : public RefCounted<MediaKeySession>, public EventTarget, public ContextDestructionObserver {
> > 
> > We need to figure out some lifetime issues in the DOM. What should happen if there are no references to a MediaKeys object in the DOM but there is still a reference to one of its MediaKeySession objects? Does the former disappear from the DOM but is kept alive internally? I think that might require a change to the implementation to hold a reference to MediaKeys from the MediaKeySession (the opposite of the current implementation).
> 
> More likely, this will need some isReachable hacks, but even so, this has the potential to cause some circular-reference leaks.
> 
> At what point should an MediaKeysSession be collectable?  Only if it has no keys (because none have been added or the session has been close()d) ?

The DOM object MKS should be collectable whenever there are no references to it, just like any other object. I suppose the CDM implementation gets to decide when it needs it, which means it may not need it if no keys have been added. The CDM can also decide when after close() it no longer needs it. When the MKS has been collected and the CDM no longer needs the session, the underlying object (CDMSession?) can be destroyed.

[Jer Noble]

(In reply to comment #32)
> (In reply to comment #29)
> > >> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:58
> > >> +    close();
> > > 
> > > I don't think this is correct. A CDM can still use a session's keys even if no representation of the session exists in the DOM.
> > > close() is intended to give the application explicit control.
> > > 
> > > close() and "noMoreDomReferences()" (i.e. dereferencing the new CDMSession) are different messages. If the latter is received after the former, the session can be erased. Otherwise, the session must remain in the CDM implementation.
> > 
> > Except, at this point, the CDMSession's lifetime is owned by the MediaKeySession.  If it's deleted, the CDMSession will be deleted soon after.  I thought it would make sense to release all the keys when the CDM was deleted, but we could also just make this the responsibility of the CDMSession itself.
> 
> The intent of the spec is that the MediaKeySession object in the DOM does not represent the lifetime of a session (and its keys). Technically, an app could call addKey() then release all references to the MKS. The MKS wouldn't exist in the DOM, but the session (or at least its keys) would still exist in the CDM.

But that's only true if the MediaKeys object has been added to a HTMLMediaElement.  Which therefore references the MediaKeys object, which in turn references the MediaKeySession object.  

If the MediaSession is owned by a MediaKeys object which is has not been added to a HTMLMediaElement, and no reference to that MediaKeys or MediaSession exists in JS, I would expect all the objects, including the CDM and CDMSession, to be GCd and deleted.

> Maybe this is weird and DOM references _should_ manage lifetime (I can think of a couple problems with this) or the MediaKeys (aka CDM) should hold a reference until close() is called, at which time it releases the reference and the MKS will only exist as long as there is a reference in the DOM. The latter seems harder to get right with respect to CDM implementations (for example, the CDM firing a key release message).

That's an easily solved problem; TextTrackCue, for instance, has this same problem, and works around it by fending off GC while a message is being delivered.

> > >> Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:47
> > >> +class MediaKeySession : public RefCounted<MediaKeySession>, public EventTarget, public ContextDestructionObserver {
> > > 
> > > We need to figure out some lifetime issues in the DOM. What should happen if there are no references to a MediaKeys object in the DOM but there is still a reference to one of its MediaKeySession objects? Does the former disappear from the DOM but is kept alive internally? I think that might require a change to the implementation to hold a reference to MediaKeys from the MediaKeySession (the opposite of the current implementation).
> > 
> > More likely, this will need some isReachable hacks, but even so, this has the potential to cause some circular-reference leaks.
> > 
> > At what point should an MediaKeysSession be collectable?  Only if it has no keys (because none have been added or the session has been close()d) ?
> 
> The DOM object MKS should be collectable whenever there are no references to it, just like any other object. I suppose the CDM implementation gets to decide when it needs it, which means it may not need it if no keys have been added. The CDM can also decide when after close() it no longer needs it. When the MKS has been collected and the CDM no longer needs the session, the underlying object (CDMSession?) can be destroyed.

Well, since the CDM is an entirely opaque object, it can do whatever it wants.  The CDMSession could just be some flyweight proxy whose destruction simply drops the refcount of the underlying session object.  But this is all very handwavy.  I still believe the lifetime of the CDM and the CDMSessions created by it should be tied to the lifetiemes of the MediaKeys and MediaKeySession objects respectively.

[David Dorwin]

Let me know if you want to discuss this more in depth offline.

(In reply to comment #31)
> Okay, how's this for an idea:
> 
> - a MediaKeys object is considered "active" when it has at least one "active" session
Do we need to define this? The CDM's lifetime is the same as MediaKeys', which includes any DOM variables AND video.mediaKeys.
I don't think we need to define anything related to having a session.

> - a MediaKeySession object  is considered "active" when it has at least one outstanding keyRequest, or has had a key added, and has not been "closed".
Per the comments in Comment #32, I think it is more nuanced than this. This is a decent approximation but not true for all CDMs/key systems.

> - Once a MediaKeySession has been "closed", it can no longer generate key requests or add keys.  Adding a key will throw an INVALID_STATE_ERR exception.
generate key request is a one-time thing that occurs at construction. After that, apps and the CDM can communicate via addKey() and keymessage, respectively.

In the key release case, a keymessage is fired at the session object (assuming there are any references and/or attached listeners()), and the application can acknowledge this message by calling addKey(). (FYI, addKey() can be used for things other than adding keys and will probably get a new name.)

Related question: Does a listener add a reference to ensure the object is not destroyed even if all other references are destroyed?
> 
> This would guarantee the following:
> 
> - The MediaKeys object would not get GCd while there were active keys in any of its MediaKeySession objects
As mentioned above, I think basic references will work for MediaKeys. It's only MediaKeySessions that are complicated.

> - Once all a MediaKeys' session object were closed, it would be GCd.
Same as above.

> - If ever a MediaKeySession object outlived its MediaKeys, it could no longer add keys or create key requests. (Because that session must have been closed.)
This is an interesting scenario. Maybe we should throw a INVALID_STATE_ERR. Alternatively, MediaKeySession can hold a reference to MediaKeys to ensure this can never happen.

Another interesting case would be if the media element was destroyed but a MediaKeys reference still existed in the DOM. I guess that should be fine since the CDM should not talk to the MediaPlayer (communication should only be the other way). Maybe that's worth including in the spec.

> - Ownership would still flow normally (MediaKeys would still own MediaKeySessions & CDMs, MediaKeySessions would still own CDMSessions), without needing upstream ownership.

Does MediaKeys need a reference to MediaKeySession or can we leave this to the CDM? Currently, the spec does not specify a relationship in MediaKeys. Thus, reversing the reference should work fine.

(If we ever do add such a relationship to the spec, we can have circular references but have something like MKS::close() remove the reference from MK to MKS.)

[Jer Noble]

Created attachment 186446 [details]
Patch

Rebased against current ToT.

[Build Bot]

Comment on attachment 186446 [details]
Patch

Attachment 186446 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16376314

[Build Bot]

Comment on attachment 186446 [details]
Patch

Attachment 186446 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16374346

[Jer Noble]

Created attachment 186485 [details]
Patch

[Build Bot]

Comment on attachment 186485 [details]
Patch

Attachment 186485 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16366510

New failing tests:
media/encrypted-media/encrypted-media-v2-syntax.html
media/encrypted-media/encrypted-media-constants.html
fast/events/constructors/media-key-event-constructor.html
media/encrypted-media/encrypted-media-v2-events.html
media/encrypted-media/encrypted-media-can-play-type.html

[Build Bot]

Comment on attachment 186485 [details]
Patch

Attachment 186485 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16367494

New failing tests:
media/encrypted-media/encrypted-media-v2-syntax.html
media/encrypted-media/encrypted-media-constants.html
fast/events/constructors/media-key-event-constructor.html
media/encrypted-media/encrypted-media-v2-events.html
media/encrypted-media/encrypted-media-can-play-type.html

[WebKit Review Bot]

Comment on attachment 186485 [details]
Patch

Attachment 186485 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16367528

New failing tests:
media/encrypted-media/encrypted-media-v2-syntax.html
media/encrypted-media/encrypted-media-v2-events.html

[WebKit Review Bot]

Comment on attachment 186485 [details]
Patch

Attachment 186485 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16377412

New failing tests:
media/encrypted-media/encrypted-media-v2-syntax.html
media/encrypted-media/encrypted-media-v2-events.html

[Jer Noble]

Created attachment 186957 [details]
Patch

Address build errors on mac bots.

[Build Bot]

Comment on attachment 186957 [details]
Patch

Attachment 186957 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16409001

[Build Bot]

Comment on attachment 186957 [details]
Patch

Attachment 186957 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16415025

[Build Bot]

Comment on attachment 186957 [details]
Patch

Attachment 186957 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16404035

[WebKit Review Bot]

Comment on attachment 186957 [details]
Patch

Attachment 186957 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16418143

New failing tests:
media/encrypted-media/encrypted-media-v2-syntax.html
media/encrypted-media/encrypted-media-v2-events.html

[WebKit Review Bot]

Comment on attachment 186957 [details]
Patch

Attachment 186957 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16429167

New failing tests:
media/encrypted-media/encrypted-media-v2-syntax.html
media/encrypted-media/encrypted-media-v2-events.html

[Jer Noble]

Created attachment 187340 [details]
Patch

(Finally) fix build issues on 10.8 Mac build bots. Skip new tests on Chromium (where v2 is not yet enabled).

[Jer Noble]

Created attachment 187344 [details]
Patch

Rebased against current ToT.

[Eric Carlson]

Comment on attachment 187344 [details]
Patch

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

The WebCore ChangeLog entry is brilliant, the best ever in a patch I have reviewed. thanks!

> Source/WebCore/ChangeLog:73
> +        (WebCore::MediaKeySession::addKeyTimerFired): Follow the steps in the spec; provide the key data t othe CDM.

Nit: "key data t othe CDM" -> "key data to the CDM"

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2012 is so last year.

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:27
> +#include "CDM.h"

Nit: this can be inside of the ENCRYPTED_MEDIA_V2 guard.

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:58
> +        // TODO: initialize specific UA CDMs.

Nit: "TODO" -> "FIXME", with bug number(s) if possible.

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:76
> +    Vector<CDMFactory*>& cdmFactories = installedCDMFactories();
> +    for (size_t i = 0; i < cdmFactories.size(); ++i) {
> +        if (cdmFactories[i]->supportsKeySystem(keySystem))
> +            return true;
> +    }
> +    return false;

Couldn't this be reduced to "return CDMFactoryForKeySystem(keySystem)" ?

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:79
> +static CDMFactory* CDMFactoryForKeySystem(const String& keySystem)

Does returned factory need to be mutable?

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:109
> +    return m_private->supportsMIMEType(mimeType);

Won't m_private be NULL if it was initialized with an unsupported key system?

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:114
> +    return m_private->createSession();

Ditto.

> Source/WebCore/Modules/encryptedmedia/CDM.h:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> Source/WebCore/Modules/encryptedmedia/CDM.h:44
> +class MediaKeyError;
> +class MediaKeySession;
> +class MediaKeys;
> +
> +class CDM;
> +class CDMPrivateInterface;
> +class CDMSession;

Nit: these could be alphabetized.

> Source/WebCore/Modules/encryptedmedia/CDMPrivate.h:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> Source/WebCore/Modules/encryptedmedia/MediaKeyMessageEvent.h:57
> +    virtual const AtomicString& interfaceName() const;

OVERRIDE.

> Source/WebCore/Modules/encryptedmedia/MediaKeyNeededEvent.h:56
> +    virtual const AtomicString& interfaceName() const;

OVERRIDE.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.idl:2
> + * Copyright (C) 2012 Apple Inc.  All rights reserved.

2013.

> Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> Source/WebCore/Modules/encryptedmedia/MediaKeys.h:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> Source/WebCore/Modules/encryptedmedia/MediaKeys.idl:2
> + * Copyright (C) 2012 Apple Inc.  All rights reserved.

2013.

> Source/WebCore/html/HTMLMediaElement.cpp:1971
>  }

It is probably worth logging a "deprecated!" warning to console the first time this is used in a build which defines ENCRYPTED_MEDIA_V2.

> Source/WebCore/html/HTMLMediaElement.h:454
>      virtual bool mediaPlayerKeyNeeded(MediaPlayer*, const String& keySystem, const String& sessionId, const unsigned char* initData, unsigned initDataLength) OVERRIDE;

OVERRIDE.

> Source/WebCore/html/HTMLMediaElement.h:458
> +    virtual void mediaPlayerKeyNeeded(MediaPlayer*, Uint8Array*);

OVERRIDE.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1082
>  }
>  #endif

Log a "deprecated!" warning to console the first time this is used in a build which defines ENCRYPTED_MEDIA_V2.

> Source/WebCore/testing/MockCDM.cpp:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> Source/WebCore/testing/MockCDM.h:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

2013.

> LayoutTests/media/encrypted-media/encrypted-media-v2-events.html:11
> +                for(var i=0,j=str.length;i<j;++i){
> +                  arr[i]=str.charCodeAt(i);

Nit: braces aren't needed.

> LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:12
> +                for(var i=0,j=str.length;i<j;++i){
> +                  arr[i]=str.charCodeAt(i);
> +                }

Ditto.

[Jer Noble]

(In reply to comment #51)
> (From update of attachment 187344 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187344&action=review
> 
> The WebCore ChangeLog entry is brilliant, the best ever in a patch I have reviewed. thanks!

Thanks!

> > Source/WebCore/ChangeLog:73
> > +        (WebCore::MediaKeySession::addKeyTimerFired): Follow the steps in the spec; provide the key data t othe CDM.
> 
> Nit: "key data t othe CDM" -> "key data to the CDM"

Whoops, fixed.

> > Source/WebCore/Modules/encryptedmedia/CDM.cpp:2
> > + * Copyright (C) 2012 Apple Inc. All rights reserved.
> 
> 2012 is so last year.

Yep. #hipsterglasses

Changed here and everywhere else.

> > Source/WebCore/Modules/encryptedmedia/CDM.cpp:27
> > +#include "CDM.h"
> 
> Nit: this can be inside of the ENCRYPTED_MEDIA_V2 guard.

Changed.

> > Source/WebCore/Modules/encryptedmedia/CDM.cpp:58
> > +        // TODO: initialize specific UA CDMs.
> 
> Nit: "TODO" -> "FIXME", with bug number(s) if possible.

Sure thing. 

> > Source/WebCore/Modules/encryptedmedia/CDM.cpp:76
> > +    Vector<CDMFactory*>& cdmFactories = installedCDMFactories();
> > +    for (size_t i = 0; i < cdmFactories.size(); ++i) {
> > +        if (cdmFactories[i]->supportsKeySystem(keySystem))
> > +            return true;
> > +    }
> > +    return false;
> 
> Couldn't this be reduced to "return CDMFactoryForKeySystem(keySystem)" ?

Sure!

> > Source/WebCore/Modules/encryptedmedia/CDM.cpp:79
> > +static CDMFactory* CDMFactoryForKeySystem(const String& keySystem)
> 
> Does returned factory need to be mutable?

Yep, since we're assigning it to a non-const pointer.

> > Source/WebCore/Modules/encryptedmedia/CDM.cpp:109
> > +    return m_private->supportsMIMEType(mimeType);
> 
> Won't m_private be NULL if it was initialized with an unsupported key system?

I don't think so; that case would be caught in CDM::create(), where it checks to see if the key system is supported before actually creating the CDM object.

Basically, if we got to this point, the key system is supported.

> > Source/WebCore/Modules/encryptedmedia/CDM.h:44
> > +class MediaKeyError;
> > +class MediaKeySession;
> > +class MediaKeys;
> > +
> > +class CDM;
> > +class CDMPrivateInterface;
> > +class CDMSession;
> 
> Nit: these could be alphabetized.

Sorted.

> > Source/WebCore/Modules/encryptedmedia/MediaKeyMessageEvent.h:57
> > +    virtual const AtomicString& interfaceName() const;
> 
> OVERRIDE.

Overridden.

> > Source/WebCore/Modules/encryptedmedia/MediaKeyNeededEvent.h:56
> > +    virtual const AtomicString& interfaceName() const;
> 
> OVERRIDE.

Ditto.

> > Source/WebCore/html/HTMLMediaElement.cpp:1971
> >  }
> 
> It is probably worth logging a "deprecated!" warning to console the first time this is used in a build which defines ENCRYPTED_MEDIA_V2.
> 
> > Source/WebCore/html/HTMLMediaElement.h:454
> >      virtual bool mediaPlayerKeyNeeded(MediaPlayer*, const String& keySystem, const String& sessionId, const unsigned char* initData, unsigned initDataLength) OVERRIDE;
> 
> OVERRIDE.
> 
> > Source/WebCore/html/HTMLMediaElement.h:458
> > +    virtual void mediaPlayerKeyNeeded(MediaPlayer*, Uint8Array*);
> 
> OVERRIDE.
> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:1082
> >  }
> >  #endif
> 
> Log a "deprecated!" warning to console the first time this is used in a build which defines ENCRYPTED_MEDIA_V2.

Sure, but I'll add the message in webkitGenerateKey() and webkitAddKey().

> > LayoutTests/media/encrypted-media/encrypted-media-v2-events.html:11
> > +                for(var i=0,j=str.length;i<j;++i){
> > +                  arr[i]=str.charCodeAt(i);
> 
> Nit: braces aren't needed.

Removed.

> > LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:12
> > +                for(var i=0,j=str.length;i<j;++i){
> > +                  arr[i]=str.charCodeAt(i);
> > +                }
> 
> Ditto.

Ditto.

[Jer Noble]

Committed r142327: <http://trac.webkit.org/changeset/142327>