Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2407013002:
EME: Improve promise lifetime (Closed)
DescriptionEME: Improve promise lifetime
Each promise generated by EME now keeps a link to the object that
initiated it so that garbage collection doesn't clean up EME objects
while there are promises outstanding.
Also don't resolve/reject a promise if the ExecutionContext
has been (or is in the process of being) destroyed.
BUG=654556
TEST=EME tests pass (plus manual test with changes to not resolve a promise)
Committed: https://crrev.com/b2d6c9f4a63e717cb37d8963a4566e70c3bef0f5
Cr-Commit-Position: refs/heads/master@{#425277}
Patch Set 1 #
Total comments: 1
Patch Set 2 : refactor check #
Total comments: 4
Patch Set 3 : remove m_contextDestroyed #
Total comments: 22
Patch Set 4 : changes #
Total comments: 2
Patch Set 5 : remove ContextLifecycleObserver #
Total comments: 2
Patch Set 6 : more checks #Messages
Total messages: 30 (8 generated)
jrummell@chromium.org changed reviewers: + haraken@chromium.org, xhwang@chromium.org, yhirano@chromium.org
Changes based on our discussion last week. I've included a question below, in case there is a better way to do it. https://codereview.chromium.org/2407013002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h (right): https://codereview.chromium.org/2407013002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:57: getExecutionContext()->activeDOMObjectsAreStopped()) { I included activeDOMObjectsAreStopped() since contextDestroyed() may not be called before it is called on MediaKeys/MediaKeySession. This appears to be the way to determine if blink is in the process of calling contextDestroyed() on all affected objects. Let me know if there's a better way to do this.
Description was changed from ========== EME: Improve promise lifetime Each promise generated by EME now keeps a link to the object that initiated it so that garbage collection doesn't clean up EME objects while there are promises outstanding. Also don't resolve/reject a promise if the ExecutionContext has been (or is in the process of being) destroyed. TEST=EME tests pass (plus manual test with changes to not resolve a promise) BUG= ========== to ========== EME: Improve promise lifetime Each promise generated by EME now keeps a link to the object that initiated it so that garbage collection doesn't clean up EME objects while there are promises outstanding. Also don't resolve/reject a promise if the ExecutionContext has been (or is in the process of being) destroyed. BUG=654556 TEST=EME tests pass (plus manual test with changes to not resolve a promise) ==========
https://codereview.chromium.org/2407013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:105: if (m_contextDestroyed || You can replace m_contextDestroyed with getExecutionContext(). https://codereview.chromium.org/2407013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:107: m_resolver.clear(); Why do you need to clear the promise when isValidToFulfillPromise() gets called? Alternately, shouldn't we clear the promise when contextDestroyed() gets called?
Updated to remove m_contextDestroyed. https://codereview.chromium.org/2407013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:105: if (m_contextDestroyed || On 2016/10/11 03:51:24, haraken wrote: > > You can replace m_contextDestroyed with getExecutionContext(). Done. https://codereview.chromium.org/2407013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:107: m_resolver.clear(); On 2016/10/11 03:51:24, haraken wrote: > > Why do you need to clear the promise when isValidToFulfillPromise() gets called? > Alternately, shouldn't we clear the promise when contextDestroyed() gets called? Done. m_resolver was cleared to release the Member<> as soon as we were done with it (and to ensure we don't call resolve/reject a second time). Much better to do it in contextDestroyed().
https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:105: !getExecutionContext()->activeDOMObjectsAreStopped(); Alternately, can we just check m_resolver?
> Also don't resolve/reject a promise if the ExecutionContext has been (or is in the process of being) destroyed. This is not needed, because ScriptPromiseResolver ignores such resolve/reject calls.
Comments only >> Also don't resolve/reject a promise if the ExecutionContext has been (or is in >> the process of being) destroyed. > This is not needed, because ScriptPromiseResolver ignores such resolve/reject > calls. I added this since there are several places where an object is created to resolve/reject the promise. No need to create the object if it's not going to be used. (examples: MediaKeys object in MediaKeySystemAccess.cpp, my next CL to create TypeErrors). And http://crbug.com/597355 had the crash from reject(DOMException::create(code, errorMessage)), so even though ScriptPromiseResolver handles it, we still can't create an object if we're in this state, and that happens before we get to the ScriptPromiseResolver. https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:105: !getExecutionContext()->activeDOMObjectsAreStopped(); On 2016/10/12 00:58:22, haraken wrote: > > Alternately, can we just check m_resolver? My concern is the order that objects are gc'd. When the context is destroyed other objects may do cleanup first, and that might trigger a promise being rejected. Since this contextDestroyed() hasn't been called yet, it will try to allocate an object (which will cause problems). This could be m_resolver && !activeDOMObjectsAreStopped(), but I'm not sure that's any clearer.
https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:105: !getExecutionContext()->activeDOMObjectsAreStopped(); On 2016/10/12 17:59:14, jrummell wrote: > On 2016/10/12 00:58:22, haraken wrote: > > > > Alternately, can we just check m_resolver? > > My concern is the order that objects are gc'd. When the context is destroyed > other objects may do cleanup first, and that might trigger a promise being > rejected. Since this contextDestroyed() hasn't been called yet, it will try to > allocate an object (which will cause problems). > > This could be m_resolver && !activeDOMObjectsAreStopped(), but I'm not sure > that's any clearer. What are the "other objects"? The other objects are responsible for clearing ContentDecryptionModuleResultPromise's m_resolver when the other objects' contextDestroyed() get called. You can add: void ContentDecryptionModuleResultPromise::clear() { m_resolver.clear(); } and call clear() on contextDestroyed() of all objects that may use the m_resolver.
https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:105: !getExecutionContext()->activeDOMObjectsAreStopped(); On 2016/10/13 02:06:47, haraken wrote: > On 2016/10/12 17:59:14, jrummell wrote: > > On 2016/10/12 00:58:22, haraken wrote: > > > > > > Alternately, can we just check m_resolver? > > > > My concern is the order that objects are gc'd. When the context is destroyed > > other objects may do cleanup first, and that might trigger a promise being > > rejected. Since this contextDestroyed() hasn't been called yet, it will try to > > allocate an object (which will cause problems). > > > > This could be m_resolver && !activeDOMObjectsAreStopped(), but I'm not sure > > that's any clearer. > > What are the "other objects"? The other objects are responsible for clearing > ContentDecryptionModuleResultPromise's m_resolver when the other objects' > contextDestroyed() get called. > > You can add: > > void ContentDecryptionModuleResultPromise::clear() { > m_resolver.clear(); > } > > and call clear() on contextDestroyed() of all objects that may use the > m_resolver. > Currently the objects (MediaKeys and MediaKeySession) that create the promises don't keep track of them (as the promise gets passed to Chromium, and that code keeps the reference to this object). I'd prefer to not do that, as that means the objects would refer to each other, and it introduces a whole bunch of dependencies. With this CL the promises keep MK/MKS alive, until the whole context goes away and then everything gets destroyed (in some order that may not clear the promise object first). These promises can also be generated from navigator.requestMediaKeySystemAccess(), which is static. The crash we're currently seeing (in the linked bug) comes from that promise, although I suspect that the current changes will prevent the crash.
LGTM https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:105: !getExecutionContext()->activeDOMObjectsAreStopped(); On 2016/10/13 07:02:23, jrummell wrote: > On 2016/10/13 02:06:47, haraken wrote: > > On 2016/10/12 17:59:14, jrummell wrote: > > > On 2016/10/12 00:58:22, haraken wrote: > > > > > > > > Alternately, can we just check m_resolver? > > > > > > My concern is the order that objects are gc'd. When the context is destroyed > > > other objects may do cleanup first, and that might trigger a promise being > > > rejected. Since this contextDestroyed() hasn't been called yet, it will try > to > > > allocate an object (which will cause problems). > > > > > > This could be m_resolver && !activeDOMObjectsAreStopped(), but I'm not sure > > > that's any clearer. > > > > What are the "other objects"? The other objects are responsible for clearing > > ContentDecryptionModuleResultPromise's m_resolver when the other objects' > > contextDestroyed() get called. > > > > You can add: > > > > void ContentDecryptionModuleResultPromise::clear() { > > m_resolver.clear(); > > } > > > > and call clear() on contextDestroyed() of all objects that may use the > > m_resolver. > > > > Currently the objects (MediaKeys and MediaKeySession) that create the promises > don't keep track of them (as the promise gets passed to Chromium, and that code > keeps the reference to this object). I'd prefer to not do that, as that means > the objects would refer to each other, and it introduces a whole bunch of > dependencies. With this CL the promises keep MK/MKS alive, until the whole > context goes away and then everything gets destroyed (in some order that may not > clear the promise object first). > > These promises can also be generated from > navigator.requestMediaKeySystemAccess(), which is static. The crash we're > currently seeing (in the linked bug) comes from that promise, although I suspect > that the current changes will prevent the crash. Fair enough. Then let's add a comment about the destruction dependency.
https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:86: m_resolver.clear(); Is this needed? ScriptPromiseResolver is a ContextLifecycleObserver and it clears its internal members in its contextDestroyed, so it will not retain any object after that.
looking good though I don't fully understand blink code :) Just a few nits from my side. https://chromiumcodereview.appspot.com/2407013002/diff/40001/third_party/WebK... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h (right): https://chromiumcodereview.appspot.com/2407013002/diff/40001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:64: // actually reject the promise later on. Update this comment. https://chromiumcodereview.appspot.com/2407013002/diff/40001/third_party/WebK... File third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp (right): https://chromiumcodereview.appspot.com/2407013002/diff/40001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:270: return; We are checking this in both the sub class and base class. Shall we just check in one place (e.g. in the base class) to avoid confusion? Or if all subclasses should check, can we DCHECK in the base class? https://chromiumcodereview.appspot.com/2407013002/diff/40001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:312: void complete() override { resolve(); } Just FWIW, we are not checking isValidToFulfillPromise() here, but as commented above, we don't need to. https://chromiumcodereview.appspot.com/2407013002/diff/40001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:320: Member<MediaKeySession> m_session; Add a comment about why we need this here. https://chromiumcodereview.appspot.com/2407013002/diff/40001/third_party/WebK... File third_party/WebKit/Source/modules/encryptedmedia/MediaKeySystemAccess.cpp (right): https://chromiumcodereview.appspot.com/2407013002/diff/40001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:48: // NOTE: Continued from step 2.8 of createMediaKeys(). empty line here since this is not specific to l.49-50? https://chromiumcodereview.appspot.com/2407013002/diff/40001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:50: return; ditto
Updated. https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:86: m_resolver.clear(); On 2016/10/13 13:35:47, yhirano wrote: > Is this needed? ScriptPromiseResolver is a ContextLifecycleObserver and it > clears its internal members in its contextDestroyed, so it will not retain any > object after that. If you think it's not needed I'm happy to remove it. I assumed that clearing Member<>'s was a good thing to do, but if it doesn't make a different then we can skip it. https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:105: !getExecutionContext()->activeDOMObjectsAreStopped(); On 2016/10/13 07:58:29, haraken wrote: > On 2016/10/13 07:02:23, jrummell wrote: > > On 2016/10/13 02:06:47, haraken wrote: > > > On 2016/10/12 17:59:14, jrummell wrote: > > > > On 2016/10/12 00:58:22, haraken wrote: > > > > > > > > > > Alternately, can we just check m_resolver? > > > > > > > > My concern is the order that objects are gc'd. When the context is > destroyed > > > > other objects may do cleanup first, and that might trigger a promise being > > > > rejected. Since this contextDestroyed() hasn't been called yet, it will > try > > to > > > > allocate an object (which will cause problems). > > > > > > > > This could be m_resolver && !activeDOMObjectsAreStopped(), but I'm not > sure > > > > that's any clearer. > > > > > > What are the "other objects"? The other objects are responsible for clearing > > > ContentDecryptionModuleResultPromise's m_resolver when the other objects' > > > contextDestroyed() get called. > > > > > > You can add: > > > > > > void ContentDecryptionModuleResultPromise::clear() { > > > m_resolver.clear(); > > > } > > > > > > and call clear() on contextDestroyed() of all objects that may use the > > > m_resolver. > > > > > > > Currently the objects (MediaKeys and MediaKeySession) that create the promises > > don't keep track of them (as the promise gets passed to Chromium, and that > code > > keeps the reference to this object). I'd prefer to not do that, as that means > > the objects would refer to each other, and it introduces a whole bunch of > > dependencies. With this CL the promises keep MK/MKS alive, until the whole > > context goes away and then everything gets destroyed (in some order that may > not > > clear the promise object first). > > > > These promises can also be generated from > > navigator.requestMediaKeySystemAccess(), which is static. The crash we're > > currently seeing (in the linked bug) comes from that promise, although I > suspect > > that the current changes will prevent the crash. > > Fair enough. > > Then let's add a comment about the destruction dependency. Added comment to class header. https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h (right): https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:64: // actually reject the promise later on. On 2016/10/13 19:29:58, xhwang wrote: > Update this comment. Done. https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp (right): https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:270: return; On 2016/10/13 19:29:58, xhwang wrote: > We are checking this in both the sub class and base class. Shall we just check > in one place (e.g. in the base class) to avoid confusion? Or if all subclasses > should check, can we DCHECK in the base class? We need to check before we attempt to create any object used to resolve/reject the promise, so it's best to do it here (no need to do the work if it's not going to be used). https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:312: void complete() override { resolve(); } On 2016/10/13 19:29:58, xhwang wrote: > Just FWIW, we are not checking isValidToFulfillPromise() here, but as commented > above, we don't need to. Added. https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:320: Member<MediaKeySession> m_session; On 2016/10/13 19:29:58, xhwang wrote: > Add a comment about why we need this here. Done. https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/MediaKeySystemAccess.cpp (right): https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:48: // NOTE: Continued from step 2.8 of createMediaKeys(). On 2016/10/13 19:29:59, xhwang wrote: > empty line here since this is not specific to l.49-50? Done. https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:50: return; On 2016/10/13 19:29:59, xhwang wrote: > ditto We don't want to create the MediaKeys object if we don't need it.
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...
https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:86: m_resolver.clear(); On 2016/10/14 00:03:31, jrummell wrote: > On 2016/10/13 13:35:47, yhirano wrote: > > Is this needed? ScriptPromiseResolver is a ContextLifecycleObserver and it > > clears its internal members in its contextDestroyed, so it will not retain any > > object after that. > > If you think it's not needed I'm happy to remove it. I assumed that clearing > Member<>'s was a good thing to do, but if it doesn't make a different then we > can skip it. This doesn't change anything. Rather, it regresses performance because of the overhead of observing ContextLifecycleObserver.
Updated to remove ContextLifecycleObserver. https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:86: m_resolver.clear(); On 2016/10/14 00:05:07, haraken wrote: > On 2016/10/14 00:03:31, jrummell wrote: > > On 2016/10/13 13:35:47, yhirano wrote: > > > Is this needed? ScriptPromiseResolver is a ContextLifecycleObserver and it > > > clears its internal members in its contextDestroyed, so it will not retain > any > > > object after that. > > > > If you think it's not needed I'm happy to remove it. I assumed that clearing > > Member<>'s was a good thing to do, but if it doesn't make a different then we > > can skip it. > > This doesn't change anything. Rather, it regresses performance because of the > overhead of observing ContextLifecycleObserver. > Removed ContextLifecycleObserver.
LGTM https://codereview.chromium.org/2407013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:107: !getExecutionContext()->activeDOMObjectsAreStopped(); Add a comment about why we need to check getExecutionContext()->activeDOMObjectsAreStopped().
https://codereview.chromium.org/2407013002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:94: DCHECK(isValidToFulfillPromise()); Is this change (from PS3) correct? You once said > My concern is the order that objects are gc'd. When the context is destroyed > other objects may do cleanup first, and that might trigger a promise being > rejected. Since this contextDestroyed() hasn't been called yet, it will try to > allocate an object (which will cause problems). and so I thought this function could be called when isValidToFulfillPromise doesn't hold.
Updated. https://codereview.chromium.org/2407013002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:94: DCHECK(isValidToFulfillPromise()); On 2016/10/14 01:05:25, yhirano wrote: > Is this change (from PS3) correct? You once said > > > My concern is the order that objects are gc'd. When the context is destroyed > > other objects may do cleanup first, and that might trigger a promise being > > rejected. Since this contextDestroyed() hasn't been called yet, it will try to > > allocate an object (which will cause problems). > > and so I thought this function could be called when isValidToFulfillPromise > doesn't hold. All the complete...() methods have the check for isValidToFulfillPromise(), so it should be checked before it gets to reject(). This just verifies it, in case somebody in the future adds a call without checking it first. (e.g. see completeWithError just above this). I just noticed that the other complete() methods in this class have NOTREACHED() since they should never be called, but will add the check to them as well, just in case one slips through. https://codereview.chromium.org/2407013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/2407013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:107: !getExecutionContext()->activeDOMObjectsAreStopped(); On 2016/10/14 00:48:41, haraken wrote: > > Add a comment about why we need to check > getExecutionContext()->activeDOMObjectsAreStopped(). Done.
LGTM
lgtm
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 Link to the patchset: https://codereview.chromium.org/2407013002/#ps100001 (title: "more checks")
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: Improve promise lifetime Each promise generated by EME now keeps a link to the object that initiated it so that garbage collection doesn't clean up EME objects while there are promises outstanding. Also don't resolve/reject a promise if the ExecutionContext has been (or is in the process of being) destroyed. BUG=654556 TEST=EME tests pass (plus manual test with changes to not resolve a promise) ========== to ========== EME: Improve promise lifetime Each promise generated by EME now keeps a link to the object that initiated it so that garbage collection doesn't clean up EME objects while there are promises outstanding. Also don't resolve/reject a promise if the ExecutionContext has been (or is in the process of being) destroyed. BUG=654556 TEST=EME tests pass (plus manual test with changes to not resolve a promise) ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== EME: Improve promise lifetime Each promise generated by EME now keeps a link to the object that initiated it so that garbage collection doesn't clean up EME objects while there are promises outstanding. Also don't resolve/reject a promise if the ExecutionContext has been (or is in the process of being) destroyed. BUG=654556 TEST=EME tests pass (plus manual test with changes to not resolve a promise) ========== to ========== EME: Improve promise lifetime Each promise generated by EME now keeps a link to the object that initiated it so that garbage collection doesn't clean up EME objects while there are promises outstanding. Also don't resolve/reject a promise if the ExecutionContext has been (or is in the process of being) destroyed. BUG=654556 TEST=EME tests pass (plus manual test with changes to not resolve a promise) Committed: https://crrev.com/b2d6c9f4a63e717cb37d8963a4566e70c3bef0f5 Cr-Commit-Position: refs/heads/master@{#425277} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b2d6c9f4a63e717cb37d8963a4566e70c3bef0f5 Cr-Commit-Position: refs/heads/master@{#425277} |
