search

w3c / encrypted-media

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

Fix #365: Do not allow operations after close() is called but before the session is actually closed #421

Closed ddorwin closed 7 years ago

ddorwin commented 7 years ago

Introduces a new "closed" value and use it when determining whether to abort MediaKeySession algorithms.

Also, make media-key-session-closed depend on an explicit state. Previously, this definition depended on whether an algorithm had been run, which is slightly ambiguous.

ddorwin commented 7 years ago

The first commit fixes the race condition identified in #365. We might want to rename the value, though since this value's state does not match the definition of "closed," which is defined at #media-key-session-closed. There is also a closed attribute. Because of the various paths to setting the new closed value, I'm having trouble finding a better name. It is really "closed to new calls" or "closed or pending close." Suggestions welcome, or we can merge this and rename it later.

The second commit is minor cleanup, which should make other algorithms more deterministic and unambiguous. It is unrelated to the race condition in #365.

mwatson2 commented 7 years ago

This LGTM. Should there be a check at the top of the Session Closed algorithm that checks whether the closed promise is already resolved ? This would handle the case where two tasks to run Session Closed are queued (one by the Monitor CDM algorithm and one by close()).

Perhaps the new variable should be called closing ? Or to be strictly correct closing-or-closed ?

ddorwin commented 7 years ago

I made the changes @mwatson2 suggested.

mwatson2 commented 7 years ago

Looks good. Should closing or closed be hyphenated, though (closing-or-closed). I'm concerned that the italicization of "or" is the only thing that indicates this is one variable not two (closing or closed).

ddorwin commented 7 years ago

I agree that would be clearer, but that would be inconsistent with all other such values (search for "value be" in the spec). This is really a style problem and should maybe be addressed with CSS.