Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider changing how the MediaKeySession method algorithms run other algorithms to more accurately reflect implementations #14

Closed
ddorwin opened this issue Jan 15, 2015 · 23 comments

Comments

Projects
None yet
6 participants
@ddorwin
Copy link
Contributor

commented Jan 15, 2015

The generateRequest(), load(), update(), and remove() algorithms all run other algorithms after the CDM step and before resolving the promise. This has the advantage of separating responsibilities, ensuring all failures are caught before firing events, and having promise resolution as the last step, but it seems unlikely to accurately represent implementations, possibly in a way that is observable to applications. We should consider whether there is a better way organize things.

I'll use the current load() algorithm as an example.
If the CDM steps succeed, the user agent has been provided with a message. The user agent then:

  1. Initializes some application-observable state (sessionId attribute and callable state)
  2. May run the usable keys changed algorithm, which fires an event at the session.
  3. Runs the update expiration algorithm, which updates the expiration attribute.
  4. May run the queue a "message" event algorithm with the message provided by the CDM.
  5. Resolves the promise.

However, a likely implementation pattern would be one of the following:
I. CDM reports success once all operations have completed.

  1. User agent asks the CDM to load a session.
    1. The CDM loads the session.
    2. The CDM triggers the usable keys changed algorithm.
    3. The CDM triggers the update expiration algorithm.
    4. The CDM provides a message to be provided to the application.
    5. The CDM reports success to the user agent.
  2. The user agent initializes some application-observable state (sessionId attribute and callable state).
  3. The user agent resolves the promise.

II. CDM reports success after processing the request but before all operations have completed.

  1. User agent asks the CDM to load a session.
  2. The CDM loads the session and reports success to the user agent.
  3. The user agent initializes some application-observable state (sessionId attribute and callable state).
  4. The user agent resolves the promise.
  5. In parallel or subsequently:
    1. The CDM triggers the usable keys changed algorithm.
    2. The CDM triggers the update expiration algorithm.
    3. The CDM provides a message to be provided to the application.

Converting either of those implementation patterns to the observable behavior specified in the current algorithm likely requires one of the following undesirable solutions:

  1. Caching requests from the CDM until the CDM reports success.
  2. Session initialization-specific mechanisms for reporting key updates, expiration, and messages.
    An example of (1) would be a CompleteLoad() method that takes all of these values rather than reusing existing SendMessage(), etc. methods.

We should also consider the desired application-observable behavior.

  • Not getting events until the load succeeds is nice.
  • However, it might be weird to get events from an operation before it has succeeded.
  • The callable state must be set to true before any events are fired to ensure that the application is able to respond to the event by calling one of the methods.
  • There is currently a potential race condition between the generateRequest() promise being resolved, indicating that sessionId is valid, and the application receiving the license request message.
  • Since the expiration attribute does not currently have an event, updating the expiration attribute before resolving the promise in the load() and update() algorithms provides a deterministic place for applications to check the expiration.
    • Maybe Object.observe() could be used to detect changes to this attribute instead. Would we get this behavior for free?

Note: The update() algorithm currently has the CDM running the the usable keys changed algorithm and update expiration algorithm rather than deferring these to the user agent. This is more consistent with implementation pattern (I) above. Regardless of the outcome of this bug, the algorithms should be consistent. We should change the steps in those algorithms to be “Provide the user agent with X and have it run the following steps.”

[Migrated from https://www.w3.org/Bugs/Public/show_bug.cgi?id=27138 with only formatting and numbering changes.]

@ddorwin

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2015

The first issue to address is the desired application-observable behavior/ordering.

For generateRequest() and load(), at least, I think the spec should ensure that the application's promise resolve handler is called before the application receives any related events. This would allow the application to do any bookkeeping on the sessionId before handling events. Any other ordering is likely to be confusing to authors and/or difficult to handle. Also, if some implementations (usually) have this ordering, applications may come to rely on it anyway. As I noted above, this probably requires complexity in the client, but I think it is worth it because of the benefit to authors. However, it's unclear whether we can ensure this ordering.

Is it possible to ensure that the application's promise resolve handler (added synchronously with the call) is called before the application receives any events caused by the method call? The primary motivation for bug 26575 was a similar issue - we could not ensure that messages would be fired after the promise was resolved (so that the application could register an event handler). We were able to improve the design to avoid this problem, but a solution to this issue is less obvious. bug 26575 comment 4, notes that "a handler can be added to the promise at any time." However, we only care about promises that are added immediately after the call. Is there any guarantee here? If not, I don't think we can provide what I believe is the desired behavior for applications.

Regardless of the event and promise resolution ordering, I think we should continue to ensure that the sessionId, callable, and expiration are updated before firing events or resolving the promise. (Note: The "update expiration" algorithm call needs to be moved up above the key status step.)

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2015

I agree we should minimize the number of different cases applications need to handle. Whenever you queue the events, it seems possible they could be executed after the promise is resolved (unless it is clear that resolving the promise itself queues a task to do that on the same queue as the event tasks).

So, then it makes sense to ensure that is the only case that needs to be handled by applications. From an application point of view, the first thing that happens is the resolution of the promise, at which pony the sessionId, callable and expiration have been updated. Then the events come.

I'm not sure about the functionality split between CDM and User Agent. Is that normative anyway ? If an implementation has a different functional split but the same externally visible behavior it is surely compliant.

@ddorwin

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2015

I have moved the discussion of ordering, which is independent and more important, to #19. This issue is blocked on #19.

@cpearce

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2015

I agree that it would be nice if we can specify that the MediaKeySession's attributes/state need to be updated before firing events or resolving the promise.

But as I said in bug 19, The Firefox/Adobe combo will need to send "individualization-request" messages and receive a valid response before we can generate a "license-request" as required by the generateRequest() function.

This is important, as it's part of our per-origin individualization; it's part of our privacy story.

Given that, I don't think it's possible to specify that event handlers can't run before the promise returned by generateRequest() is resolved; we will need to send messages and receive responses before we can resolve the promise.

Our implementation requires the sessionId to be set (and observable by JS) before any "individualization-request" messages are sent. So one path forward here would be to move the step that sets the sessionId to earlier in the generateRequest() async section, and add an explicit step after that which specifies when the "individualization-request" messages must be sent, and that awaits the response coming in by update(). Then follow on with the existing logic to generate a requested license.

We could then specify that the "license-request" message gets dispatched after the promise returned by generateRequest() is resolved. It's trivial for the CDM to implement that.

We then have it explicitly in the spec that the sessionId is set before messages can be sent, and a fixed place where messages these messages unexpected by some implementations would be sent.

@ddorwin

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2015

I do not think we should leave the returned promise unresolved while a message exchange occurs. It is confusing and inconsistent for authors. I proposed an alternative in #31.

@cpearce

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2015

OK, I discussed this with Adobe. Sounds like we can get away with sending indiv messages after the promise resolution, but we'll need some way to report errors outside of the promise, as we can fail after the indiv message is sent. So I withdraw my objection, provided we can get https://www.w3.org/Bugs/Public/show_bug.cgi?id=26776 resolved (sorry, I've not had time to comment on that bug, will try to get to it soon).

@ddorwin

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2015

What types of errors do you need to report? Can they be reported by rejecting the promise returned by update()?

@steelejoe

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2015

We will need to report unexpected errors that occur during playback after the promise has been resolved. For example resource exhaustion, mutex contention, RPC failures, decryption failures, etc. On single function devices (STB) these can usually be controlled for, but on general purpose desktops these are not known until playback actually starts.

@paulbrucecotton

This comment has been minimized.

Copy link

commented Apr 28, 2016

@ddorwin, @steelejoe, @cpearce: What is left to resolve here? Issue-120 (Bug 26776) on error codes is resolved and Issue-19 on ordering are both resolved.

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 13, 2016

I take it back.

#19 addressed the message ordering issue for generateRequest() and load() but did not address remove() and update(). Nor did it address whether other (non-message) events - specifically keystatuschange - should be fired before or after promise resolution.

Currently, keystatuschange is executed before the promise resolution for all methods and message events occur before the promise resolution for remove() and update().

I would expect that all events are executed after promise resolution. @ddorwin, @cpearce, @steelejoe, @jdsmith3000 ?

@ddorwin

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2016

I think the desired behavior is as follows. This doesn't necessarily match what is easiest for implementations (the original intent of this issue), but it does seem the most rational for applications.

  1. Yes, promise resolution before events.
    • This was the outcome of #19 and all methods should be consistent.
    • However, #19 was incompletely implemented, even for the methods it touched. For example, as @mwatson2 notes, load() calls the Update Key Statuses algorithm before resolving the promise, which happens before firing the "message" event.
    • Fixing this is related to/would also fix some of the issues with calling algorithms that fire events from the CDM portion of the algorithms (see #181).
  2. Any attributes that do not have a corresponding event should be updated before the promise.
    • I believe this currently applies to sessionId (important for management of persistent sessions) and expiration.
    • While I don't think we have another choice for these attributes, this does have some downsides:
      • It leads to inconsistencies with other properties, such as keystatuses, unless we extract the updating of those attributes or otherwise find a way to defer firing the events
        • One such option would be to return an event from these algorithms that is be added to a queue to be processed after resolving the promise.
        • Another option would be to break up the two affected algorithms into updating the attribute and firing the event.
      • If we later added events for these attributes (i.e. "expirationchange"), we would be even more inconsistent.

Going forward, we need to be careful to consider the observable behaviors of algorithms, including the firing of events, when deciding where and how to call them.

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 17, 2016

@ddorwin I agree with the approach you outline.

I think we should have a generic solution to the problem of splitting attribute update and event processing to before and after the promise, respectively. Previously, when promise resolution happened immediately, rather than as a queued task, this was easy, since we ran all the algorithms before promise resolution and then the promise resolution would be done before all the queued tasks execute. It seems a step backwards that we would now either have to split up out algorithms or explicitly manage that queue of events.

@ddorwin

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2016

Changed to "needs implementation" since we agree on the approach. I think there is still a question of how. @mwatson2, did you have thoughts on what the "generic solution" might be?

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 24, 2016

The only option I can think of is the suggestion you made to have algorithms that update attributes return events that they would like to be queued when the caller is ready.

This would require us to agree on this pull request first to describe CDM-triggered algorithms as it would be in this "monitor" algorithm that we queued the events returned by the attribute update algorithms.

@ddorwin

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2016

I believe you are referring to these suggestions:

  • One such option would be to return an event from these algorithms that is be added to a queue to be processed after resolving the promise.
  • Another option would be to break up the two affected algorithms into updating the attribute and firing the event.

Both were intended to be implemented within each algorithm (and thus not depend on the "monitor" algorithm, though I think we will probably do that too). I think we'll just have to try to implement this and see which approach makes the most sense.

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 25, 2016

Implementing the first of these without the monitor algorithm will be awkward, because each algorithm will need to handle both the case of being called by another algorithm (return the event to be queued later) and the case of direct invocation by the CDM (queue the event directly).

With the monitor algorithm, we would always return the event and the monitor algorithm or other caller would queue it.

With the second option, we'll need some other section to specify that the CDM triggers execution of both halves of the algorithm and this will be almost equal to the monitor algorithm anyway.

With the second option, we're basically splitting up algorithms into one-liners where the utility of calling it an "algorithm" (vs duplicating the one-liner at the calling site) is limited.

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 26, 2016

Note that the Update Key Statuses algorithm calls Attempt To Resume Playback If Necessary, which, depending on what we do with #129, may cause ready state change events.

Should these, too, come after the promise is resolved ?

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 26, 2016

I think there may be another solution: after completing asynchronous work involving the CDM, rather than updating attributes, queuing events and resolving the promise in the same asynchronous 'thread' we could queue a task to perform those things (in that order, in the main thread).

The promise resolution is queued as a microtask, so it will be executed before the events even though we queue it after we queue the events.

Additionally, this approach means the attribute updates are atomic as far as the main script is concerned (it is not clear to me exactly what "execute the following steps in parallel" means, but I have been assuming they are essentially on a separate thread executing in parallel with the main event loop. So changes made in this asynchronous section could become visible mid-task as far as the main event loop is concerned.)

I'll take a shot at implementing this if people agree.

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 26, 2016

In fact, queueing both microtasks and normal tasks from an "in parallel" block seems risky, since execution order is not deterministic unless you queue all the microtasks first (A task queued before a microtask is queued may be executed before the microtask if the event loop happens to be free during that interval, but will otherwise be executed after the microtask).

mwatson2 added a commit to mwatson2/encrypted-media that referenced this issue May 26, 2016

mwatson2 added a commit to mwatson2/encrypted-media that referenced this issue May 26, 2016

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 26, 2016

I created Pull Request #203 for this.

@mwatson2 mwatson2 self-assigned this May 26, 2016

@ddorwin

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2016

Thoughts on the last few comments from @mwatson2:

I think any readyState changes and related events should be fired after the update() promise is resolved. In theory, attempting to resume could happen "in parallel," but that would make the spec more complicated. Implementations can still make such an optimization as long as the observable behavior is consistent.

I believe "execute the following steps in parallel" means cause this work to be performed, but continue executing the following steps asynchronously and do not block the main thread for them.

I also believe that all application-observable behavior must occur on the main thread - thus, any attribute updates and event firing should probably be done in a queued task. It sounds like that task may (or not) be performed before the promise is resolved. That's problematic since we wanted the attributes to be updated before the promise and the events to be fired after. Does that mean we need to queue a task to update the attributes and resolve the promise?

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 27, 2016

Does that mean we need to queue a task to update the attributes and resolve the promise?

That is indeed exactly what my Pull Request does.

I also queue the events in that same task. It actually doesn't matter what order these three things are done in that task (update attributes, resolve promise, queue events), the attributes update immediately, the promise handlers are executed in micro tasks after the task is done and the event handlers are executed in tasks after that.

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 27, 2016

Here is an htmldiff for the proposed changes.

mwatson2 added a commit that referenced this issue May 27, 2016

Issue #14: Update object, queue tasks and resolve promises in main ev…
…ent loop (#203)

* Issue #14: Update object state and queue tasks and resolve promises in main event loop

* Issue #14: Move session closed algorithm calls out of async section in update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.