search

w3c / encrypted-media

Encrypted Media Extensions
https://w3c.github.io/encrypted-media/
Other
180 stars 80 forks source link

setMediaKeys() step 1 and 2 should be reversed #372

Closed xhwang-chromium closed 7 years ago

xhwang-chromium commented 7 years ago

The first two steps of setMediaKeys() are as follows:

  1. If mediaKeys and the mediaKeys attribute are the same object, return a resolved promise.
  2. If this object's attaching media keys value is true, return a promise rejected with an InvalidStateError.

Now imagine the application makes the following calls:

video.setMediaKeys(mediaKeys);
video.setMediaKeys(null);

On the second call, the promise of the first call could still be pending, e.g. the result is in a task waiting in a queue. So the mediaKeys attribute is still null. Then according to step 1, since both the mediaKeys and mediaKeys attribute are null, in theory they are the same object, a resolved promise is returned. Then, when the task in the queue runs, the promise of the first call is also resolved.

This could confuse the application that even if the promise of the second call is resolved, the mediaKeys attribute is not null.

Should we switch the order of step 1 and step 2, such that we always check the attaching media keys value first, and any setMediaKeys() during another pending setMediaKeys() call will always be rejected?

ddorwin commented 7 years ago

That makes sense. The most important thing is to avoid multiple outstanding calls, and step 1 adds an exception to that rule.

ddorwin commented 7 years ago

Any comments on this suggestion? This seems like a bug, but can we fix it in V1? @plehegar

mavgit commented 7 years ago

This is a complex spec and issues like this are bound to come up. If we can't change V1 (which is understandable) we could open a new V1.1 version of the spec and start making these changes. Alternatively, we could just keep a list of V1.1 desired fixes somewhere, but editing the spec seems clearer.

ddorwin commented 7 years ago

PR #372.

jdsmith3000 commented 7 years ago

This seems like it would only change behaviors when the scenarios like @xhwang-chromium describes occur. I'd lean toward cleaning this up if there's opportunity.