EME: Improve promise lifetime

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

Index: third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp
diff --git a/third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp b/third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp
index 40d3e7fa1d64910517529b7aa182df489d8c7f13..6cb53412610345a24a65d8a47c146768599b3f61 100644
--- a/third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp
+++ b/third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp
@@ -38,7 +38,6 @@
 #include "modules/encryptedmedia/EncryptedMediaUtils.h"
 #include "modules/encryptedmedia/MediaKeyMessageEvent.h"
 #include "modules/encryptedmedia/MediaKeys.h"
-#include "modules/encryptedmedia/SimpleContentDecryptionModuleResultPromise.h"
 #include "platform/ContentDecryptionModuleResult.h"
 #include "platform/ContentType.h"
 #include "platform/Timer.h"
@@ -231,6 +230,9 @@ class NewSessionResultPromise : public ContentDecryptionModuleResultPromise {
   // ContentDecryptionModuleResult implementation.
   void completeWithSession(
       WebContentDecryptionModuleResult::SessionStatus status) override {
+    if (!isValidToFulfillPromise())
+      return;
+
     if (status != WebContentDecryptionModuleResult::NewSession) {
       NOTREACHED();
       reject(InvalidStateError, "Unexpected completion.");
@@ -264,6 +266,9 @@ class LoadSessionResultPromise : public ContentDecryptionModuleResultPromise {
   // ContentDecryptionModuleResult implementation.
   void completeWithSession(
       WebContentDecryptionModuleResult::SessionStatus status) override {
+    if (!isValidToFulfillPromise())
+      return;

[xhwang, 2016/10/13 19:29:58]

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?

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

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).

+
     switch (status) {
       case WebContentDecryptionModuleResult::NewSession:
         m_session->finishLoad();
@@ -292,6 +297,29 @@ class LoadSessionResultPromise : public ContentDecryptionModuleResultPromise {
   Member<MediaKeySession> m_session;
 };
 
+// This class wraps the promise resolver used by update/close/remove. The
+// implementation of complete() will resolve the promise with void. All other
+// complete() methods are not expected to be called (and will reject the
+// promise).
+class SimpleResultPromise : public ContentDecryptionModuleResultPromise {
+ public:
+  SimpleResultPromise(ScriptState* scriptState, MediaKeySession* session)
+      : ContentDecryptionModuleResultPromise(scriptState), m_session(session) {}
+
+  ~SimpleResultPromise() override {}
+
+  // ContentDecryptionModuleResultPromise implementation.
+  void complete() override { resolve(); }

[xhwang, 2016/10/13 19:29:58]

Just FWIW, we are not checking isValidToFulfillPromise() here, but as commented
above, we don't need to.

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

Added.

+
+  DEFINE_INLINE_TRACE() {
+    visitor->trace(m_session);
+    ContentDecryptionModuleResultPromise::trace(visitor);
+  }
+
+ private:
+  Member<MediaKeySession> m_session;

[xhwang, 2016/10/13 19:29:58]

Add a comment about why we need this here.

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

Done.

+};
+
 MediaKeySession* MediaKeySession::create(
     ScriptState* scriptState,
     MediaKeys* mediaKeys,
@@ -546,8 +574,7 @@ ScriptPromise MediaKeySession::update(ScriptState* scriptState,
       DOMArrayBuffer::create(response.data(), response.byteLength());
 
   // 4. Let promise be a new promise.
-  SimpleContentDecryptionModuleResultPromise* result =
-      new SimpleContentDecryptionModuleResultPromise(scriptState);
+  SimpleResultPromise* result = new SimpleResultPromise(scriptState, this);
   ScriptPromise promise = result->promise();
 
   // 5. Run the following steps asynchronously (documented in
@@ -580,8 +607,7 @@ ScriptPromise MediaKeySession::close(ScriptState* scriptState) {
     return ScriptPromise::cast(scriptState, ScriptValue());
 
   // 3. Let promise be a new promise.
-  SimpleContentDecryptionModuleResultPromise* result =
-      new SimpleContentDecryptionModuleResultPromise(scriptState);
+  SimpleResultPromise* result = new SimpleResultPromise(scriptState, this);
   ScriptPromise promise = result->promise();
 
   // 4. Run the following steps asynchronously (documented in
@@ -627,8 +653,7 @@ ScriptPromise MediaKeySession::remove(ScriptState* scriptState) {
   }
 
   // 4. Let promise be a new promise.
-  SimpleContentDecryptionModuleResultPromise* result =
-      new SimpleContentDecryptionModuleResultPromise(scriptState);
+  SimpleResultPromise* result = new SimpleResultPromise(scriptState, this);
   ScriptPromise promise = result->promise();
 
   // 5. Run the following steps asynchronously (documented in