Keep reference to CDM to avoid it's premature destruction

media/blink/webmediaplayer_impl.cc

Index: media/blink/webmediaplayer_impl.cc
diff --git a/media/blink/webmediaplayer_impl.cc b/media/blink/webmediaplayer_impl.cc
index 7c67743e982b430e3c7794a1e1d2a3310806ee3c..66b9c86e698ababf2f30010270f4f6a2c86c5b3e 100644
--- a/media/blink/webmediaplayer_impl.cc
+++ b/media/blink/webmediaplayer_impl.cc
@@ -32,6 +32,7 @@
 #include "media/base/cdm_context.h"
 #include "media/base/limits.h"
 #include "media/base/media_content_type.h"
+#include "media/base/media_keys.h"
 #include "media/base/media_log.h"
 #include "media/base/media_switches.h"
 #include "media/base/text_renderer.h"
@@ -215,7 +216,6 @@ WebMediaPlayerImpl::WebMediaPlayerImpl(
                                   ? params.compositor_task_runner()
                                   : base::ThreadTaskRunnerHandle::Get()),
       compositor_(new VideoFrameCompositor(compositor_task_runner_)),
-      is_cdm_attached_(false),
 #if defined(OS_ANDROID)  // WMPI_CAST
       cast_impl_(this, client_, params.context_3d_cb()),
 #endif
@@ -244,11 +244,8 @@ WebMediaPlayerImpl::WebMediaPlayerImpl(
   media_log_->AddEvent(
       media_log_->CreateEvent(MediaLogEvent::WEBMEDIAPLAYER_CREATED));
 
-  if (params.initial_cdm()) {
-    SetCdm(base::Bind(&IgnoreCdmAttached),
-           ToWebContentDecryptionModuleImpl(params.initial_cdm())
-               ->GetCdmContext());
-  }
+  if (params.initial_cdm())
+    SetCdm(params.initial_cdm());

[xhwang, 2016/10/07 20:44:31]

Thanks for catching this bug!

[jrummell, 2016/10/07 23:46:45]

Acknowledged.

 
   // TODO(xhwang): When we use an external Renderer, many methods won't work,
   // e.g. GetCurrentFrameFromCompositor(). See http://crbug.com/434861
@@ -780,7 +777,7 @@ void WebMediaPlayerImpl::paint(blink::WebCanvas* canvas,
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   TRACE_EVENT0("media", "WebMediaPlayerImpl:paint");
 
-  if (is_cdm_attached_)
+  if (cdm_)
     return;
 
   scoped_refptr<VideoFrame> video_frame = GetCurrentFrameFromCompositor();
@@ -895,8 +892,7 @@ void WebMediaPlayerImpl::setContentDecryptionModule(
   if (!was_encrypted && watch_time_reporter_)
     CreateWatchTimeReporter();
 
-  SetCdm(BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnCdmAttached),
-         ToWebContentDecryptionModuleImpl(cdm)->GetCdmContext());
+  SetCdm(cdm);
 }
 
 void WebMediaPlayerImpl::OnEncryptedMediaInitData(
@@ -946,29 +942,49 @@ void WebMediaPlayerImpl::OnFFmpegMediaTracksUpdated(
   }
 }
 
-void WebMediaPlayerImpl::SetCdm(const CdmAttachedCB& cdm_attached_cb,
-                                CdmContext* cdm_context) {
+void WebMediaPlayerImpl::SetCdm(blink::WebContentDecryptionModule* cdm) {

[xhwang, 2016/10/07 20:44:31]

nit: DCHECK this is called on the main thread.

[jrummell, 2016/10/07 23:46:46]

Done.

+  DCHECK(cdm);
+  scoped_refptr<MediaKeys> cdm_reference =
+      ToWebContentDecryptionModuleImpl(cdm)->GetCdmReference();
+  if (!cdm_reference) {

[xhwang, 2016/10/07 20:44:31]

Shall we NOTREACHED() here as well?

[jrummell, 2016/10/07 23:46:45]

Sure. It shouldn't happen.

+    OnCdmAttached(false);
+    return;
+  }
+
+  CdmContext* cdm_context = cdm_reference->GetCdmContext();
   if (!cdm_context) {
-    cdm_attached_cb.Run(false);
+    OnCdmAttached(false);
     return;
   }
 
+  // Keep the reference to the CDM, as it shouldn't be destroyed until
+  // after the pipeline is done with it.
+  cdm_ = std::move(cdm_reference);

[xhwang, 2016/10/07 20:44:31]

I wonder whether the following could happen:

If we already have a CDM set (cdm_ != null), and that MediaKeys is destroyed by
JS so that |cdm_| holds the last ref of the old CDM. Then  SetCdm() is called
again, we could end up releasing the ref-count of the old CDM here, causing the
old CDM to be destroyed. But the media pipeline could still be using the old
CDM.

Currently we prevent resetting the CDM in RendererImpl, such that different
Renderer implementations could choose whether resetting CDM is supported or not.

Maybe we could have a |pending_cdm_| variable, to hold the CDM being set, while
|cdm_| is for a CDM that's already set. WDYT?

We could also bind the pending CDM in a callback, but callback lifetime is
tricky and I am scared of it...

[jrummell, 2016/10/07 23:46:46]

I thought we didn't allow the CDM to be changed, but it appears that the check
in setContentDecryptionModule() only prevents clearing the CDM. Done.

+
   // If CDM initialization succeeded, tell the pipeline about it.

[xhwang, 2016/10/07 20:44:31]

I don't really know what this comment means now :)

[jrummell, 2016/10/07 23:46:45]

Removed.

-  pipeline_.SetCdm(cdm_context, cdm_attached_cb);
+  pipeline_.SetCdm(cdm_context,
+                   BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnCdmAttached));

[xhwang, 2016/10/07 20:44:31]

This BIND_TO_RENDER_LOOP is not needed now that the main/media logic should be
fully buried within PipelineImpl:

https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=14758535...

[jrummell, 2016/10/07 23:46:45]

Done.

 }
 
 void WebMediaPlayerImpl::OnCdmAttached(bool success) {

[xhwang, 2016/10/07 20:44:31]

DCHECK this is called on the main thread.

[jrummell, 2016/10/07 23:46:45]

Done.

+  // If the CDM is set from the constructor there is no
+  // promise (|set_cdm_result_|) to fulfill.

[xhwang, 2016/10/07 20:44:31]

DCHECK(cdm_) here to make assumptions more clear

[jrummell, 2016/10/07 23:46:45]

Done.

   if (success) {
-    set_cdm_result_->complete();
-    set_cdm_result_.reset();
-    is_cdm_attached_ = true;
+    if (set_cdm_result_) {
+      set_cdm_result_->complete();
+      set_cdm_result_.reset();
+    }
+
     return;
   }
 
-  set_cdm_result_->completeWithError(
-      blink::WebContentDecryptionModuleExceptionNotSupportedError, 0,
-      "Unable to set MediaKeys object");
-  set_cdm_result_.reset();
+  cdm_ = nullptr;
+  if (set_cdm_result_) {
+    set_cdm_result_->completeWithError(
+        blink::WebContentDecryptionModuleExceptionNotSupportedError, 0,
+        "Unable to set MediaKeys object");
+    set_cdm_result_.reset();
+  }
 }
 
 void WebMediaPlayerImpl::OnPipelineSeeked(bool time_updated) {