swap the renderer_ reference to local before calling Stop()

content/renderer/media/webrtc_audio_device_impl.cc

 // Copyright 2013 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_device_impl.h"
 
 #include "base/bind.h"
 #include "base/metrics/histogram.h"
 #include "base/string_util.h"
 #include "base/win/windows_version.h"
@@ skipping 241 matching lines @@
 
   // Calling Terminate() multiple times in a row is OK.
   if (!initialized_)
     return 0;
 
   StopRecording();
   StopPlayout();
 
   // It is necessary to stop the |renderer_| before going away.
   if (renderer_) {
-    renderer_->Stop();
-    renderer_ = NULL;
+    // WebRtcAudioRenderer::Stop() will trigger RemoveAudioRenderer(), so we

[henrika (OOO until Aug 14), 2013/05/27 10:06:00]

I am stupid, but the example says:

{
//     scoped_refptr<MyFoo> a = new MyFoo();
//     scoped_refptr<MyFoo> b;
//
//     b.swap(a);
//     // now, |b| references the MyFoo object, and |a| references NULL.
//   }

[henrika (OOO until Aug 14), 2013/05/27 10:11:21]

OK, checking the swap implementation it looks like:

renderer_ will be NULL, and
renderer will be set to valid renderer_

but it is still not clear what that solves.

+    // have to swap the member reference to a local reference here.
+    scoped_refptr<WebRtcAudioRenderer> renderer;
+    renderer_.swap(renderer);

[tommi (sloooow) - chröme, 2013/05/27 10:48:13]

I don't think swap() is the right thing here because it explicitly sets
renderer_ to NULL even before RemoveAudioRenderer() is called.  If you run the
code (and assuming that RemoveAudioRenderer() is called as you say), then you
should hit a DCHECK.

Instead, what about doing this instead:

if (renderer_) {
  // Grab a local reference while we call Stop().
  scoped_refptr<WebRtcAudioRenderer> local_renderer(renderer_);
  local_renderer->Stop();
  // The above call to Stop() should have triggered a call to
RemoveAudioRenderer
  // which clears our reference to the audio renderer.
  DCHECK(!renderer_.get());
}

[no longer working on chromium, 2013/05/27 11:15:41]

Done.

+    renderer->Stop();
   }
 
   if (capturer_) {
     // |capturer_| is stopped by the media stream, so do not need to
     // call Stop() here.
     capturer_->RemoveSink(this);
     capturer_ = NULL;
   }
 
   initialized_ = false;
@@ skipping 222 matching lines @@
     return false;
 
   if (!renderer->Initialize(this))
     return false;
 
   renderer_ = renderer;
   return true;
 }
 
 }  // namespace content