EME: Improve promise lifetime

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "modules/encryptedmedia/ContentDecryptionModuleResultPromise.h"
 
 #include "bindings/core/v8/ScriptPromise.h"
 #include "bindings/core/v8/ScriptState.h"
 #include "core/dom/DOMException.h"
 #include "core/dom/ExecutionContext.h"
@@ skipping 23 matching lines @@
       // the EME spec.
       return UnknownError;
   }
 
   NOTREACHED();
   return UnknownError;
 }
 
 ContentDecryptionModuleResultPromise::ContentDecryptionModuleResultPromise(
     ScriptState* scriptState)
-    : m_resolver(ScriptPromiseResolver::create(scriptState)) {}
+    : ContextLifecycleObserver(scriptState->getExecutionContext()),
+      m_resolver(ScriptPromiseResolver::create(scriptState)) {}
 
 ContentDecryptionModuleResultPromise::~ContentDecryptionModuleResultPromise() {}
 
 void ContentDecryptionModuleResultPromise::complete() {
   NOTREACHED();
   reject(InvalidStateError, "Unexpected completion.");
 }
 
 void ContentDecryptionModuleResultPromise::completeWithContentDecryptionModule(
     WebContentDecryptionModule* cdm) {
@@ skipping 19 matching lines @@
   if (systemCode != 0) {
     if (result.isEmpty())
       result.append("Rejected with system code");
     result.append(" (");
     result.appendNumber(systemCode);
     result.append(')');
   }
   reject(WebCdmExceptionToExceptionCode(exceptionCode), result.toString());
 }
 
+void ContentDecryptionModuleResultPromise::contextDestroyed() {
+  m_resolver.clear();

[yhirano, 2016/10/13 13:35:47]

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.

[jrummell, 2016/10/14 00:03:31]

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.

[haraken, 2016/10/14 00:05:07]

This doesn't change anything. Rather, it regresses performance because of the
overhead of observing ContextLifecycleObserver.

[jrummell, 2016/10/14 00:41:55]

Removed ContextLifecycleObserver.

+}
+
 ScriptPromise ContentDecryptionModuleResultPromise::promise() {
   return m_resolver->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));
-  }
-}
+  if (!isValidToFulfillPromise())
+    return;
 
-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 contextDestroyed() is called.
+  return getExecutionContext() &&
+         !getExecutionContext()->activeDOMObjectsAreStopped();

[haraken, 2016/10/12 00:58:22]

Alternately, can we just check m_resolver?

[jrummell, 2016/10/12 17:59:14]

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.

[haraken, 2016/10/13 02:06:47]

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.

[jrummell, 2016/10/13 07:02:23]

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.

[haraken, 2016/10/13 07:58:29]

Fair enough.

Then let's add a comment about the destruction dependency.

[jrummell, 2016/10/14 00:03:31]

Added comment to class header.

 }
 
 DEFINE_TRACE(ContentDecryptionModuleResultPromise) {
   visitor->trace(m_resolver);
   ContentDecryptionModuleResult::trace(visitor);
+  ContextLifecycleObserver::trace(visitor);
 }
 
 }  // namespace blink