Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2342953002:
Update EME errors to use TypeError (Closed)
|
Created:
4 years, 3 months ago by jrummell Modified:
4 years, 2 months ago CC:
chromium-reviews, mlamouri+watch-blink_chromium.org, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, blink-reviews, Srirama, blink-reviews-api_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate EME errors to use TypeError
The current version of the EME spec has specified that TypeErrors
should be returned in certain cases. Update the EME code to use
TypeError where needed.
As well, verify that MediaKeySession is not callable after close().
BUG=646902, 655095
TEST=EME tests pass
Committed: https://crrev.com/8446ebfbbd8b1442d41a3109d08f347fb9a6a060
Cr-Commit-Position: refs/heads/master@{#426032}
Patch Set 1 #
Total comments: 12
Patch Set 2 : V8ThrowException #
Total comments: 6
Patch Set 3 : comments only #
Total comments: 1
Patch Set 4 : rebase on promise changes #
Total comments: 6
Patch Set 5 : createDOMException #
Total comments: 14
Patch Set 6 : changes #Messages
Total messages: 49 (16 generated)
jrummell@chromium.org changed reviewers: + xhwang@chromium.org
PTAL. cc ddorwin@ since it may affect the W3C tests.
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
I only skimmed the code. We should be able to eliminate some of the exceptions and handle them at lower levels. Fixing setServerCertificate may be a separate CL. https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:25: case WebContentDecryptionModuleExceptionInvalidAccessError: Is this still used? Maybe treat it as a TypeError per the bug and make progress towards deprecation (really, that should be caught at a lower level). https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:29: case WebContentDecryptionModuleExceptionUnknownError: Are these three still used? Remove/NOTREACHED and deprecate? https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:30: return UnknownError; Remove this line since the comment below also applies, though is out of date. https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:105: DCHECK_NE(WebContentDecryptionModuleExceptionTypeError, code); What would happen if we received this? Would the callback not handle it correctly? Should the callback be changed instead? https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/encryptedmedia/MediaKeys.cpp (right): https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/MediaKeys.cpp:182: // (Let the CDM decide whether to support this or not.) This is not spec compliant. The purpose of the spec algorithm steps is that null can be passed to an implementation that does not support server certificates. This allows simplification of some application designs. See https://github.com/w3c/encrypted-media/issues/145. https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebContentDecryptionModuleException.h (right): https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebContentDecryptionModuleException.h:13: WebContentDecryptionModuleExceptionInvalidAccessError, Can we delete four of theses?
haraken@chromium.org changed reviewers: + haraken@chromium.org
Can we avoid using v8::Exception::TypeError directly and use helper methods on V8ThrowException?
Updated. > Can we avoid using v8::Exception::TypeError directly and use helper methods on > V8ThrowException? Done. I just cloned what was done in CryptoResultImpl.cpp, but V8ThrowException was helpful. https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:25: case WebContentDecryptionModuleExceptionInvalidAccessError: On 2016/09/15 22:43:03, ddorwin wrote: > Is this still used? Maybe treat it as a TypeError per the bug and make progress > towards deprecation (really, that should be caught at a lower level). CDM interface has kInvalidAccessError, so it could be returned. Updated WebContentDecryptionModuleException to match the expected list, and updated the CDM_9 interface bug (http://crbug.com/570216) to note that the extra errors should be removed. https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:29: case WebContentDecryptionModuleExceptionUnknownError: On 2016/09/15 22:43:03, ddorwin wrote: > Are these three still used? Remove/NOTREACHED and deprecate? Done. https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:30: return UnknownError; On 2016/09/15 22:43:03, ddorwin wrote: > Remove this line since the comment below also applies, though is out of date. Removed the unnecessary values. https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:105: DCHECK_NE(WebContentDecryptionModuleExceptionTypeError, code); On 2016/09/15 22:43:03, ddorwin wrote: > What would happen if we received this? Would the callback not handle it > correctly? Should the callback be changed instead? Removed now that haraken@ pointed out a better way to do it (TypeError can be handled as a DomException). https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/encryptedmedia/MediaKeys.cpp (right): https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/MediaKeys.cpp:182: // (Let the CDM decide whether to support this or not.) On 2016/09/15 22:43:03, ddorwin wrote: > This is not spec compliant. The purpose of the spec algorithm steps is that null > can be passed to an implementation that does not support server certificates. > This allows simplification of some application designs. See > https://github.com/w3c/encrypted-media/issues/145. Replaced it with a TODO since it's currently not determinable. https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebContentDecryptionModuleException.h (right): https://codereview.chromium.org/2342953002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebContentDecryptionModuleException.h:13: WebContentDecryptionModuleExceptionInvalidAccessError, On 2016/09/15 22:43:03, ddorwin wrote: > Can we delete four of theses? I was going to do it as a separate CL (since there are several versions of these), but it can be done now.
https://codereview.chromium.org/2342953002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2342953002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:104: ScriptState::Scope scope(m_resolver->getScriptState()); You don't need to enter ScriptState::Scope because m_resolver->reject() enters the scope. Also you don't need to check contextIsValid() because m_resolver->reject() checks it. https://codereview.chromium.org/2342953002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/2342953002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:247: ScriptState::Scope scope(getScriptState()); Ditto.
haraken: If I remove the ScriptState::Scope() the renderer crashes. v8::String::NewExternalOneByte(v8::Isolate*, v8::String::ExternalOneByteStringResource*) blink::StringCache::createStringAndInsertIntoCache(v8::Isolate*, WTF::StringImpl*) blink::StringCache::v8ExternalStringSlow(v8::Isolate*, WTF::StringImpl*) blink::V8ThrowException::createTypeError(v8::Isolate*, WTF::String const&) blink::V8ThrowException::createDOMException(v8::Isolate*, int, WTF::String const&, WTF::String const&) I assume this is due to V8ThrowException::createDOMException() trying to create an object for the error message before you get into reject(). So it appears I need to keep at least the scope, and possibly the contextIsValid() test. WDYT?
https://codereview.chromium.org/2342953002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebContentDecryptionModuleException.h (right): https://codereview.chromium.org/2342953002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebContentDecryptionModuleException.h:10: // From http://w3c.github.io/encrypted-media/#exceptions. http*s* please :) https://codereview.chromium.org/2342953002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebContentDecryptionModuleException.h:16: WebContentDecryptionModuleExceptionUnknownError This isn't listed at that URL. Do we still need it for something? If so, comment that with a TODO to remove it.
On 2016/09/17 00:49:41, jrummell wrote: > haraken: If I remove the ScriptState::Scope() the renderer crashes. > > v8::String::NewExternalOneByte(v8::Isolate*, > v8::String::ExternalOneByteStringResource*) > blink::StringCache::createStringAndInsertIntoCache(v8::Isolate*, > WTF::StringImpl*) > blink::StringCache::v8ExternalStringSlow(v8::Isolate*, WTF::StringImpl*) > blink::V8ThrowException::createTypeError(v8::Isolate*, WTF::String const&) > blink::V8ThrowException::createDOMException(v8::Isolate*, int, WTF::String > const&, WTF::String const&) > > I assume this is due to V8ThrowException::createDOMException() trying to create > an object for the error message before you get into reject(). So it appears I > need to keep at least the scope, and possibly the contextIsValid() test. > > WDYT? Can we use m_resolver->reject(DOMException::create(...)) instead? Then you don't need to create a V8 object before calling reject. See how CredentialContainer handles this: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/creden...
That is what the code did before. However, I'm adding TypeError, which is not a standard DomException. V8ThrowException::createDOMException() handles the case by creating a TypeError rather than a DOMException for that case. I could go back to using DOMException::create(), but then I would have to special case the TypeError like I did in PS#1. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... On Mon, Sep 19, 2016 at 6:29 PM, <haraken@chromium.org> wrote: > On 2016/09/17 00:49:41, jrummell wrote: > > haraken: If I remove the ScriptState::Scope() the renderer crashes. > > > > v8::String::NewExternalOneByte(v8::Isolate*, > > v8::String::ExternalOneByteStringResource*) > > blink::StringCache::createStringAndInsertIntoCache(v8::Isolate*, > > WTF::StringImpl*) > > blink::StringCache::v8ExternalStringSlow(v8::Isolate*, WTF::StringImpl*) > > blink::V8ThrowException::createTypeError(v8::Isolate*, WTF::String > const&) > > blink::V8ThrowException::createDOMException(v8::Isolate*, int, > WTF::String > > const&, WTF::String const&) > > > > I assume this is due to V8ThrowException::createDOMException() trying to > create > > an object for the error message before you get into reject(). So it > appears I > > need to keep at least the scope, and possibly the contextIsValid() test. > > > > WDYT? > > Can we use m_resolver->reject(DOMException::create(...)) instead? Then > you don't > need to create a V8 object before calling reject. > > See how CredentialContainer handles this: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/ > credentialmanager/CredentialsContainer.cpp?q=CredentialsContainer.cpp&sq= > package:chromium&dr > > > https://codereview.chromium.org/2342953002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
That is what the code did before. However, I'm adding TypeError, which is not a standard DomException. V8ThrowException::createDOMException() handles the case by creating a TypeError rather than a DOMException for that case. I could go back to using DOMException::create(), but then I would have to special case the TypeError like I did in PS#1. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... On Mon, Sep 19, 2016 at 6:29 PM, <haraken@chromium.org> wrote: > On 2016/09/17 00:49:41, jrummell wrote: > > haraken: If I remove the ScriptState::Scope() the renderer crashes. > > > > v8::String::NewExternalOneByte(v8::Isolate*, > > v8::String::ExternalOneByteStringResource*) > > blink::StringCache::createStringAndInsertIntoCache(v8::Isolate*, > > WTF::StringImpl*) > > blink::StringCache::v8ExternalStringSlow(v8::Isolate*, WTF::StringImpl*) > > blink::V8ThrowException::createTypeError(v8::Isolate*, WTF::String > const&) > > blink::V8ThrowException::createDOMException(v8::Isolate*, int, > WTF::String > > const&, WTF::String const&) > > > > I assume this is due to V8ThrowException::createDOMException() trying to > create > > an object for the error message before you get into reject(). So it > appears I > > need to keep at least the scope, and possibly the contextIsValid() test. > > > > WDYT? > > Can we use m_resolver->reject(DOMException::create(...)) instead? Then > you don't > need to create a V8 object before calling reject. > > See how CredentialContainer handles this: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/ > credentialmanager/CredentialsContainer.cpp?q=CredentialsContainer.cpp&sq= > package:chromium&dr > > > https://codereview.chromium.org/2342953002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Addressed ddorwin's comments. https://codereview.chromium.org/2342953002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebContentDecryptionModuleException.h (right): https://codereview.chromium.org/2342953002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebContentDecryptionModuleException.h:10: // From http://w3c.github.io/encrypted-media/#exceptions. On 2016/09/19 22:33:53, ddorwin wrote: > http*s* please :) Done. https://codereview.chromium.org/2342953002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebContentDecryptionModuleException.h:16: WebContentDecryptionModuleExceptionUnknownError On 2016/09/19 22:33:53, ddorwin wrote: > This isn't listed at that URL. Do we still need it for something? If so, comment > that with a TODO to remove it. Yes. Added comment.
The CQ bit was checked by jrummell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/20 01:38:46, jrummell wrote: > That is what the code did before. However, I'm adding TypeError, which is > not a standard DomException. V8ThrowException::createDOMException() handles > the case by creating a TypeError rather than a DOMException for that case. > > I could go back to using DOMException::create(), but then I would have to > special case the TypeError like I did in PS#1. Ah, sorry, I was lacking the context. Have you discussed the behavior in the spec community? It looks strange that DOM raises a TypeError (Note that TypeError is a JS error).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/20 02:41:44, haraken wrote: > On 2016/09/20 01:38:46, jrummell wrote: > > That is what the code did before. However, I'm adding TypeError, which is > > not a standard DomException. V8ThrowException::createDOMException() handles > > the case by creating a TypeError rather than a DOMException for that case. > > > > I could go back to using DOMException::create(), but then I would have to > > special case the TypeError like I did in PS#1. > > Ah, sorry, I was lacking the context. > > Have you discussed the behavior in the spec community? It looks strange that DOM > raises a TypeError (Note that TypeError is a JS error). This was changed in the spec a while ago. See https://github.com/w3c/encrypted-media/issues/103. Firefox has started to transition.
https://codereview.chromium.org/2342953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2342953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:95: getExecutionContext()->postTask(BLINK_FROM_HERE, createSameThreadTask(&ContentDecryptionModuleResultPromise::rejectInternal, wrapPersistent(this), code, errorMessage)); I begin to think that this postTask is the culprit of the weirdness... - It looks strange that you need to enter a ScriptState::Scope before you create a JS error. You should already be in a correct ScriptState when you create a JS error. - As we discussed before (e.g., https://codereview.chromium.org/1987883002), it is wrong to use postTask to work around the gc-related crash. You should guarantee that the promise is explicitly rejected without relying on GC timings. - If you fix that behavior, you can remove postTask from here. Then you shouldn't need to enter ScriptState::Scope here because you're already in a correct ScriptState. What do you think?
I agree that this code should not be dependent on GC timings. Currently the promise can be rejected for a variety of reasons: - the request actually failed (normal case). - the CDM crashed, so all outstanding async work is cancelled. - GC has removed all blink objects that care about the CDM, so it gets destroyed, and any remaining async work is cancelled. So ideally the blink code would automatically handle it. But I've been unable to find any simple way to do that. Any suggestions? We used PostTask() as reject() may happen due to GC, and we needed to avoid that. Are you aware of other blink code that uses promises and cancels outstanding async operations properly? I found rejectWithTypeError() in CryptoResultImpl.cpp that I based this on, so it appears they have a similar issue. On 2016/09/20 05:22:32, haraken wrote: > https://codereview.chromium.org/2342953002/diff/40001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp > (right): > > https://codereview.chromium.org/2342953002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:95: > getExecutionContext()->postTask(BLINK_FROM_HERE, > createSameThreadTask(&ContentDecryptionModuleResultPromise::rejectInternal, > wrapPersistent(this), code, errorMessage)); > > I begin to think that this postTask is the culprit of the weirdness... > > - It looks strange that you need to enter a ScriptState::Scope before you create > a JS error. You should already be in a correct ScriptState when you create a JS > error. > > - As we discussed before (e.g., https://codereview.chromium.org/1987883002), it > is wrong to use postTask to work around the gc-related crash. You should > guarantee that the promise is explicitly rejected without relying on GC timings. > > - If you fix that behavior, you can remove postTask from here. Then you > shouldn't need to enter ScriptState::Scope here because you're already in a > correct ScriptState. > > What do you think?
haraken@chromium.org changed reviewers: + yhirano@chromium.org
On 2016/09/21 00:43:03, jrummell wrote: > I agree that this code should not be dependent on GC timings. Currently the > promise can be rejected for a variety of reasons: > - the request actually failed (normal case). > - the CDM crashed, so all outstanding async work is cancelled. > - GC has removed all blink objects that care about the CDM, so it gets > destroyed, and any remaining async work is cancelled. The first & second ones are fine. The third one is problematic. Promises must have an explicit point when they are resolved/rejected. Otherwise, it will expose GC's behavior to the web, which is clearly wrong. > > So ideally the blink code would automatically handle it. But I've been unable to > find any simple way to do that. > > Any suggestions? We used PostTask() as reject() may happen due to GC, and we > needed to avoid that. Are you aware of other blink code that uses promises and > cancels outstanding async operations properly? I found rejectWithTypeError() in > CryptoResultImpl.cpp that I based this on, so it appears they have a similar > issue. Would you elaborate on how CryptoResultImpl has the same issue? It looks like that their promises are rejected at explicit timings. > > > On 2016/09/20 05:22:32, haraken wrote: > > > https://codereview.chromium.org/2342953002/diff/40001/third_party/WebKit/Sour... > > File > > > third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp > > (right): > > > > > https://codereview.chromium.org/2342953002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:95: > > getExecutionContext()->postTask(BLINK_FROM_HERE, > > createSameThreadTask(&ContentDecryptionModuleResultPromise::rejectInternal, > > wrapPersistent(this), code, errorMessage)); > > > > I begin to think that this postTask is the culprit of the weirdness... > > > > - It looks strange that you need to enter a ScriptState::Scope before you > create > > a JS error. You should already be in a correct ScriptState when you create a > JS > > error. > > > > - As we discussed before (e.g., https://codereview.chromium.org/1987883002), > it > > is wrong to use postTask to work around the gc-related crash. You should > > guarantee that the promise is explicitly rejected without relying on GC > timings. > > > > - If you fix that behavior, you can remove postTask from here. Then you > > shouldn't need to enter ScriptState::Scope here because you're already in a > > correct ScriptState. > > > > What do you think?
On 2016/09/21 02:32:47, haraken wrote: > On 2016/09/21 00:43:03, jrummell wrote: > > I agree that this code should not be dependent on GC timings. Currently the > > promise can be rejected for a variety of reasons: > > - the request actually failed (normal case). > > - the CDM crashed, so all outstanding async work is cancelled. > > - GC has removed all blink objects that care about the CDM, so it gets > > destroyed, and any remaining async work is cancelled. > > The first & second ones are fine. The third one is problematic. Promises must > have an explicit point when they are resolved/rejected. Otherwise, it will > expose GC's behavior to the web, which is clearly wrong. > Just FYI, the fetch spec has a description[1] how to handle such a situation. Does the CDM spec have a similar specification? 1: https://fetch.spec.whatwg.org/#garbage-collection
On 2016/09/23 04:30:27, yhirano wrote: > On 2016/09/21 02:32:47, haraken wrote: > > On 2016/09/21 00:43:03, jrummell wrote: > > > I agree that this code should not be dependent on GC timings. Currently the > > > promise can be rejected for a variety of reasons: > > > - the request actually failed (normal case). > > > - the CDM crashed, so all outstanding async work is cancelled. > > > - GC has removed all blink objects that care about the CDM, so it gets > > > destroyed, and any remaining async work is cancelled. > > > > The first & second ones are fine. The third one is problematic. Promises must > > have an explicit point when they are resolved/rejected. Otherwise, it will > > expose GC's behavior to the web, which is clearly wrong. > > > Just FYI, the fetch spec has a description[1] how to handle such a situation. > Does the CDM spec have a similar specification? > > 1: https://fetch.spec.whatwg.org/#garbage-collection The EME spec makes no reference to garbage collection. ddorwin@ is out, but I don't think there has been any intention to mention GC in the spec. I'm curious why there is a concern whether GC is observable or not. What are we trying to avoid? I would think that once you don't have a reference to some object anymore, you would expect any outstanding promises for async work on the object would fail somehow. I looked at the fetch spec. How does the UA know whether or not the termination is visible? From a quick look at the fetch code, it appears they just drop the promise if the object goes away (at least they don't do anything special in ~FetchManager::Loader() with the ScriptPromiseResolver or checking if it was resolved/rejected). I suspect the majority of crashes we saw on the original bug were clients simply calling close() and not caring about the promise result since the page was going away. However, as far as the blink code is concerned we returned a promise, and the goal is to resolve/reject it some time later. In fact, we have DCHECKs in the promise destructors [1] to make sure they are always resolved/rejected. Currently we have MediaKeys and MediaKeySession objects that ultimately reference a CdmSessionAdapter (ref-counted). When the blink objects (MK/MKS) are gone, CdmSessionAdapter goes away (and takes the CDM with it), so any promises that are still outstanding are rejected. So to avoid this it appears we need to avoid having GC free MK/MKS as long as a (visible?) promise is outstanding. Would having all the blink promises have a Member<> to the appropriate MK/MKS "fix" this problem? I would think that if both the promise and the MK/MKS are unreachable we would still have a problem with reject() happening during GC, but at least it shouldn't be visible. [1] https://cs.chromium.org/chromium/src/media/base/cdm_promise.h?l=106
On 2016/09/23 20:56:39, jrummell wrote: > On 2016/09/23 04:30:27, yhirano wrote: > > On 2016/09/21 02:32:47, haraken wrote: > > > On 2016/09/21 00:43:03, jrummell wrote: > > > > I agree that this code should not be dependent on GC timings. Currently > the > > > > promise can be rejected for a variety of reasons: > > > > - the request actually failed (normal case). > > > > - the CDM crashed, so all outstanding async work is cancelled. > > > > - GC has removed all blink objects that care about the CDM, so it gets > > > > destroyed, and any remaining async work is cancelled. > > > > > > The first & second ones are fine. The third one is problematic. Promises > must > > > have an explicit point when they are resolved/rejected. Otherwise, it will > > > expose GC's behavior to the web, which is clearly wrong. > > > > > Just FYI, the fetch spec has a description[1] how to handle such a situation. > > Does the CDM spec have a similar specification? > > > > 1: https://fetch.spec.whatwg.org/#garbage-collection > > The EME spec makes no reference to garbage collection. ddorwin@ is out, but I > don't think there has been any intention to mention GC in the spec. > > I'm curious why there is a concern whether GC is observable or not. What are we > trying to avoid? I would think that once you don't have a reference to some > object anymore, you would expect any outstanding promises for async work on the > object would fail somehow. There's not guarantee about the timing when GC is triggered (it may not happen). If we implement something that depends on GC timings, it will start showing an undeterministic behavior. That's what we should avoid (unless the spec says it's okay that reject() may not happen). > > I looked at the fetch spec. How does the UA know whether or not the termination > is visible? From a quick look at the fetch code, it appears they just drop the > promise if the object goes away (at least they don't do anything special in > ~FetchManager::Loader() with the ScriptPromiseResolver or checking if it was > resolved/rejected). > > I suspect the majority of crashes we saw on the original bug were clients simply > calling close() and not caring about the promise result since the page was going > away. However, as far as the blink code is concerned we returned a promise, and > the goal is to resolve/reject it some time later. In fact, we have DCHECKs in > the promise destructors [1] to make sure they are always resolved/rejected. > > Currently we have MediaKeys and MediaKeySession objects that ultimately > reference a CdmSessionAdapter (ref-counted). When the blink objects (MK/MKS) are > gone, CdmSessionAdapter goes away (and takes the CDM with it), so any promises > that are still outstanding are rejected. So to avoid this it appears we need to > avoid having GC free MK/MKS as long as a (visible?) promise is outstanding. > Would having all the blink promises have a Member<> to the appropriate MK/MKS > "fix" this problem? I would think that if both the promise and the MK/MKS are > unreachable we would still have a problem with reject() happening during GC, but > at least it shouldn't be visible. Won't it leak the MK/MKS? > > [1] https://cs.chromium.org/chromium/src/media/base/cdm_promise.h?l=106
> I'm curious why there is a concern whether GC is observable or not. What are we > trying to avoid? I would think that once you don't have a reference to some > object anymore, you would expect any outstanding promises for async work on the > object would fail somehow. A garbage collector in general can collect an object if that is not observable to anyone in the system. So I am not comfortable with collecting if an object collecting the object is observable through promise callbacks unless formally specified otherwise.
On 2016/09/26 01:18:27, yhirano wrote: > > I'm curious why there is a concern whether GC is observable or not. What are > we > > trying to avoid? I would think that once you don't have a reference to some > > object anymore, you would expect any outstanding promises for async work on > the > > object would fail somehow. > > A garbage collector in general can collect an object if that is not observable > to anyone in the system. So I am not comfortable with collecting if an object > collecting the object is observable through promise callbacks unless formally > specified otherwise. Sorry, I meant "I am not comfortable with collecting an object if collecting the object is ..."
Description was changed from ========== Update EME errors to use TypeError The current version of the EME spec has specified that TypeErrors should be returned in certain cases. Update the EME code to use TypeError where needed. BUG=646902 TEST=EME tests pass ========== to ========== Update EME errors to use TypeError The current version of the EME spec has specified that TypeErrors should be returned in certain cases. Update the EME code to use TypeError where needed. BUG=646902,655095 TEST=EME tests pass ==========
Rebased onto the latest changes (my previous CL changing the promises). With the reformat of blink you're best simply looking at the complete file rather than the diff from PS#3. https://codereview.chromium.org/2342953002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2342953002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:97: ScriptState::Scope scope(m_resolver->getScriptState()); Without this line I get a crash in v8::internal::HandleScope::Extend(v8::internal::Isolate*), so I assume the scope is needed.
https://codereview.chromium.org/2342953002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2342953002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:96: if (code == V8TypeError) { Can you just use V8ThrowException::createDOMException? https://codereview.chromium.org/2342953002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:97: ScriptState::Scope scope(m_resolver->getScriptState()); On 2016/10/14 20:43:47, jrummell wrote: > Without this line I get a crash in > v8::internal::HandleScope::Extend(v8::internal::Isolate*), so I assume the scope > is needed. Yes, this is needed. https://codereview.chromium.org/2342953002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/2342953002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:254: if (code == V8TypeError) { Can you just use V8ThrowException::createDOMException?
Updated to use V8ThrowException::createDOMException() https://codereview.chromium.org/2342953002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2342953002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:96: if (code == V8TypeError) { On 2016/10/15 01:30:46, haraken wrote: > > Can you just use V8ThrowException::createDOMException? Done. https://codereview.chromium.org/2342953002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/2342953002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:254: if (code == V8TypeError) { On 2016/10/15 01:30:46, haraken wrote: > > Can you just use V8ThrowException::createDOMException? Done.
LGTM on my side.
LGTM with nits. I am not familiar with the blink details so I defer that part to blink owners. https://codereview.chromium.org/2342953002/diff/80001/media/blink/cdm_result_... File media/blink/cdm_result_promise_helper.cc (right): https://codereview.chromium.org/2342953002/diff/80001/media/blink/cdm_result_... media/blink/cdm_result_promise_helper.cc:43: // renamed to TYPE_ERROR. http://crbug.com/570216#c11. nit: move this to above the "case"? https://codereview.chromium.org/2342953002/diff/80001/media/blink/cdm_result_... media/blink/cdm_result_promise_helper.cc:52: // http://crbug.com/570216#c11. ditto https://codereview.chromium.org/2342953002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-not-callable-after-close.html (right): https://codereview.chromium.org/2342953002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-not-callable-after-close.html:1: <!DOCTYPE html> This seems unrelated to the "TypeError" change of this CL. Could you please update your CL description about what else is in this CL? https://codereview.chromium.org/2342953002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-not-callable-after-close.html:16: // resolves with the created mediaKeySession. nit: created and closed mediaKeySession? https://codereview.chromium.org/2342953002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-not-callable-after-close.html:37: return navigator.requestMediaKeySystemAccess('org.w3.clearkey', getSimpleConfiguration()).then(function(access) { Can we put rMKSA part of createClosedSession() so we don't need it in every test? https://codereview.chromium.org/2342953002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp (right): https://codereview.chromium.org/2342953002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:85: static bool IsPersistentSessionType(WebEncryptedMediaSessionType sessionType) { Add a comment that this implements "5.1.1 Is persistent session type"? https://w3c.github.io/encrypted-media/#is-persistent-session-type Also OOC, what's the plan for "persistent-usage-record"? https://codereview.chromium.org/2342953002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:718: case PendingAction::GenerateRequest: nit: Not related to this CL. I wonder whether we should wrap the content of each "case" block into a helper function so that we can put the async part directly under the sync part for better readability, e.g. load() { // Load step 1 to 7 } loadTask() { // Load step 8 } actionTimerFired() { ... case Load: loadTask(); }
Description was changed from ========== Update EME errors to use TypeError The current version of the EME spec has specified that TypeErrors should be returned in certain cases. Update the EME code to use TypeError where needed. BUG=646902,655095 TEST=EME tests pass ========== to ========== Update EME errors to use TypeError The current version of the EME spec has specified that TypeErrors should be returned in certain cases. Update the EME code to use TypeError where needed. As well, verify that MediaKeySession is not callable after close(). BUG=646902,655095 TEST=EME tests pass ==========
Thanks for the reviews. https://codereview.chromium.org/2342953002/diff/80001/media/blink/cdm_result_... File media/blink/cdm_result_promise_helper.cc (right): https://codereview.chromium.org/2342953002/diff/80001/media/blink/cdm_result_... media/blink/cdm_result_promise_helper.cc:43: // renamed to TYPE_ERROR. http://crbug.com/570216#c11. On 2016/10/17 23:13:55, xhwang wrote: > nit: move this to above the "case"? Done. https://codereview.chromium.org/2342953002/diff/80001/media/blink/cdm_result_... media/blink/cdm_result_promise_helper.cc:52: // http://crbug.com/570216#c11. On 2016/10/17 23:13:55, xhwang wrote: > ditto Done. https://codereview.chromium.org/2342953002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-not-callable-after-close.html (right): https://codereview.chromium.org/2342953002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-not-callable-after-close.html:1: <!DOCTYPE html> On 2016/10/17 23:13:55, xhwang wrote: > This seems unrelated to the "TypeError" change of this CL. Could you please > update your CL description about what else is in this CL? Done. https://codereview.chromium.org/2342953002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-not-callable-after-close.html:16: // resolves with the created mediaKeySession. On 2016/10/17 23:13:55, xhwang wrote: > nit: created and closed mediaKeySession? Done. https://codereview.chromium.org/2342953002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-not-callable-after-close.html:37: return navigator.requestMediaKeySystemAccess('org.w3.clearkey', getSimpleConfiguration()).then(function(access) { On 2016/10/17 23:13:55, xhwang wrote: > Can we put rMKSA part of createClosedSession() so we don't need it in every > test? Done. https://codereview.chromium.org/2342953002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp (right): https://codereview.chromium.org/2342953002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:85: static bool IsPersistentSessionType(WebEncryptedMediaSessionType sessionType) { On 2016/10/17 23:13:55, xhwang wrote: > Add a comment that this implements "5.1.1 Is persistent session type"? > > https://w3c.github.io/encrypted-media/#is-persistent-session-type > > Also OOC, what's the plan for "persistent-usage-record"? Done. https://codereview.chromium.org/2342953002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:718: case PendingAction::GenerateRequest: On 2016/10/17 23:13:55, xhwang wrote: > nit: Not related to this CL. I wonder whether we should wrap the content of each > "case" block into a helper function so that we can put the async part directly > under the sync part for better readability, e.g. > > load() { > // Load step 1 to 7 > } > > loadTask() { > // Load step 8 > } > > actionTimerFired() { > ... > case Load: > loadTask(); > } Good idea. Next CL.
The CQ bit was checked by jrummell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2342953002/#ps100001 (title: "changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ddorwin@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 ========== Update EME errors to use TypeError The current version of the EME spec has specified that TypeErrors should be returned in certain cases. Update the EME code to use TypeError where needed. As well, verify that MediaKeySession is not callable after close(). BUG=646902,655095 TEST=EME tests pass ========== to ========== Update EME errors to use TypeError The current version of the EME spec has specified that TypeErrors should be returned in certain cases. Update the EME code to use TypeError where needed. As well, verify that MediaKeySession is not callable after close(). BUG=646902,655095 TEST=EME tests pass ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
Description was changed from ========== Update EME errors to use TypeError The current version of the EME spec has specified that TypeErrors should be returned in certain cases. Update the EME code to use TypeError where needed. As well, verify that MediaKeySession is not callable after close(). BUG=646902,655095 TEST=EME tests pass ========== to ========== Update EME errors to use TypeError The current version of the EME spec has specified that TypeErrors should be returned in certain cases. Update the EME code to use TypeError where needed. As well, verify that MediaKeySession is not callable after close(). BUG=646902,655095 TEST=EME tests pass Committed: https://crrev.com/8446ebfbbd8b1442d41a3109d08f347fb9a6a060 Cr-Commit-Position: refs/heads/master@{#426032} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8446ebfbbd8b1442d41a3109d08f347fb9a6a060 Cr-Commit-Position: refs/heads/master@{#426032} |
