MediaRecorder: make MediaRecorderHandler a MediaStreamObserver

content/renderer/media/media_stream.cc

 // 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 "content/renderer/media/media_stream.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
-#include "third_party/WebKit/public/platform/WebString.h"
 
 namespace content {
 
 // static
-MediaStream* MediaStream::GetMediaStream(
-    const blink::WebMediaStream& stream) {
+MediaStream* MediaStream::GetMediaStream(const blink::WebMediaStream& stream) {
+  DCHECK(stream.extraData());
   return static_cast<MediaStream*>(stream.extraData());
 }
 
 MediaStream::MediaStream() {
 }
 
 MediaStream::~MediaStream() {
-  DCHECK(observers_.empty());
+  observers_.clear();

[perkj_chrome, 2016/01/07 09:18:00]

Why this change? Is there a case when the observers can outlive the MediaStream?
It that case, how do they know that they can not call RemoveObserver ? 

[mcasas, 2016/01/08 00:14:12]

Yes, MediaRecorder is destroyed independently from the MediaStream().
Because this is the destructor of the class, that is accessed
via l.14
MediaStream* MediaStream::GetMediaStream(...)
once the dtor has been called, noone should try to
de-register as observer, right?

Correct me if I'm wrong, but this DCHECK was there to
debug some strange race condition appeared on garbage
collection between the only current observer, WebRtc,
and this MediaStream. But honestly, it does not make
sense that MediaStream waits for the Observers to
deregister before going down, specially taken into 
account the complete lack of guarantee from Blink on
the destruction order.

Note that I brought this up in the CL description.

[perkj_chrome, 2016/01/08 07:54:50]

Right - and hence the DCHECK that no observer is still valid. Because that could
lead to a heap use after free if the observer call RemoveObserver after the
dtor.

A content::MediaStream will never be destroyed as long as the blink MediaStream
exist. And the blink MediaStream exists as long has there is a reference to it.

So the DCHECK(observers_.empty()) is correct I would say.

 }
 
 void MediaStream::AddObserver(MediaStreamObserver* observer) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(std::find(observers_.begin(), observers_.end(), observer) ==
       observers_.end());
   observers_.push_back(observer);
 }
 
 void MediaStream::RemoveObserver(MediaStreamObserver* observer) {
@@ skipping 16 matching lines @@
 bool MediaStream::RemoveTrack(const blink::WebMediaStreamTrack& track) {
   DCHECK(thread_checker_.CalledOnValidThread());
   for (std::vector<MediaStreamObserver*>::iterator it = observers_.begin();
        it != observers_.end(); ++it) {
     (*it)->TrackRemoved(track);
   }
   return true;
 }
 
 }  // namespace content