Bug 97037 - Bring WebKit up to speed with latest Encrypted Media spec.
Summary: Bring WebKit up to speed with latest Encrypted Media spec.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: WebExposed
Depends on:
Blocks: 109318 109320
  Show dependency treegraph
 
Reported: 2012-09-18 12:57 PDT by Jer Noble
Modified: 2013-02-08 15:33 PST (History)
16 users (show)

See Also:


Attachments
Patch (133.39 KB, patch)
2012-09-18 12:58 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (102.61 KB, patch)
2012-09-18 16:44 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (83.67 KB, patch)
2012-09-19 13:20 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (137.03 KB, patch)
2012-09-20 02:51 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (137.22 KB, patch)
2012-09-20 03:07 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (137.39 KB, patch)
2012-09-20 08:53 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (137.28 KB, patch)
2012-09-20 09:02 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (146.29 KB, patch)
2013-02-04 13:21 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (146.59 KB, patch)
2013-02-04 15:55 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (147.22 KB, patch)
2013-02-06 17:45 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (147.01 KB, patch)
2013-02-08 11:31 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (146.95 KB, patch)
2013-02-08 11:47 PST, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2012-09-18 12:57:57 PDT
Bring WebKit up to speed with latest Encrypted Media spec.
Comment 1 Jer Noble 2012-09-18 12:58:09 PDT
Created attachment 164609 [details]
Patch

Initial WIP.
Comment 2 Jer Noble 2012-09-18 16:44:47 PDT
Created attachment 164635 [details]
Patch

WIP v2; Added a ENABLE_ENCRYPTED_MEDIA_V2 to allow the older spec implementation to be deprecated.
Comment 3 Jer Noble 2012-09-19 13:20:56 PDT
Created attachment 164768 [details]
Patch
Comment 4 Gyuyoung Kim 2012-09-19 13:28:45 PDT
Comment on attachment 164768 [details]
Patch

Attachment 164768 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13898628
Comment 5 WebKit Review Bot 2012-09-19 13:30:39 PDT
Comment on attachment 164768 [details]
Patch

Attachment 164768 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13900647
Comment 6 Peter Beverloo (cr-android ews) 2012-09-19 13:35:02 PDT
Comment on attachment 164768 [details]
Patch

Attachment 164768 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13894646
Comment 7 Adam Barth 2012-09-19 13:36:08 PDT
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?
Comment 8 Build Bot 2012-09-19 13:36:32 PDT
Comment on attachment 164768 [details]
Patch

Attachment 164768 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13895612
Comment 9 Adam Barth 2012-09-19 13:37:54 PDT
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?
Comment 10 Adam Barth 2012-09-19 13:39:58 PDT
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.
Comment 11 David Dorwin 2012-09-19 18:12:06 PDT
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.
Comment 12 Jer Noble 2012-09-20 00:29:21 PDT
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.
Comment 13 Jer Noble 2012-09-20 00:33:59 PDT
(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.
Comment 14 Jer Noble 2012-09-20 00:45:15 PDT
(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.
Comment 15 Jer Noble 2012-09-20 02:51:15 PDT
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.
Comment 16 Jer Noble 2012-09-20 02:52:29 PDT
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.
Comment 17 Jer Noble 2012-09-20 02:53:27 PDT
(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.
Comment 18 Jer Noble 2012-09-20 03:07:30 PDT
Created attachment 164874 [details]
Patch

Made MediaKeySession inherit from ContextDestructionObserver, which should allay the problems Adam has with the bare ScriptExecutionContext pointer.
Comment 19 WebKit Review Bot 2012-09-20 03:34:22 PDT
Comment on attachment 164874 [details]
Patch

Attachment 164874 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13950157
Comment 20 Peter Beverloo (cr-android ews) 2012-09-20 03:50:06 PDT
Comment on attachment 164874 [details]
Patch

Attachment 164874 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13953115
Comment 21 Jer Noble 2012-09-20 08:53:38 PDT
Created attachment 164927 [details]
Patch

Wrapped the HTMLMediaElement mediaKey attribute in more careful conditionals. It should no longer be defined for the deprecated flag.
Comment 22 Jer Noble 2012-09-20 09:02:58 PDT
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.
Comment 23 Adam Barth 2012-09-20 09:58:27 PDT
> Would you suggest this be an ActiveDOMObject instead?

That will certainly work.
Comment 24 Jer Noble 2012-09-20 10:10:42 PDT
(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.
Comment 25 Jer Noble 2012-09-20 10:13:47 PDT
And David, I've changed my mind on making a separate CDMSession class (or interface).  I'll get that change into an upcoming patch.
Comment 26 Adam Barth 2012-09-20 11:11:58 PDT
> 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.
Comment 27 David Dorwin 2012-09-20 11:29:23 PDT
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).
Comment 28 WebKit Review Bot 2012-09-20 13:43:39 PDT
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
Comment 29 Jer Noble 2012-09-20 14:42:37 PDT
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) ?
Comment 30 WebKit Review Bot 2012-09-20 14:45:51 PDT
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
Comment 31 Jer Noble 2012-09-20 15:17:09 PDT
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.
Comment 32 David Dorwin 2012-09-20 21:30:14 PDT
(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.
Comment 33 Jer Noble 2012-09-20 21:47:10 PDT
(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.
Comment 34 David Dorwin 2012-09-20 21:49:32 PDT
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.)
Comment 35 Jer Noble 2013-02-04 13:21:49 PST
Created attachment 186446 [details]
Patch

Rebased against current ToT.
Comment 36 Build Bot 2013-02-04 14:02:27 PST
Comment on attachment 186446 [details]
Patch

Attachment 186446 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16376314
Comment 37 Build Bot 2013-02-04 14:53:53 PST
Comment on attachment 186446 [details]
Patch

Attachment 186446 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16374346
Comment 38 Jer Noble 2013-02-04 15:55:02 PST
Created attachment 186485 [details]
Patch
Comment 39 Build Bot 2013-02-04 17:12:49 PST
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
Comment 40 Build Bot 2013-02-04 18:54:36 PST
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
Comment 41 WebKit Review Bot 2013-02-04 19:54:33 PST
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
Comment 42 WebKit Review Bot 2013-02-04 21:06:19 PST
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
Comment 43 Jer Noble 2013-02-06 17:45:05 PST
Created attachment 186957 [details]
Patch

Address build errors on mac bots.
Comment 44 Build Bot 2013-02-06 19:02:19 PST
Comment on attachment 186957 [details]
Patch

Attachment 186957 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16409001
Comment 45 Build Bot 2013-02-06 19:36:47 PST
Comment on attachment 186957 [details]
Patch

Attachment 186957 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16415025
Comment 46 Build Bot 2013-02-06 20:16:00 PST
Comment on attachment 186957 [details]
Patch

Attachment 186957 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16404035
Comment 47 WebKit Review Bot 2013-02-07 06:37:14 PST
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
Comment 48 WebKit Review Bot 2013-02-07 07:51:17 PST
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
Comment 49 Jer Noble 2013-02-08 11:31:35 PST
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).
Comment 50 Jer Noble 2013-02-08 11:47:17 PST
Created attachment 187344 [details]
Patch

Rebased against current ToT.
Comment 51 Eric Carlson 2013-02-08 12:53:33 PST
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.
Comment 52 Jer Noble 2013-02-08 15:15:45 PST
(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.
Comment 53 Jer Noble 2013-02-08 15:33:07 PST
Committed r142327: <http://trac.webkit.org/changeset/142327>