This is an archived snapshot of W3C's public bugzilla bug tracker, decommissioned in April 2019. Please see the home page for more details.

Bug 26575 - Separate creation of the MediaKeySession from "message" event generation
Summary: Separate creation of the MediaKeySession from "message" event generation
Status: RESOLVED FIXED
Alias: None
Product: HTML WG
Classification: Unclassified
Component: Encrypted Media Extensions (show other bugs)
Version: unspecified
Hardware: All All
: P1 normal
Target Milestone: LC
Assignee: David Dorwin
QA Contact: HTML WG Bugzilla archive list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-14 00:02 UTC by David Dorwin
Modified: 2014-08-27 18:58 UTC (History)
7 users (show)

See Also:


Attachments

Description David Dorwin 2014-08-14 00:02:30 UTC
The current APIs assume the following model for creating sessions:
  mediaKeys.createSession(initDataType, initData).then(
    function(keySession) {
      keySession.addEventListener("message", licenseRequestReady, false);
    }
  );

This requires that the "message" event not be fired until the resolve function is run. Unfortunately, there is nothing that guarantees this ordering. We could try to make the ordering more likely by introducing artificial delays, but that is really just bandaging over the problem.

The same applies to loadSession() and messages it might generate.

Unless we want to abandon the use of the "message" event for these initial messages (e.g. license requests), we need to separate the creation of the MediaKeySession object from anything that might generate a "message" event.
Comment 1 David Dorwin 2014-08-14 00:07:47 UTC
*** Option #1 ***
One option is something we discussed when createSession() was first adopted (before promises): a separate generateLicenseRequest() method. At the time, the asynchronous timing issue did not exist and a license request would always be generated when a session was created, so we chose to combine the two operations. Now, we have asynchronous object creation and session loading.

** Changes **
Make creation of MediaKeySession objects a distinct synchronous method (keeping the createSession name):
  MediaKeySession createSession(optional SessionType sessionType = "temporary");
Note: The sessionId attribute is initially empty.

Move the old createSession() and loadSession() algorithms to MediaKeySession and rename them:
 * Promise<void> generateLicenseRequest(DOMString initDataType, (ArrayBuffer or ArrayBufferView) initData);
 * Promise<bool> load(DOMString sessionId);

Apps would always follow these synchronous steps:
1. Call mediaKeys.createSession().
2. Add an event handler for "message".
3. Call generateLicenseRequest() or load().

** Examples **
New session:
  var session = mediaKeys.createSession();
  session.addEventListener("message", handleMessage, false);
  session.generateLicenseRequest(initDataType, initData).then(
    function() {
      // There is nothing to do. Just wait for the "message" event.
    }
  ).catch() {
    function() {
      // Handle the failure.
    }
  );

Load session:
  var session = mediaKeys.createSession("persistent");
  session.addEventListener("message", handleMessage, false);
  session.load(sessionId).then(
    function(isFound) {
      if (isFound) {
        // Session not found.  
      } 
      else
        // Session is loaded - use it. May also receive a "message" event in an undetermined order.
      } 
    }
  ).catch() {
    function() {
      // Handle the failure. (As with the current API, per the Promises Guide, this does not include failing to find the session.)
    }
  );
Comment 2 David Dorwin 2014-08-14 01:21:59 UTC
Separating license request generation from session creation opens the possibility of generateLicenseRequest() being called twice on the same session. For now, any subsequent calls would always fail to be consistent with the current model of one initData per session.
Comment 3 Mark Watson 2014-08-19 15:36:13 UTC
Is it not possible, in the existing model, to simply require that the promise MUST be resolved (and the resolution hander executed) before any message events are generated ?

In Option #1, perhaps a better name for generateLicenseRequest would just be init() ? This makes it more obvious why it should fail if done a second time.
Comment 4 Domenic Denicola 2014-08-19 15:40:01 UTC
> Is it not possible, in the existing model, to simply require that the promise MUST be resolved

Sure

> (and the resolution hander executed)

Impossible; a handler can be added to the promise at any time.
Comment 5 Mark Watson 2014-08-19 17:10:30 UTC
(In reply to Domenic Denicola from comment #4)
> > Is it not possible, in the existing model, to simply require that the promise MUST be resolved
> 
> Sure
> 
> > (and the resolution hander executed)
> 
> Impossible; a handler can be added to the promise at any time.

Ah, let me rephrase: could the specification require that strictly before any message event generated, the promise must be resolved and any resolution handler present *at the time of resolution* be executed ?

When I say in the specification "resolve the promise", when does any handler present at that time get executed ? Right away ? Or is a task queued somewhere to execute that handler ?

If a task is queued, then if I later say "queue a task to fire a simple event ..." can I be sure that this task is put into the same queue as the task to execute the resolution handler ?
Comment 6 Domenic Denicola 2014-08-19 17:36:54 UTC
Yes, you can always add an artificial delay (be it one task, or one second, or whatever) to allow a grace period before messages are dropped. But this seems like problematic API design that is better avoided.
Comment 7 Mark Watson 2014-08-19 17:41:10 UTC
(In reply to Domenic Denicola from comment #6)
> Yes, you can always add an artificial delay (be it one task, or one second,
> or whatever) to allow a grace period before messages are dropped. But this
> seems like problematic API design that is better avoided.

I wasn't think so much of a "delay" (which implies the possibility of race conditions) as being explicit about the order in which things are placed into the same queue (with an assumption that the queue is FIFO).
Comment 8 Jerry Smith 2014-08-22 16:37:36 UTC
I think it would be best to handle this race condition by separating session creation from requesting a license, like David's proposed.  This is a deterministic solution.  Introducing a wait until the key message handler is created might be made to work, but seems less clean and direct.  Mark:  Do you have other reasons why adding generateLicenseRequest() might be undesirable?
Comment 9 Joe Steele 2014-08-25 16:54:37 UTC
If this is adopted, then the new generateLicenseRequest method would be the appropriate place for modifiers to indicate to the CDM whether the licenses requested should be persistent or ephemeral, and whether they should be loaded from local cache or not. 

In fact -- this might be a good time to introduce a loadCachedLicense() call that only loads locally cached licenses. If it fails, then a generateLicenseRequest() would be called. 

Alternately this use case could be handled with parameters on generateLicenseRequest() but that seems less clean and obvious in intent.
Comment 10 David Dorwin 2014-08-25 18:47:42 UTC
https://dvcs.w3.org/hg/html-media/rev/19ef12173afb (along with the uninteresting https://dvcs.w3.org/hg/html-media/rev/ec33087ef0db) implements something close to the proposal in comment #1. The primary differences are createSession() is still asynchronous (see below) and generateRequest() instead of generateLicenseRequest().

The IDL is as follows:

MediaKeys
 Promise<MediaKeySession> createSession(optional SessionType sessionType = "temporary");

MediaKeySession
 Promise<void> generateRequest(DOMString initDataType, (ArrayBuffer or ArrayBufferView) initData);
 Promise<bool> load(DOMString sessionId);

Note the return types. load()'s bool indicates whether the session was found, since not found is not a rejectable condition.


createSession() still asynchronously returns the MediaKeySession in a promise, but I think we can probably make it synchronous since it does not really do anything or touch the CDM (see the algorithm). Does anyone see a reason not to make it synchronous as proposed in comment #1?
 MediaKeySession createSession(optional SessionType sessionType = "temporary");


The algorithms have to track both the uninitialized state (neither generateRequest() nor load() has been called) and callable state generateRequest() or load() has completed successfully).
Comment 11 Joe Steele 2014-08-26 16:56:36 UTC
The  generateRequest(initDataType, initData) method algorithm is incorrect.
Step 10.7 reads - "Run the Queue a "message" Event algorithm on the session, providing request and null."

This should instead read "Run the Queue a "message" Event algorithm on the session, providing request and the destinationURL."
Comment 12 David Dorwin 2014-08-26 20:10:38 UTC
https://dvcs.w3.org/hg/html-media/rev/30e440862bc9 makes createSession() synchronous as in comment #1.

(https://dvcs.w3.org/hg/html-media/rev/4c40f901e80e is a minor fix to the previous to changesets.)

This bug is now fixed. We can address any oversights bugs with the change per the normal process.
Comment 13 Joe Steele 2014-08-26 20:27:50 UTC
I do not believe I can agree this is fixed until the text I mentioned is fixed. It is blocked by the same issue as Bug 26401.
Comment 14 Shinya Maruyama 2014-08-27 07:40:16 UTC
The bug seems to be introduced by this change in example '8.5. Stored License'.

  // Called if the application does not have a stored sessionId for the media resource.
  function makeNewRequest(mediaKeys, initDataType, initData) {
    var keySession = mediaKeys.createSession("persistent");
    keySession.addEventListener("message", handleMessage, false);
    keySession.closed.then(
      function() {
        console.log("Session " + this.sessionId + " closed");
      }.bind(keySession)        
    );
    // Store keySession.sessionId in the application.
    keySession.generateRequest(initDataType, initData).catch(
      console.error.bind(console, "Unable to create or initialize a persistable key session")
    );
  }

"Store keySession.sessionId in the application" needs to be moved after generateRequest is resolved because sessionId attribute is not available at the timing above.
Essentially this seems to be the API design issue. I believe application should store the sessionId before the session data is persisted within update() to avoid application losing the sessionId. However, as the generateRequest is asynchronous, the API can no longer expect that sessionId is made available to the application before update() unless the application takes a special care for the timing.
For the reason, the API which sets the sessionId attribute should be separate from generateRequest and load methods, for example, adding init() method;
 mediakeysession.init(optional sessionId)
  - if sessionId was not provided, create a new sessionId
  - set the sessionId to sessionId attribute
 mediakeysession.load() // sessionId parameter is removed

Alternatively, we may have to get back to the separate loadSession(sessionId). In that case, the sessionId attribute is set by either createSession() or loadSession(). Then, the sessionId parameter is removed from the mediakeysession.load(). If load() method takes optional initData parameter, this is useful for someone who prefer to use initData for loading persistent license. Meaning load() method can be used with both createSession and loadSession. Once license is persisted, the application can load the persistent license by; createSession(sessionType=persistent) followed by load(initDataType, initData) instead of loadSession(sessionId). It maybe helps resolve the discussion on http://lists.w3.org/Archives/Public/public-html-media/2014Aug/0016.html. 

In addition, probably it's not the bug introduced by this change but there seems to be the bug in 8.5 Stored License regarding the removal of sessionId.

  // Called when the application wants to remove the stored license.
  // The stored session data has not been completely removed until the promise returned by remove() is fulfilled.
  // The remove() call may initiate a series of messages to/from the server that must be completed before this occurs.
  function removeStoredSession(keySession) {
    keySession.remove().then(
      function() {
        console.log("Session " + this.sessionId + " removed");
        // The application should remove its record of this.sessionId.
      }.bind(keySession)
    ).catch(
      console.error.bind(console, "Failed to remove the session")
    );
  }

The sessionId should not be removed at the timing if remove() requires message exchange(s).
Comment 15 Mark Watson 2014-08-27 15:19:50 UTC
I agree with Joe, we should not make changes that are unrelated to the bug, especially if they are controversial.
Comment 16 David Dorwin 2014-08-27 17:58:34 UTC
(In reply to Joe Steele from comment #13)
> I do not believe I can agree this is fixed until the text I mentioned is
> fixed. It is blocked by the same issue as Bug 26401.

The exact text you quoted in comment #11 was present before this bug was fixed. You could have easily checked this in the change [1] in comment #10 or the previous version of the spec [2]. As you noted, there is already a bug tracking your concern. Please don't obstruct progress on other issues.

(In reply to Mark Watson from comment #15)
> I agree with Joe, we should not make changes that are unrelated to the bug,
> especially if they are controversial.

We didn't.


[1] https://dvcs.w3.org/hg/html-media/rev/19ef12173afb
[2] https://dvcs.w3.org/hg/html-media/raw-file/9a94854a5999/encrypted-media/encrypted-media.html#dom-createsession
Comment 17 David Dorwin 2014-08-27 18:01:27 UTC
(In reply to Shinya Maruyama from comment #14)
> "Store keySession.sessionId in the application" needs to be moved after
> generateRequest is resolved because sessionId attribute is not available at
> the timing above.

Thanks for catching this. Fixed in https://dvcs.w3.org/hg/html-media/rev/3a5e8f5332a2.

> Essentially this seems to be the API design issue. I believe application
> should store the sessionId before the session data is persisted within
> update() to avoid application losing the sessionId. However, as the
> generateRequest is asynchronous, the API can no longer expect that sessionId
> is made available to the application before update() unless the application
> takes a special care for the timing.

The sessionId is known when the promise returned by generateRequest() is resolved. This should be well before a "message" event can be handled, including a server roundtrip. In theory, "callable" could be set to true and update() called before the resolve function(s) on the promise are run. If this was an issue, the application could either delay the update() call or ensure that the sessionId is stored before calling update() (the sessionId is guaranteed to be valid before "callable" is true). Another option would be to save the returned promise and use it in update():

  var generatePromise = session.generateRequest();

  function handleResponse(response) {
    generatePromise.then(
      function {
        session.update(response);
      }
    );
  }

This works because generatePromise.then() will always call the resolve function once it's been resolved. Of course, I don't think there is any guarantee that the other resolve function will run before this one, so this might need some tweaking.

We've basically moved the very real and common timing issue of firing the "message" event before a listener can be added in a promise resolver to a theoretical issue of update() being called before the sessionId can be recorded in a promise resolver. I'm open to suggestions on algorithmically addressing this issue.

> The sessionId should not be removed at the timing if remove() requires
> message exchange(s).

Step 5.2.1 of the remove() algorithm guarantees that the resolve function is not called until the message exchange completes. This was intentional to handle this scenario.


The rest of the discussion appears unrelated to this bug.
Comment 18 Joe Steele 2014-08-27 18:41:33 UTC
(In reply to David Dorwin from comment #16)
> (In reply to Joe Steele from comment #13)
> > I do not believe I can agree this is fixed until the text I mentioned is
> > fixed. It is blocked by the same issue as Bug 26401.
> 
> The exact text you quoted in comment #11 was present before this bug was
> fixed. You could have easily checked this in the change [1] in comment #10
> or the previous version of the spec [2]. 

I realize that and I apologize for not catching it sooner. 

> As you noted, there is already a bug tracking your concern. 
> Please don't obstruct progress on other issues.

That is not what I noted. They are blocked by the same issue, but they are not the same bug. The other bug is specifically about the example code. This bug is about the actual implementation. The implementation as defined is incorrect. Fixing the other bug will not fix this one. I can file a new bug instead if you like.
Comment 19 Mark Watson 2014-08-27 18:58:33 UTC
I think we should reopen 25920. In the discussion (and indeed title) of that bug we talked about removing the extraction of the default URL from the createSession() procedure. But the implementation of that appears to have removed the possibility for the CDM to populate the destinationURL of the keymessage event. I think it was explicit in the discussion that we would NOT do that.

Apologies for missing that at the time.