Remove dangling PlayingState pointers in WebRtcAudioRenderer.

content/renderer/media/webrtc_audio_renderer.cc

 // Copyright (c) 2012 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/webrtc_audio_renderer.h"
 
+#include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "build/build_config.h"
@@ skipping 31 matching lines @@
 // This is a simple wrapper class that's handed out to users of a shared
 // WebRtcAudioRenderer instance.  This class maintains the per-user 'playing'
 // and 'started' states to avoid problems related to incorrect usage which
 // might violate the implementation assumptions inside WebRtcAudioRenderer
 // (see the play reference count).
 class SharedAudioRenderer : public MediaStreamAudioRenderer {
  public:
   // Callback definition for a callback that is called when when Play(), Pause()
   // or SetVolume are called (whenever the internal |playing_state_| changes).
   typedef base::Callback<void(const blink::WebMediaStream&,
-                              WebRtcAudioRenderer::PlayingState*)>
+                              WebRtcAudioRenderer::PlayingState*,
+                              bool playing_state_deleted)>
       OnPlayStateChanged;
 
   SharedAudioRenderer(const scoped_refptr<MediaStreamAudioRenderer>& delegate,
                       const blink::WebMediaStream& media_stream,
                       const OnPlayStateChanged& on_play_state_changed)
       : delegate_(delegate),
         media_stream_(media_stream),
         started_(false),
         on_play_state_changed_(on_play_state_changed) {
     DCHECK(!on_play_state_changed_.is_null());
     DCHECK(!media_stream_.isNull());
   }
 
  protected:
   ~SharedAudioRenderer() override {
     DCHECK(thread_checker_.CalledOnValidThread());
     DVLOG(1) << __func__;
     Stop();
+    // Make extra sure no reference to the playing state is left.
+    on_play_state_changed_.Run(media_stream_, &playing_state_, true);

[tommi (sloooow) - chröme, 2017/03/17 12:44:08]

did you consider having a separate callback for when a shared renderer goes
away?

Alternatively, since a SharedAudioRenderer does have a pointer to the root
WebRtcAudioRenderer, what about calling a method such as
delegate_->OnSharedAudioRendererGone(this)?  (I suppose we can't call
delegate_->RemovePlayingState()?)

[Max Morin, 2017/03/17 14:08:04]

I went ahead and kept delegate_ as a pointer to interface and used a callback,
consistent with how OnPlayStateChanged is treated. Calling RemovePlayingState
directly isn't possible since we don't know which sources to call it with.

   }
 
   void Start() override {
     DCHECK(thread_checker_.CalledOnValidThread());
     if (started_)
       return;
     started_ = true;
     delegate_->Start();
   }
 
   void Play() override {
     DCHECK(thread_checker_.CalledOnValidThread());
     DCHECK(started_);
     if (playing_state_.playing())
       return;
     playing_state_.set_playing(true);
-    on_play_state_changed_.Run(media_stream_, &playing_state_);
+    on_play_state_changed_.Run(media_stream_, &playing_state_, false);
   }
 
   void Pause() override {
     DCHECK(thread_checker_.CalledOnValidThread());
     DCHECK(started_);
     if (!playing_state_.playing())
       return;
     playing_state_.set_playing(false);
-    on_play_state_changed_.Run(media_stream_, &playing_state_);
+    on_play_state_changed_.Run(media_stream_, &playing_state_, false);
   }
 
   void Stop() override {
     DCHECK(thread_checker_.CalledOnValidThread());
     if (!started_)
       return;
     Pause();
     started_ = false;
     delegate_->Stop();
   }
 
   void SetVolume(float volume) override {
     DCHECK(thread_checker_.CalledOnValidThread());
     DCHECK(volume >= 0.0f && volume <= 1.0f);
     playing_state_.set_volume(volume);
-    on_play_state_changed_.Run(media_stream_, &playing_state_);
+    on_play_state_changed_.Run(media_stream_, &playing_state_, false);
   }
 
   media::OutputDeviceInfo GetOutputDeviceInfo() override {
     DCHECK(thread_checker_.CalledOnValidThread());
     return delegate_->GetOutputDeviceInfo();
   }
 
   void SwitchOutputDevice(
       const std::string& device_id,
       const url::Origin& security_origin,
@@ skipping 113 matching lines @@
 
 void WebRtcAudioRenderer::Play() {
   DVLOG(1) << "WebRtcAudioRenderer::Play()";
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (playing_state_.playing())
     return;
 
   playing_state_.set_playing(true);
 
-  OnPlayStateChanged(media_stream_, &playing_state_);
+  OnPlayStateChanged(media_stream_, &playing_state_, false);
 }
 
 void WebRtcAudioRenderer::EnterPlayState() {
   DVLOG(1) << "WebRtcAudioRenderer::EnterPlayState()";
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_GT(start_ref_count_, 0) << "Did you forget to call Start()?";
   base::AutoLock auto_lock(lock_);
   if (state_ == UNINITIALIZED)
     return;
 
@@ skipping 11 matching lines @@
 }
 
 void WebRtcAudioRenderer::Pause() {
   DVLOG(1) << "WebRtcAudioRenderer::Pause()";
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!playing_state_.playing())
     return;
 
   playing_state_.set_playing(false);
 
-  OnPlayStateChanged(media_stream_, &playing_state_);
+  OnPlayStateChanged(media_stream_, &playing_state_, false);
 }
 
 void WebRtcAudioRenderer::EnterPauseState() {
   DVLOG(1) << "WebRtcAudioRenderer::EnterPauseState()";
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_GT(start_ref_count_, 0) << "Did you forget to call Start()?";
   base::AutoLock auto_lock(lock_);
   if (state_ == UNINITIALIZED)
     return;
 
@@ skipping 38 matching lines @@
   // callback may currently be executing and trying to grab the lock while we're
   // stopping the thread on which it runs.
   sink_->Stop();
 }
 
 void WebRtcAudioRenderer::SetVolume(float volume) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(volume >= 0.0f && volume <= 1.0f);
 
   playing_state_.set_volume(volume);
-  OnPlayStateChanged(media_stream_, &playing_state_);
+  OnPlayStateChanged(media_stream_, &playing_state_, false);
 }
 
 media::OutputDeviceInfo WebRtcAudioRenderer::GetOutputDeviceInfo() {
   DCHECK(thread_checker_.CalledOnValidThread());
   return sink_ ? sink_->GetOutputDeviceInfo() : media::OutputDeviceInfo();
 }
 
 base::TimeDelta WebRtcAudioRenderer::GetCurrentRenderTime() const {
   DCHECK(thread_checker_.CalledOnValidThread());
   base::AutoLock auto_lock(lock_);
@@ skipping 205 matching lines @@
   array.erase(state_it);
 
   if (array.empty())
     source_playing_states_.erase(found);
 
   return true;
 }
 
 void WebRtcAudioRenderer::OnPlayStateChanged(
     const blink::WebMediaStream& media_stream,
-    PlayingState* state) {
+    PlayingState* state,
+    bool playing_state_deleted) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  if (playing_state_deleted) {
+    // It is possible we associated |state| to a source for a track that is no
+    // longer in |media_stream|. We iterate over |source_playing_states_| to
+    // ensure there are no dangling pointers to |state| there. See
+    // crbug.com/697256.
+    for (auto it = source_playing_states_.begin();
+         it != source_playing_states_.end();) {
+      PlayingStates& states = it->second;
+      // We cannot use RemovePlayingState as it might invalidate |it|.
+      states.erase(std::remove(states.begin(), states.end(), state),
+                   states.end());
+      if (states.empty())
+        it = source_playing_states_.erase(it);
+      else
+        ++it;
+    }
+    return;
+  }
   blink::WebVector<blink::WebMediaStreamTrack> web_tracks;
   media_stream.audioTracks(web_tracks);
 
   for (const blink::WebMediaStreamTrack& web_track : web_tracks) {
     // WebRtcAudioRenderer can only render audio tracks received from a remote
     // peer. Since the actual MediaStream is mutable from JavaScript, we need
     // to make sure |web_track| is actually a remote track.
     PeerConnectionRemoteAudioTrack* const remote_track =
         PeerConnectionRemoteAudioTrack::From(
             MediaStreamAudioTrack::From(web_track));
@@ skipping 77 matching lines @@
     sink_params_ = new_sink_params;
   }
 
   // Specify the latency info to be passed to the browser side.
   new_sink_params.set_latency_tag(AudioDeviceFactory::GetSourceLatencyType(
       AudioDeviceFactory::AudioDeviceFactory::kSourceWebRtc));
   sink_->Initialize(new_sink_params, this);
 }
 
 }  // namespace content