EME: Improve promise lifetime

third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp

Index: third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp
diff --git a/third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp b/third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp
index 58b35116e66ac7d09d376be848413d8994f3eb3a..84179e3dbd09ef9fdd7ea3f1813598ce8ba16022 100644
--- a/third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp
+++ b/third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp
@@ -41,7 +41,8 @@ ExceptionCode WebCdmExceptionToExceptionCode(
 
 ContentDecryptionModuleResultPromise::ContentDecryptionModuleResultPromise(
     ScriptState* scriptState)
-    : m_resolver(ScriptPromiseResolver::create(scriptState)) {}
+    : ContextLifecycleObserver(scriptState->getExecutionContext()),
+      m_resolver(ScriptPromiseResolver::create(scriptState)) {}
 
 ContentDecryptionModuleResultPromise::~ContentDecryptionModuleResultPromise() {}
 
@@ -66,6 +67,9 @@ void ContentDecryptionModuleResultPromise::completeWithError(
     WebContentDecryptionModuleException exceptionCode,
     unsigned long systemCode,
     const WebString& errorMessage) {
+  if (!isValidToFulfillPromise())
+    return;
+
   // Non-zero |systemCode| is appended to the |errorMessage|. If the
   // |errorMessage| is empty, we'll report "Rejected with system code
   // (systemCode)".
@@ -87,36 +91,22 @@ ScriptPromise ContentDecryptionModuleResultPromise::promise() {
 
 void ContentDecryptionModuleResultPromise::reject(ExceptionCode code,
                                                   const String& errorMessage) {
-  // Reject the promise asynchronously. This avoids problems when gc is
-  // destroying objects that result in unfulfilled promises being rejected.
-  // (Resolving promises is still done synchronously as there may be events
-  // already posted that need to happen only after the promise is resolved.)
-  // TODO(jrummell): Make resolving a promise asynchronous as well (including
-  // making sure events still happen after the promise is resolved).
-  if (getExecutionContext()) {
-    getExecutionContext()->postTask(
-        BLINK_FROM_HERE,
-        createSameThreadTask(
-            &ContentDecryptionModuleResultPromise::rejectInternal,
-            wrapPersistent(this), code, errorMessage));
-  }
-}
+  DCHECK(isValidToFulfillPromise());

[yhirano, 2016/10/14 01:05:25]

Is this change (from PS3) correct? You once said
and so I thought this function could be called when isValidToFulfillPromise
doesn't hold.

[jrummell, 2016/10/14 02:13:30]

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.

 
-void ContentDecryptionModuleResultPromise::rejectInternal(
-    ExceptionCode code,
-    const String& errorMessage) {
   m_resolver->reject(DOMException::create(code, errorMessage));
   m_resolver.clear();
 }
 
-ExecutionContext* ContentDecryptionModuleResultPromise::getExecutionContext()
-    const {
-  return m_resolver->getExecutionContext();
+bool ContentDecryptionModuleResultPromise::isValidToFulfillPromise() {
+  // getExecutionContext() is no longer valid once the context is destroyed.
+  return getExecutionContext() &&
+         !getExecutionContext()->activeDOMObjectsAreStopped();
 }
 
 DEFINE_TRACE(ContentDecryptionModuleResultPromise) {
   visitor->trace(m_resolver);
   ContentDecryptionModuleResult::trace(visitor);
+  ContextLifecycleObserver::trace(visitor);
 }
 
 }  // namespace blink