Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2484873002:
EME: Make sure sessions are closed before they are destroyed (Closed)
DescriptionEME: Make sure sessions are closed before they are destroyed
EME spec notes that "the User Agent must use the CDM to close the
session before User Agent state associated with the session is
deleted", so close the session if the session hasn't been closed.
BUG=660750
TEST=EME layout tests pass
Committed: https://crrev.com/266df595a25c40f0ddf6876742149559ff42040b
Cr-Commit-Position: refs/heads/master@{#431701}
Patch Set 1 #
Total comments: 6
Patch Set 2 : comments only #
Total comments: 3
Patch Set 3 : update spec comment #
Total comments: 8
Patch Set 4 : changes #
Messages
Total messages: 19 (6 generated)
jrummell@chromium.org changed reviewers: + xhwang@chromium.org
PTAL.
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
High-level comments only. https://codereview.chromium.org/2484873002/diff/1/media/blink/webcontentdecry... File media/blink/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/2484873002/diff/1/media/blink/webcontentdecry... media/blink/webcontentdecryptionmodulesession_impl.cc:228: // If we need to call close() on destruction, we need a promise that won't "call close()" (and similar wording in the description) are misleading IMO. This is most likely to be interpreted as the MKS.close() JS API, but it is really asking the implementation to close the session. https://codereview.chromium.org/2484873002/diff/1/media/blink/webcontentdecry... media/blink/webcontentdecryptionmodulesession_impl.cc:258: // From http://w3c.github.io/encrypted-media/#mediakeysession-interface This text was just updated. (MediaKeySession destroyed algorithm has been removed and collapsed into the prose.) You'll want to update the description too. https://codereview.chromium.org/2484873002/diff/1/media/blink/webcontentdecry... media/blink/webcontentdecryptionmodulesession_impl.cc:261: // before User Agent state associated with the session is deleted." We don't actually do this last part and can't because the API is asynchronous. We'd need a "prepare to delete" method. That's fine, though, since this is the observable behavior, and there's not actually anything observable. There is a bit of an expectation that the CDM will destroy the keys in memory, but that is not observable.
Description was changed from ========== EME: Make sure close() is called before a session is destroyed EME spec notes that the MediaKeySession destroyed algorithm should be run before the session is deleted, so call close() if the app hasn't. BUG=660750 TEST=EME layout tests pass ========== to ========== EME: Make sure sessions are closed before they are destroyed EME spec notes that "the User Agent must use the CDM to close the session before User Agent state associated with the session is deleted", so close the session if the session hasn't been closed. BUG=660750 TEST=EME layout tests pass ==========
Updated code comments (and CL description). https://codereview.chromium.org/2484873002/diff/1/media/blink/webcontentdecry... File media/blink/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/2484873002/diff/1/media/blink/webcontentdecry... media/blink/webcontentdecryptionmodulesession_impl.cc:228: // If we need to call close() on destruction, we need a promise that won't On 2016/11/07 19:45:30, ddorwin wrote: > "call close()" (and similar wording in the description) are misleading IMO. This > is most likely to be interpreted as the MKS.close() JS API, but it is really > asking the implementation to close the session. Done. https://codereview.chromium.org/2484873002/diff/1/media/blink/webcontentdecry... media/blink/webcontentdecryptionmodulesession_impl.cc:258: // From http://w3c.github.io/encrypted-media/#mediakeysession-interface On 2016/11/07 19:45:30, ddorwin wrote: > This text was just updated. (MediaKeySession destroyed algorithm has been > removed and collapsed into the prose.) You'll want to update the description > too. Done. https://codereview.chromium.org/2484873002/diff/1/media/blink/webcontentdecry... media/blink/webcontentdecryptionmodulesession_impl.cc:261: // before User Agent state associated with the session is deleted." On 2016/11/07 19:45:31, ddorwin wrote: > We don't actually do this last part and can't because the API is asynchronous. > We'd need a "prepare to delete" method. > > That's fine, though, since this is the observable behavior, and there's not > actually anything observable. There is a bit of an expectation that the CDM will > destroy the keys in memory, but that is not observable. Acknowledged.
Thanks. I defer to xhwang on details. https://codereview.chromium.org/2484873002/diff/20001/media/blink/webcontentd... File media/blink/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/2484873002/diff/20001/media/blink/webcontentd... media/blink/webcontentdecryptionmodulesession_impl.cc:262: // before User Agent state associated with the session is deleted." You might note that this will happen after, but the visible result is the same. I'm not quite sure why that is how it's spec'd.
https://codereview.chromium.org/2484873002/diff/20001/media/blink/webcontentd... File media/blink/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/2484873002/diff/20001/media/blink/webcontentd... media/blink/webcontentdecryptionmodulesession_impl.cc:262: // before User Agent state associated with the session is deleted." On 2016/11/08 00:41:13, ddorwin wrote: > You might note that this will happen after, but the visible result is the same. > > I'm not quite sure why that is how it's spec'd. Filed https://github.com/w3c/encrypted-media/issues/358 to remove this text.
On 2016/11/08 01:05:20, ddorwin wrote: > https://codereview.chromium.org/2484873002/diff/20001/media/blink/webcontentd... > File media/blink/webcontentdecryptionmodulesession_impl.cc (right): > > https://codereview.chromium.org/2484873002/diff/20001/media/blink/webcontentd... > media/blink/webcontentdecryptionmodulesession_impl.cc:262: // before User Agent > state associated with the session is deleted." > On 2016/11/08 00:41:13, ddorwin wrote: > > You might note that this will happen after, but the visible result is the > same. > > > > I'm not quite sure why that is how it's spec'd. > > Filed https://github.com/w3c/encrypted-media/issues/358 to remove this text. The spec has been updated. Please update the comments.
ddorwin@: Thanks for updating the spec. https://codereview.chromium.org/2484873002/diff/20001/media/blink/webcontentd... File media/blink/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/2484873002/diff/20001/media/blink/webcontentd... media/blink/webcontentdecryptionmodulesession_impl.cc:262: // before User Agent state associated with the session is deleted." On 2016/11/08 01:05:20, ddorwin wrote: > On 2016/11/08 00:41:13, ddorwin wrote: > > You might note that this will happen after, but the visible result is the > same. > > > > I'm not quite sure why that is how it's spec'd. > > Filed https://github.com/w3c/encrypted-media/issues/358 to remove this text. Updated comment copied from spec.
Thanks! I just have a few comments/nits. https://codereview.chromium.org/2484873002/diff/40001/media/blink/webcontentd... File media/blink/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/2484873002/diff/40001/media/blink/webcontentd... media/blink/webcontentdecryptionmodulesession_impl.cc:231: class IgnoreResponsePromise : public SimpleCdmPromise { move this to an anonymous namespace, maybe together with all the static functions above. https://codereview.chromium.org/2484873002/diff/40001/media/blink/webcontentd... media/blink/webcontentdecryptionmodulesession_impl.cc:270: if (!is_closed_ && !has_close_been_called_) { There's a corner case where the CDM may self-close the session but |this| is not notified yet. CDMs should handle this case. But this seems a general issue and not related to this CL. https://codereview.chromium.org/2484873002/diff/40001/media/blink/webcontentd... media/blink/webcontentdecryptionmodulesession_impl.cc:434: DCHECK(thread_checker_.CalledOnValidThread()); DCHECK(!has_close_been_called_) since we can't close() twice? Or can we? Also, can we call close() if |is_closed_| is true? https://codereview.chromium.org/2484873002/diff/40001/media/blink/webcontentd... File media/blink/webcontentdecryptionmodulesession_impl.h (right): https://codereview.chromium.org/2484873002/diff/40001/media/blink/webcontentd... media/blink/webcontentdecryptionmodulesession_impl.h:81: bool is_closed_; It seems possible that is_closed_ is true but has_close_been_called_ is false, e.g. when the CDM closed the session due to resource lost etc: The CDM may close a session at any point, such as when the session is no longer needed or when system resources are lost. https://w3c.github.io/encrypted-media/#session-closed We should document it clearly here since it's a bit tricky.
Updated. https://codereview.chromium.org/2484873002/diff/40001/media/blink/webcontentd... File media/blink/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/2484873002/diff/40001/media/blink/webcontentd... media/blink/webcontentdecryptionmodulesession_impl.cc:231: class IgnoreResponsePromise : public SimpleCdmPromise { On 2016/11/11 18:52:46, xhwang wrote: > move this to an anonymous namespace, maybe together with all the static > functions above. Done. https://codereview.chromium.org/2484873002/diff/40001/media/blink/webcontentd... media/blink/webcontentdecryptionmodulesession_impl.cc:270: if (!is_closed_ && !has_close_been_called_) { On 2016/11/11 18:52:46, xhwang wrote: > There's a corner case where the CDM may self-close the session but |this| is not > notified yet. CDMs should handle this case. But this seems a general issue and > not related to this CL. Acknowledged. https://codereview.chromium.org/2484873002/diff/40001/media/blink/webcontentd... media/blink/webcontentdecryptionmodulesession_impl.cc:434: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/11/11 18:52:46, xhwang wrote: > DCHECK(!has_close_been_called_) since we can't close() twice? Or can we? > > Also, can we call close() if |is_closed_| is true? Done. We shouldn't call close() if |is_closed_| is true, but since close() is async there is a small window where close() was called just before the closed event arrives. https://codereview.chromium.org/2484873002/diff/40001/media/blink/webcontentd... File media/blink/webcontentdecryptionmodulesession_impl.h (right): https://codereview.chromium.org/2484873002/diff/40001/media/blink/webcontentd... media/blink/webcontentdecryptionmodulesession_impl.h:81: bool is_closed_; On 2016/11/11 18:52:46, xhwang wrote: > It seems possible that is_closed_ is true but has_close_been_called_ is false, > e.g. when the CDM closed the session due to resource lost etc: > > The CDM may close a session at any point, such as when the session is no longer > needed or when system resources are lost. > > https://w3c.github.io/encrypted-media/#session-closed > > We should document it clearly here since it's a bit tricky. Done.
lgtm
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== EME: Make sure sessions are closed before they are destroyed EME spec notes that "the User Agent must use the CDM to close the session before User Agent state associated with the session is deleted", so close the session if the session hasn't been closed. BUG=660750 TEST=EME layout tests pass ========== to ========== EME: Make sure sessions are closed before they are destroyed EME spec notes that "the User Agent must use the CDM to close the session before User Agent state associated with the session is deleted", so close the session if the session hasn't been closed. BUG=660750 TEST=EME layout tests pass ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== EME: Make sure sessions are closed before they are destroyed EME spec notes that "the User Agent must use the CDM to close the session before User Agent state associated with the session is deleted", so close the session if the session hasn't been closed. BUG=660750 TEST=EME layout tests pass ========== to ========== EME: Make sure sessions are closed before they are destroyed EME spec notes that "the User Agent must use the CDM to close the session before User Agent state associated with the session is deleted", so close the session if the session hasn't been closed. BUG=660750 TEST=EME layout tests pass Committed: https://crrev.com/266df595a25c40f0ddf6876742149559ff42040b Cr-Commit-Position: refs/heads/master@{#431701} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/266df595a25c40f0ddf6876742149559ff42040b Cr-Commit-Position: refs/heads/master@{#431701} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
